On Wed, 24 May 2023 19:18:33 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:

> UUID is very important class that is used to track identities of objects in 
> large scale systems. Yet, the coverage in JDK test is disappointing: it tests 
> only 100 of UUID instances per test, which is way too small to detect 
> collisions due to the bad randomness for example.
> 
> I have some pending work to improve UUID performance, and tests should be 
> improved. 
> 
> The improved test still runs in less than 5 seconds on my laptop.

Overall, this test-only change look OK to me. It updates the test to use a 
`Set` instead of `List` for "contains" checks, which detect random generated 
UUID collisions. The other part of this change is increasing the UUID 
generation count/repeatition and a new test method which generates the UUIDs in 
parallel and tries to detect any unexpected collisions.

I just have a couple of minor suggestions, which I have noted inline.

test/jdk/java/util/UUID/UUIDTest.java line 40:

> 38:     private static final int COUNT = 1_000_000;
> 39: 
> 40:     static final Random generator = new Random();

Hello Aleksey, I realize this is unrelated to the changes in this PR, but since 
we are updating this test, would using `jdk.test.lib.RandomFactory.getRandom()` 
from test library be a better idea here? That would then print the seed in the 
logs and could help debug any collisions more easily?

test/jdk/java/util/UUID/UUIDTest.java line 316:

> 314:             UUID u2 = UUID.fromString(u1.toString());
> 315:             if (u1.hashCode() != u2.hashCode()) {
> 316:                 throw new Exception("Equal UUIDs with different hash 
> codes: " + u1 + " and " + u2);

Perhaps we should even print the hash codes that weren't matching to provide 
assistance when debugging?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/14134#issuecomment-1566645445
PR Review Comment: https://git.openjdk.org/jdk/pull/14134#discussion_r1208977029
PR Review Comment: https://git.openjdk.org/jdk/pull/14134#discussion_r1208978832

Reply via email to