Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Tue, 16 May 2023 09:18:57 GMT, Aleksey Shipilev wrote: >> One of our services has a hot path with AES/GCM cipher reuse. The JDK code >> reinitializes the session key on that path, and >> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up >> prominently there. >> >> Fixing [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) would take >> a while, as would likely require multiple patches in VM internals. >> Meanwhile, we can avoid the multiarray allocations in >> AESCrypt.makeSessionKey completely, reaping performance benefits. We can go >> even deeper: replace the multi-array with the flat array and drop >> `expandToSubKey` completely. >> >> Example original profile is in the bug. >> >> There are other things we can polish in that code, but experiments show >> those polishings have rather diminshed returns. >> >> On new benchmark: >> >> >> Benchmark Mode CntScore Error Units >> >> ## Mac M1 >> >> # Before >> AESReinit.test avgt 15 873,842 ± 6,911 ns/op >> >> # After >> AESReinit.test avgt 15 347,632 ± 8,764 ns/op ; <--- 2.5x faster >> >> ## Xeon, c6.8xlarge >> >> # Before >> AESReinit.test avgt 15 1524.307 ± 24.231 ns/op >> >> # After >> AESReinit.test avgt 15 554.727 ± 12.876 ns/op ; <--- 2.75x faster >> >> ## Graviton, m6g.4xlarge >> >> # Before >> AESReinit.test avgt 15 1913.492 ± 23.489 ns/op >> >> # After >> AESReinit.test avgt 15 639.701 ± 5.033 ns/op ; <--- 2.99x faster >> >> >> Additional testing: >> - [x] Benchmarks >> - [x] macos-aarch64-server-release, `jdk_security` >> - [x] linux-x86_64-server-fastdebug, `tier1 tier2 tier3` > > Aleksey Shipilev has updated the pull request incrementally with three > additional commits since the last revision: > > - Micro-optimizations > - Handle Kd > - Handle Ke Rechecked `jdk_security`, passes. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/13996#issuecomment-1554093536
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Thu, 18 May 2023 07:18:04 GMT, Aleksey Shipilev wrote: > > jdk_security or tier2. > > Gotcha, I already tested both, see "Additional Testing" section in PR. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/13996#issuecomment-1553126729
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Thu, 18 May 2023 07:16:31 GMT, Xue-Lei Andrew Fan wrote: > jdk_security or tier2. Gotcha, I already tested both, see "Additional Testing" section in PR. - PR Comment: https://git.openjdk.org/jdk/pull/13996#issuecomment-1552619095
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Thu, 18 May 2023 06:44:10 GMT, Aleksey Shipilev wrote: > > Looks good to me. Please make sure the security regression testing passed. > > Thanks! By "security regression testing" that you mean `jdk_security`, or > something else? jdk_security or tier2. - PR Comment: https://git.openjdk.org/jdk/pull/13996#issuecomment-1552616281
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Wed, 17 May 2023 20:09:50 GMT, Xue-Lei Andrew Fan wrote: > Looks good to me. Please make sure the security regression testing passed. Thanks! By "security regression testing" that you mean `jdk_security`, or something else? - PR Comment: https://git.openjdk.org/jdk/pull/13996#issuecomment-1552552955
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Tue, 16 May 2023 09:18:57 GMT, Aleksey Shipilev wrote: >> One of our services has a hot path with AES/GCM cipher reuse. The JDK code >> reinitializes the session key on that path, and >> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up >> prominently there. >> >> Fixing [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) would take >> a while, as would likely require multiple patches in VM internals. >> Meanwhile, we can avoid the multiarray allocations in >> AESCrypt.makeSessionKey completely, reaping performance benefits. We can go >> even deeper: replace the multi-array with the flat array and drop >> `expandToSubKey` completely. >> >> Example original profile is in the bug. >> >> There are other things we can polish in that code, but experiments show >> those polishings have rather diminshed returns. >> >> On new benchmark: >> >> >> Benchmark Mode CntScore Error Units >> >> ## Mac M1 >> >> # Before >> AESReinit.test avgt 15 873,842 ± 6,911 ns/op >> >> # After >> AESReinit.test avgt 15 347,632 ± 8,764 ns/op ; <--- 2.5x faster >> >> ## Xeon, c6.8xlarge >> >> # Before >> AESReinit.test avgt 15 1524.307 ± 24.231 ns/op >> >> # After >> AESReinit.test avgt 15 554.727 ± 12.876 ns/op ; <--- 2.75x faster >> >> ## Graviton, m6g.4xlarge >> >> # Before >> AESReinit.test avgt 15 1913.492 ± 23.489 ns/op >> >> # After >> AESReinit.test avgt 15 639.701 ± 5.033 ns/op ; <--- 2.99x faster >> >> >> Additional testing: >> - [x] Benchmarks >> - [x] macos-aarch64-server-release, `jdk_security` >> - [x] linux-x86_64-server-fastdebug, `tier1 tier2 tier3` > > Aleksey Shipilev has updated the pull request incrementally with three > additional commits since the last revision: > > - Micro-optimizations > - Handle Kd > - Handle Ke I don't think my review counts, but this looks good to me - Marked as reviewed by schlo...@github.com (no known OpenJDK username). PR Review: https://git.openjdk.org/jdk/pull/13996#pullrequestreview-1431597342
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Tue, 16 May 2023 09:18:57 GMT, Aleksey Shipilev wrote: >> One of our services has a hot path with AES/GCM cipher reuse. The JDK code >> reinitializes the session key on that path, and >> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up >> prominently there. >> >> Fixing [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) would take >> a while, as would likely require multiple patches in VM internals. >> Meanwhile, we can avoid the multiarray allocations in >> AESCrypt.makeSessionKey completely, reaping performance benefits. We can go >> even deeper: replace the multi-array with the flat array and drop >> `expandToSubKey` completely. >> >> Example original profile is in the bug. >> >> There are other things we can polish in that code, but experiments show >> those polishings have rather diminshed returns. >> >> On new benchmark: >> >> >> Benchmark Mode CntScore Error Units >> >> ## Mac M1 >> >> # Before >> AESReinit.test avgt 15 873,842 ± 6,911 ns/op >> >> # After >> AESReinit.test avgt 15 347,632 ± 8,764 ns/op ; <--- 2.5x faster >> >> ## Xeon, c6.8xlarge >> >> # Before >> AESReinit.test avgt 15 1524.307 ± 24.231 ns/op >> >> # After >> AESReinit.test avgt 15 554.727 ± 12.876 ns/op ; <--- 2.75x faster >> >> ## Graviton, m6g.4xlarge >> >> # Before >> AESReinit.test avgt 15 1913.492 ± 23.489 ns/op >> >> # After >> AESReinit.test avgt 15 639.701 ± 5.033 ns/op ; <--- 2.99x faster >> >> >> Additional testing: >> - [x] Benchmarks >> - [x] macos-aarch64-server-release, `jdk_security` >> - [x] linux-x86_64-server-fastdebug, `tier1 tier2 tier3` > > Aleksey Shipilev has updated the pull request incrementally with three > additional commits since the last revision: > > - Micro-optimizations > - Handle Kd > - Handle Ke Looks good to me. Please make sure the security regression testing passed. - Marked as reviewed by xuelei (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13996#pullrequestreview-1431510226
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Wed, 17 May 2023 12:57:15 GMT, Aleksey Shipilev wrote: > @XueleiFan, or anyone else, please take a look? I will have a look, but I may need more time. - PR Comment: https://git.openjdk.org/jdk/pull/13996#issuecomment-1551895053
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Tue, 16 May 2023 09:18:57 GMT, Aleksey Shipilev wrote: >> One of our services has a hot path with AES/GCM cipher reuse. The JDK code >> reinitializes the session key on that path, and >> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up >> prominently there. >> >> Fixing [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) would take >> a while, as would likely require multiple patches in VM internals. >> Meanwhile, we can avoid the multiarray allocations in >> AESCrypt.makeSessionKey completely, reaping performance benefits. We can go >> even deeper: replace the multi-array with the flat array and drop >> `expandToSubKey` completely. >> >> Example original profile is in the bug. >> >> There are other things we can polish in that code, but experiments show >> those polishings have rather diminshed returns. >> >> On new benchmark: >> >> >> Benchmark Mode CntScore Error Units >> >> ## Mac M1 >> >> # Before >> AESReinit.test avgt 15 873,842 ± 6,911 ns/op >> >> # After >> AESReinit.test avgt 15 347,632 ± 8,764 ns/op ; <--- 2.5x faster >> >> ## Xeon, c6.8xlarge >> >> # Before >> AESReinit.test avgt 15 1524.307 ± 24.231 ns/op >> >> # After >> AESReinit.test avgt 15 554.727 ± 12.876 ns/op ; <--- 2.75x faster >> >> ## Graviton, m6g.4xlarge >> >> # Before >> AESReinit.test avgt 15 1913.492 ± 23.489 ns/op >> >> # After >> AESReinit.test avgt 15 639.701 ± 5.033 ns/op ; <--- 2.99x faster >> >> >> Additional testing: >> - [x] Benchmarks >> - [x] macos-aarch64-server-release, `jdk_security` >> - [x] linux-x86_64-server-fastdebug, `tier1 tier2 tier3` > > Aleksey Shipilev has updated the pull request incrementally with three > additional commits since the last revision: > > - Micro-optimizations > - Handle Kd > - Handle Ke @XueleiFan, or anyone else, please take a look? - PR Comment: https://git.openjdk.org/jdk/pull/13996#issuecomment-1551343907
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Tue, 16 May 2023 09:28:11 GMT, Aleksey Shipilev wrote: >> True, let me try that! > > New commit implements this, with even more performance benefits. Excellent, thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/13996#discussion_r1195043072
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Tue, 16 May 2023 06:28:17 GMT, Aleksey Shipilev wrote: >> src/java.base/share/classes/com/sun/crypto/provider/AESCrypt.java line 1372: >> >>> 1370: >>> 1371: // It is significantly faster to allocate individual arrays, >>> 1372: // instead of doing the multi-array allocation. See >>> JDK-8308105. >> >> Alternatively, could the multi-array allocation get improved? > > Yes, it could and it eventually would, but it would take significantly more > work: there are multiple issues of differing complexity, impact, and risk. > This workaround, on the other hand, is straight-forward and backportable. Please look again, I think we can improve this code even further, and that is where JVM optos would not help us much :) - PR Review Comment: https://git.openjdk.org/jdk/pull/13996#discussion_r1194898514
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
On Tue, 16 May 2023 08:29:52 GMT, Aleksey Shipilev wrote: >> src/java.base/share/classes/com/sun/crypto/provider/AESCrypt.java line 1369: >> >>> 1367: int BC = 4; >>> 1368: int[][] Ke = new int[ROUNDS + 1][]; // encryption round keys >>> 1369: int[][] Kd = new int[ROUNDS + 1][]; // decryption round keys >> >> This is more a question of curiosity and likely beyond the scope of this >> PR/bug ID -- would it be beneficial to refactor this to allocate and operate >> over just two arrays, one for Ke and one for Kd, effectively removing the >> need for allocation in `expandToSubKey` as well? >> >> >> int[] Ke = new int[4 * (ROUNDS + 1)]; >> int[] Kd = new int[4 * (ROUNDS + 1)]; > > True, let me try that! New commit implements this, with even more performance benefits. - PR Review Comment: https://git.openjdk.org/jdk/pull/13996#discussion_r1194883146
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]
> One of our services has a hot path with AES/GCM cipher reuse. The JDK code > reinitializes the session key on that path, and > [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up > prominently there. While > [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) is being fixed, it > would take significantly more work: there are multiple issues of differing > complexity, impact, and risk. > > Meanwhile, we can work this issue around by avoiding the multiarray > allocations in AESCrypt.makeSessionKey. We can go even deeper: replace the > multi-array with the flat array and drop `expandToSubKey` completely. > > > Example profile is in the bug. > > On new benchmark and M1 Mac: > > > Benchmark Mode CntScore Error Units > > # Before > AESReinit.test avgt 15 873,842 ± 6,911 ns/op > > # After > AESReinit.test avgt 15 347,632 ± 8,764 ns/op > > > Additional testing: > - [x] Benchmarks > - [ ] macos-aarch64-server-release, `jdk_security` > - [ ] linux-x86_64-server-fastdebug, `tier1 tier2` > - [ ] linux-aarch64-server-fastdebug, `tier1 tier2` Aleksey Shipilev has updated the pull request incrementally with three additional commits since the last revision: - Micro-optimizations - Handle Kd - Handle Ke - Changes: - all: https://git.openjdk.org/jdk/pull/13996/files - new: https://git.openjdk.org/jdk/pull/13996/files/7842e0e2..bb712183 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13996&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13996&range=00-01 Stats: 72 lines in 1 file changed: 9 ins; 43 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/13996.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13996/head:pull/13996 PR: https://git.openjdk.org/jdk/pull/13996
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey
On Tue, 16 May 2023 01:44:24 GMT, David Schlosnagle wrote: >> One of our services has a hot path with AES/GCM cipher reuse. The JDK code >> reinitializes the session key on that path, and >> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up >> prominently there. While >> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) is being fixed, >> it would take significantly more work: there are multiple issues of >> differing complexity, impact, and risk. >> >> Meanwhile, we can work this issue around by avoiding the multiarray >> allocations in AESCrypt.makeSessionKey. This workaround is straight-forward >> and backportable. >> >> Example profile is in the bug. >> >> On new benchmark and M1 Mac: >> >> >> Benchmark Mode CntScore Error Units >> >> # Before >> AESReinit.test avgt 15 873,842 ± 6,911 ns/op >> >> # After >> AESReinit.test avgt 15 533,018 ± 4,048 ns/op >> >> >> Additional testing: >> - [x] Benchmarks >> - [x] macos-aarch64-server-release, `jdk_security` >> - [ ] linux-x86_64-server-fastdebug, `tier1 tier2` >> - [x] linux-aarch64-server-fastdebug, `tier1 tier2` > > src/java.base/share/classes/com/sun/crypto/provider/AESCrypt.java line 1369: > >> 1367: int BC = 4; >> 1368: int[][] Ke = new int[ROUNDS + 1][]; // encryption round keys >> 1369: int[][] Kd = new int[ROUNDS + 1][]; // decryption round keys > > This is more a question of curiosity and likely beyond the scope of this > PR/bug ID -- would it be beneficial to refactor this to allocate and operate > over just two arrays, one for Ke and one for Kd, effectively removing the > need for allocation in `expandToSubKey` as well? > > > int[] Ke = new int[4 * (ROUNDS + 1)]; > int[] Kd = new int[4 * (ROUNDS + 1)]; True, let me try that! - PR Review Comment: https://git.openjdk.org/jdk/pull/13996#discussion_r1194807087
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey
On Mon, 15 May 2023 20:51:21 GMT, Xue-Lei Andrew Fan wrote: >> One of our services has a hot path with AES/GCM cipher reuse. The JDK code >> reinitializes the session key on that path, and >> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up >> prominently there. While >> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) is being fixed, >> it would take significantly more work: there are multiple issues of >> differing complexity, impact, and risk. >> >> Meanwhile, we can work this issue around by avoiding the multiarray >> allocations in AESCrypt.makeSessionKey. This workaround is straight-forward >> and backportable. >> >> Example profile is in the bug. >> >> On new benchmark and M1 Mac: >> >> >> Benchmark Mode CntScore Error Units >> >> # Before >> AESReinit.test avgt 15 873,842 ± 6,911 ns/op >> >> # After >> AESReinit.test avgt 15 533,018 ± 4,048 ns/op >> >> >> Additional testing: >> - [x] Benchmarks >> - [x] macos-aarch64-server-release, `jdk_security` >> - [ ] linux-x86_64-server-fastdebug, `tier1 tier2` >> - [x] linux-aarch64-server-fastdebug, `tier1 tier2` > > src/java.base/share/classes/com/sun/crypto/provider/AESCrypt.java line 1372: > >> 1370: >> 1371: // It is significantly faster to allocate individual arrays, >> 1372: // instead of doing the multi-array allocation. See >> JDK-8308105. > > Alternatively, could the multi-array allocation get improved? Yes, it could and it eventually would, but it would take significantly more work: there are multiple issues of differing complexity, impact, and risk. This workaround, on the other hand, is straight-forward and backportable. - PR Review Comment: https://git.openjdk.org/jdk/pull/13996#discussion_r1194673153
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey
On Mon, 15 May 2023 19:59:13 GMT, Aleksey Shipilev wrote: > One of our services has a hot path with AES/GCM cipher reuse. The JDK code > reinitializes the session key on that path, and > [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up > prominently there. While > [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) is being fixed, > which would take a while, and would likely require multiple patches, we can > work this issue around by avoiding the multiarray allocations in > AESCrypt.makeSessionKey. > > Example profile is in the bug. > > On new benchmark and M1 Mac: > > > Benchmark Mode CntScore Error Units > > # Before > AESReinit.test avgt 15 873,842 ± 6,911 ns/op > > # After > AESReinit.test avgt 15 533,018 ± 4,048 ns/op > > > Additional testing: > - [x] Benchmarks > - [x] macos-aarch64-server-release, `jdk_security` > - [ ] linux-x86_64-server-fastdebug, `tier1 tier2` > - [ ] linux-aarch64-server-fastdebug, `tier1 tier2` src/java.base/share/classes/com/sun/crypto/provider/AESCrypt.java line 1369: > 1367: int BC = 4; > 1368: int[][] Ke = new int[ROUNDS + 1][]; // encryption round keys > 1369: int[][] Kd = new int[ROUNDS + 1][]; // decryption round keys This is more a question of curiosity and likely beyond the scope of this PR/bug ID -- would it be beneficial to refactor this to allocate and operate over just two arrays, one for Ke and one for Kd, effectively removing the need for allocation in `expandToSubKey` as well? int[] Ke = new int[4 * (ROUNDS + 1)]; int[] Kd = new int[4 * (ROUNDS + 1)]; - PR Review Comment: https://git.openjdk.org/jdk/pull/13996#discussion_r1194511682
Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey
On Mon, 15 May 2023 19:59:13 GMT, Aleksey Shipilev wrote: > One of our services has a hot path with AES/GCM cipher reuse. The JDK code > reinitializes the session key on that path, and > [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up > prominently there. While > [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) is being fixed, > which would take a while, and would likely require multiple patches, we can > work this issue around by avoiding the multiarray allocations in > AESCrypt.makeSessionKey. > > Example profile is in the bug. > > On new benchmark and M1 Mac: > > > Benchmark Mode CntScore Error Units > > # Before > AESReinit.test avgt 15 873,842 ± 6,911 ns/op > > # After > AESReinit.test avgt 15 533,018 ± 4,048 ns/op > > > Additional testing: > - [x] Benchmarks > - [x] macos-aarch64-server-release, `jdk_security` > - [ ] linux-x86_64-server-fastdebug, `tier1 tier2` > - [ ] linux-aarch64-server-fastdebug, `tier1 tier2` src/java.base/share/classes/com/sun/crypto/provider/AESCrypt.java line 1372: > 1370: > 1371: // It is significantly faster to allocate individual arrays, > 1372: // instead of doing the multi-array allocation. See JDK-8308105. Alternatively, could the multi-array allocation get improved? - PR Review Comment: https://git.openjdk.org/jdk/pull/13996#discussion_r1194351546
