On Wed, 26 Nov 2025 15:59:36 GMT, Sean Mullan <[email protected]> wrote:
>> Hai-May Chao has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Update names to uppercase
>> - Remove fallback in engineGeneratePublic
>> - Change default named group list to have only X25519MLKEM768
>
> test/micro/org/openjdk/bench/javax/crypto/full/KEMBench.java line 94:
>
>> 92: }
>> 93:
>> 94: protected static Provider getInternalJce() {
>
> Can this just be private?
It sure can! Done.
> test/micro/org/openjdk/bench/javax/crypto/full/KEMBench.java line 97:
>
>> 95: try {
>> 96: Class<?> dhClazz =
>> Class.forName("com.sun.crypto.provider.DH");
>> 97: return (Provider) (dhClazz.getField("PROVIDER").get(null));
>
> Nit: params around `dhClazz.getField("PROVIDER").get(null)` not necessary.
I assume you meant "parenthesis"? If so I've removed the extra set.
> test/micro/org/openjdk/bench/javax/crypto/full/KEMBench.java line 120:
>
>> 118:
>> 119: public static class MLKEM extends KEMBench {
>> 120: @Param({"ML-KEM-512", "ML-KEM-768", "ML-KEM-1024" })
>
> Not a JMH expert, but what is the difference between these parameters and the
> ones on line 48? It looks like a duplicate test to me.
It turns out this is a mistake. By having both this line and 48 with the same
algorithm names, the benchmark runs them twice. Our benchmarks tend to be
organized either with the main class as a baseline benchmark with subclasses
for all other types, or an abstract base class with an empty @param tag and
subclasses for each actual benchmark. `KeyAgreementBench.java` is done that
way, and I personally like that approach better and changed this class to
follow that model. Either way is just fine though. The important thing is
that if you have parameters in the parent class, it should not match any
parameter sets in the child classes or you just end up running the same
benchmark twice.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595703134
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595703039
PR Review Comment: https://git.openjdk.org/jdk/pull/27614#discussion_r2595702480
