On Wed, 24 May 2023 19:18:33 GMT, Aleksey Shipilev <[email protected]> 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
