On Tue, 3 Nov 2026 22:37:25 GMT, Volodymyr Paprotski <[email protected]>
wrote:
>> I added an AVX2 version of AES/CTR intrinsic. Rather then adding a third
>> version of that intrinsic (we already have an AVX512 and an AES128), I
>> refactored existing ones so I could parametrize just one intrinsic. AVX512
>> got better at small payloads and AVX128 got better at large payloads. And
>> ECore/AVX256 used to use fall back to 128, now it doesn't.
>>
>> | Benchmark | Data | Key | AVX128 | AVX256 | AVX512 |
>> |---|---|---|---|---|---|
>> | AESBench.encrypt | 16 | 128 | 0% | 5% | 92% |
>> | AESBench.encrypt | 32 | 128 | 0% | 1% | 37% |
>> | AESBench.encrypt | 64 | 128 | 23% | 33% | 30% |
>> | AESBench.encrypt | 128 | 128 | 6% | 1% | 35% |
>> | AESBench.encrypt | 256 | 128 | 26% | 16% | 4% |
>> | AESBench.encrypt | 1024 | 128 | 29% | 13% | 0% |
>> | AESBench.encrypt | 16384 | 128 | 24% | 13% | 7% |
>> | AESBench.encrypt | 16 | 192 | 3% | 5% | 69% |
>> | AESBench.encrypt | 32 | 192 | 1% | 4% | 39% |
>> | AESBench.encrypt | 64 | 192 | 30% | 42% | 30% |
>> | AESBench.encrypt | 128 | 192 | 12% | 13% | 30% |
>> | AESBench.encrypt | 256 | 192 | 32% | 19% | 6% |
>> | AESBench.encrypt | 1024 | 192 | 25% | 15% | 1% |
>> | AESBench.encrypt | 16384 | 192 | 20% | 12% | 8% |
>> | AESBench.encrypt | 16 | 256 | -1% | 4% | 62% |
>> | AESBench.encrypt | 32 | 256 | 2% | 5% | 31% |
>> | AESBench.encrypt | 64 | 256 | 31% | 67% | 18% |
>> | AESBench.encrypt | 128 | 256 | 13% | 16% | 29% |
>> | AESBench.encrypt | 256 | 256 | 33% | 18% | 4% |
>> | AESBench.encrypt | 1024 | 256 | 23% | 15% | -2% |
>> | AESBench.encrypt | 16384 | 256 | 19% | 13% | 9% |
>>
>> Just note, measuring exact performance with our current AESBench JMH, and
>> especially at small payloads, can be an exercise in frustration. The
>> benchmark constantly allocates the result and discards it, producing a lot
>> of GC. (While developing, I instrumented the code to call the intrinsic
>> directly, but that involved making many methods public.. so not something
>> that can be checked in.)
>>
>> This is the invocation that gave me the most stable/repeatable results
>> (still swings on reruns, but not as much.. a very long run however).
>>
>>
>> HEAP="-Xms4g -Xmx4g"
>> JMH="-f 3 -i 10 -r 3 -wi 5 -w 5"
>> NEW=./build/linux-x86_64-server-release
>> BASE=./build/base
>> BENCH=org.openjdk.bench.javax.crypto.full.AESBench
>> SIZES="16 32 64 128 256 1024 16384"
>> ARGS="-p algorithm=AES/CTR/NoPadding"
>> for size in $SIZES; do
>> $NEW/images/jdk/bin/java -jar $NEW/images/test/micro/benchmarks.jar
>> $JMH -jvmArgsAppend "$HEAP" $ARGS -p dataSize=$size $BENCH 2>&1 | te...
>
> Volodymyr Paprotski has updated the pull request incrementally with one
> additional commit since the last revision:
>
> comments from Sandhya
src/hotspot/cpu/x86/assembler_x86.cpp line 1578:
> 1576:
> 1577: void Assembler::vaesenc(XMMRegister dst, XMMRegister nds, XMMRegister
> src, int vector_len) {
> 1578: assert(vector_len == AVX_128bit ? (VM_Version::supports_aes() &&
> VM_Version::supports_avx()) || // VEX
Wondering if we should restrict this and other vaes instructions to only
supports_vaes().
i.e. remove VM_Version::supports_aes() && VM_Version::supports_avx().
It is bit conservative but meets our usage.
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 161:
> 159: 0x0000000000000000ULL, 0x0000000000000000ULL,
> 160:
> 161: // [0:15] Sequence 0-8
Should this be Sequence 0-3
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 161:
> 159: 0x0000000000000000ULL, 0x0000000000000000ULL,
> 160:
> 161: // [0:15] Sequence 0-8
Should this be Sequence 0-3?
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 345:
> 343: }
> 344:
> 345: void vpxor(XMMRegister dst, XMMRegister nds, Address src, int
> vector_len, XMMRegister scratch = xmm1) {
Wonder if it would be good to pass scratch always. When not needed xnoreg could
be passed.
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 352:
> 350: if (dst != nds) {
> 351: movdqu(dst, src);
> 352: pxor(dst, nds);
Assert here:
assert(scratch == noreg);
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 356:
> 354: movdqu(scratch, src);
> 355: pxor(dst, scratch);
> 356: }
Assert here:
assert(scratch != noreg);
assert_different_registers(dst, scratch);
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 565:
> 563: Ctr[i] = _next;
> 564: }
> 565: for (int i = 0; i<15 && _next->is_valid(); i++, _next =
> _next->successor()) {
Should this be i < keyRegs?
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 664:
> 662: }
> 663: };
> 664: keysAndRounds(true, 0, 10, -1, 2);
May be add a comment here to say why -1?
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 926:
> 924:
> 925: // record the stub entry and end
> 926: stubgen->store_archive_data(stub_id, start, __ pc());
store_archive_data should be called after __ ret(0).
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 954:
> 952: if (UseAESCTRIntrinsics) {
> 953: int vector_len = Assembler::AVX_128bit;
> 954: if (VM_Version::supports_vaes() && VM_Version::supports_avx512bw()
> && VM_Version::supports_avx512vl()) {
We also need avx512dq() for evbroadcasti64x2.
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 956:
> 954: if (VM_Version::supports_vaes() && VM_Version::supports_avx512bw()
> && VM_Version::supports_avx512vl()) {
> 955: vector_len = Assembler::AVX_512bit;
> 956: } else if (VM_Version::supports_vaes()) {
Check for E-core here?
src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 959:
> 957: vector_len = Assembler::AVX_256bit;
> 958: } else {
> 959: assert(VM_Version::supports_aes() &&
> VM_Version::supports_sse4_1(), "AESNI intrinsic unsupported");
This should be VM_Version::supports_sse4_2() (pcmpgtq needs sse4_2).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498801591
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498813931
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498815255
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498859250
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498862343
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498860151
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498648491
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498657836
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498457891
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498466481
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498695568
PR Review Comment: https://git.openjdk.org/jdk/pull/33128#discussion_r4498500395
