Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]
On Mon, 20 Sep 2021 16:44:58 GMT, Anthony Scarpino wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain >> and added a new constant for htbl entries > > I approve the jdk changes. You'll need a hotspot reviewer to approve the > other changes @ascarpino Is it okay to integrate this patch? - PR: https://git.openjdk.java.net/jdk/pull/5402
Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]
On Mon, 20 Sep 2021 16:44:58 GMT, Anthony Scarpino wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain >> and added a new constant for htbl entries > > I approve the jdk changes. You'll need a hotspot reviewer to approve the > other changes @ascarpino I've modified the code style. Thank you. - PR: https://git.openjdk.java.net/jdk/pull/5402
Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]
On Mon, 20 Sep 2021 16:44:58 GMT, Anthony Scarpino wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain >> and added a new constant for htbl entries > > I approve the jdk changes. You'll need a hotspot reviewer to approve the > other changes @ascarpino I dont see your comments on this PR. Could you please post them again? - PR: https://git.openjdk.java.net/jdk/pull/5402
Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]
I'll run them.. Did you seem my comments? They are just code-style comments thanks Tony On 9/21/21 12:43 PM, Smita Kamath wrote: On Mon, 20 Sep 2021 16:44:58 GMT, Anthony Scarpino wrote: Smita Kamath has updated the pull request incrementally with one additional commit since the last revision: Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain and added a new constant for htbl entries I approve the jdk changes. You'll need a hotspot reviewer to approve the other changes @ascarpino Could you please run tier 1-3 tests? We have two reviewers for the patch. - PR: https://git.openjdk.java.net/jdk/pull/5402
Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]
On Mon, 20 Sep 2021 16:44:58 GMT, Anthony Scarpino wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain >> and added a new constant for htbl entries > > I approve the jdk changes. You'll need a hotspot reviewer to approve the > other changes @ascarpino Could you please run tier 1-3 tests? We have two reviewers for the patch. - PR: https://git.openjdk.java.net/jdk/pull/5402
Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]
On Tue, 21 Sep 2021 16:37:49 GMT, Sandhya Viswanathan wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain >> and added a new constant for htbl entries > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 588: > >> 586: ctOfs+len, out, outOfs+len, gctr, ghash); >> 587: len+= partlen; >> 588: inLen-= len; > > This should be inLen -= partlen; Done. Thank you for pointing this out. - PR: https://git.openjdk.java.net/jdk/pull/5402
Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]
On Mon, 20 Sep 2021 05:16:16 GMT, Smita Kamath wrote: >> Performance dropped up to 10% for 1k data after 8267125 for CPUs that do not >> support the new intrinsic. Tests run were crypto.full.AESGCMBench and >> crypto.full.AESGCMByteBuffer from the jmh micro benchmarks. >> >> The problem is each instance of GHASH allocates 96 extra longs for the >> AVX512+VAES intrinsic regardless if the intrinsic is used. This extra table >> space should be allocated differently so that non-supporting CPUs do not >> suffer this penalty. This issue also affects non-Intel CPUs too. > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain > and added a new constant for htbl entries src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 588: > 586: ctOfs+len, out, outOfs+len, gctr, ghash); > 587: len+= partlen; > 588: inLen-= len; This should be inLen -= partlen; - PR: https://git.openjdk.java.net/jdk/pull/5402
Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]
On Mon, 20 Sep 2021 05:16:16 GMT, Smita Kamath wrote: >> Performance dropped up to 10% for 1k data after 8267125 for CPUs that do not >> support the new intrinsic. Tests run were crypto.full.AESGCMBench and >> crypto.full.AESGCMByteBuffer from the jmh micro benchmarks. >> >> The problem is each instance of GHASH allocates 96 extra longs for the >> AVX512+VAES intrinsic regardless if the intrinsic is used. This extra table >> space should be allocated differently so that non-supporting CPUs do not >> suffer this penalty. This issue also affects non-Intel CPUs too. > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain > and added a new constant for htbl entries I approve the jdk changes. You'll need a hotspot reviewer to approve the other changes - Marked as reviewed by ascarpino (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5402
Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]
> Performance dropped up to 10% for 1k data after 8267125 for CPUs that do not > support the new intrinsic. Tests run were crypto.full.AESGCMBench and > crypto.full.AESGCMByteBuffer from the jmh micro benchmarks. > > The problem is each instance of GHASH allocates 96 extra longs for the > AVX512+VAES intrinsic regardless if the intrinsic is used. This extra table > space should be allocated differently so that non-supporting CPUs do not > suffer this penalty. This issue also affects non-Intel CPUs too. Smita Kamath has updated the pull request incrementally with one additional commit since the last revision: Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain and added a new constant for htbl entries - Changes: - all: https://git.openjdk.java.net/jdk/pull/5402/files - new: https://git.openjdk.java.net/jdk/pull/5402/files/4628dc3a..7ea464ae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5402=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5402=00-01 Stats: 42 lines in 5 files changed: 28 ins; 1 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/5402.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5402/head:pull/5402 PR: https://git.openjdk.java.net/jdk/pull/5402
