Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v7]
> UUID is the very important class that is used to track identities of objects
> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
> of total CPU time, and is frequently a scalability bottleneck due to
> `SecureRandom` synchronization.
>
> The major issue with UUID code itself is that it reads from the single
> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
> is bashed with very small requests. This also has a chilling effect on other
> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>
> We can improve this by doing the bulk reads from the backing SecureRandom and
> possibly striping the reads across many instances of it.
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>
> # After
> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive scaling,
> ~1.5x
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative scaling
>
> # After
> BenchmarkMode Cnt Score Error Units
> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
> scaling, ~1.2x
>
>
> Note that there is still a scalability bottleneck in current default random
> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
> out the synchronization story there. The scalability fix in current default
> `SecureRandom` would be much more intrusive and risky, since it would change
> a core crypto class with unknown bug fanout.
>
> Using the bulk reads even when the underlying PRNG is heavily synchronized is
> still a win. A more scalable PRNG would benefit from this as well. This PR
> adds a system property to select the PRNG implementation, and there we can
> clearly see the benefit with more scalable PRNG sources:
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before, hacked `new SecureRandom()` to
> `SecureRandom.getInstance("SHA1PRNG")`
> UUIDRandomBench.single thrpt 15 3.661 ± 0.008 ops/us
> UUIDRandomBench...
Aleksey Shipilev has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 13 commits:
- Revert test changes
- Merge branch 'master' into JDK-8308804-uuid-buffers
- Simplify clinit
- Remove the properties
- Swap lsb/msb
- Fine-tune exceptions
- Handle privileged properties
- Use ByteArray to convert. Do version/variant preparations straight on
locals. Move init out of optimistic lock section.
- More touchups
- Comment updates
- ... and 3 more: https://git.openjdk.org/jdk/compare/4460429d...fd7eaa1a
-
Changes: https://git.openjdk.org/jdk/pull/14135/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14135&range=06
Stats: 197 lines in 2 files changed: 186 ins; 8 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/14135.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14135/head:pull/14135
PR: https://git.openjdk.org/jdk/pull/14135
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v7]
On Thu, 1 Jun 2023 09:37:29 GMT, Aleksey Shipilev wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
>> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
>> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> Aleksey Shipilev has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 13 commits:
>
> - Revert test changes
> - Merge branch 'master' into JDK-8308804-uuid-buffers
> - Simplify clinit
> - Remove the properties
> - Swap lsb/msb
> - Fine-tune exceptions
> - Handle privileged properties
> - Use ByteArray to convert. Do version/variant preparations straight on
> locals. Move init out of optimistic lock section.
> - More touchups
> - Comment updates
> - ... and 3 more: https://git.openjdk.org/jdk/compare/4460429d...fd7eaa1a
src/java.base/share/classes/java/util/UUID.java line 234:
> 232: long msb = ByteArray.getLong(buf, 0);
> 233: long lsb = ByteArray.getLong(buf, 8);
> 234: return fromRandom(msb, lsb);
is there any value in moving this outside of the critical section?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1213117598
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v7]
On Thu, 1 Jun 2023 12:59:14 GMT, Brett Okken wrote: >> Aleksey Shipilev has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains 13 commits: >> >> - Revert test changes >> - Merge branch 'master' into JDK-8308804-uuid-buffers >> - Simplify clinit >> - Remove the properties >> - Swap lsb/msb >> - Fine-tune exceptions >> - Handle privileged properties >> - Use ByteArray to convert. Do version/variant preparations straight on >> locals. Move init out of optimistic lock section. >> - More touchups >> - Comment updates >> - ... and 3 more: https://git.openjdk.org/jdk/compare/4460429d...fd7eaa1a > > src/java.base/share/classes/java/util/UUID.java line 234: > >> 232: long msb = ByteArray.getLong(buf, 0); >> 233: long lsb = ByteArray.getLong(buf, 8); >> 234: return fromRandom(msb, lsb); > > is there any value in moving this outside of the critical section? I have tried it before, and it gives no benefit, but significantly complicates the code. This is the part under write lock, where we have spent a significant time refilling the buffer. The little improvement we might get here is drowning in those costs. - PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1213121408
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v7]
On Thu, 1 Jun 2023 09:37:29 GMT, Aleksey Shipilev wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
>> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
>> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> Aleksey Shipilev has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 13 commits:
>
> - Revert test changes
> - Merge branch 'master' into JDK-8308804-uuid-buffers
> - Simplify clinit
> - Remove the properties
> - Swap lsb/msb
> - Fine-tune exceptions
> - Handle privileged properties
> - Use ByteArray to convert. Do version/variant preparations straight on
> locals. Move init out of optimistic lock section.
> - More touchups
> - Comment updates
> - ... and 3 more: https://git.openjdk.org/jdk/compare/4460429d...fd7eaa1a
src/java.base/share/classes/java/util/UUID.java line 181:
> 179:
> 180: private static UUID fromRandom(long msb, long lsb) {
> 181: // set version to 3
The comment doesn't match the code. Is it version 4?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1213472422
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v7]
On Thu, 1 Jun 2023 09:37:29 GMT, Aleksey Shipilev wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
>> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
>> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> Aleksey Shipilev has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 13 commits:
>
> - Revert test changes
> - Merge branch 'master' into JDK-8308804-uuid-buffers
> - Simplify clinit
> - Remove the properties
> - Swap lsb/msb
> - Fine-tune exceptions
> - Handle privileged properties
> - Use ByteArray to convert. Do version/variant preparations straight on
> locals. Move init out of optimistic lock section.
> - More touchups
> - Comment updates
> - ... and 3 more: https://git.openjdk.org/jdk/compare/4460429d...fd7eaa1a
I see this PR was closed out as stale and not integrated. I am curious if there
is anything I could do to help push this over the line. As highlighted by
@shipilev in the PR description, UUID generation is often a critical component
of some systems and contention on SecureRandom can become a scalability
bottleneck that I would love to reduce.
Thanks!
-
PR Comment: https://git.openjdk.org/jdk/pull/14135#issuecomment-1705192263
