Skip to content

Make Uuids have better random distribution #494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

dkropachev
Copy link
Collaborator

@dkropachev dkropachev commented Apr 4, 2025

Before this commit Uuids used to initiate Random at point when it needs a random number.
Not only it is not efficient it also produced bad distribution which resulted in should_generate_unique_random_uuids_Random to fail, due to inability of Uuids::random generate unique vales.

I have tested it with:

  @Test
  public void should_generate_unique_random_uuids_Random() {
    for (int i = 0; i < 10000; i++) {
      System.out.println(i);
      Set<UUID> generated = serialGeneration(1_000_000, Uuids::random);
      assertThat(generated).hasSize(1_000_000);
    }
  }

Which was failing after 100 iterations before fix and never failed after.

@dkropachev dkropachev force-pushed the dk/4.x-change-uuuid-random branch from 6d21590 to 9891e31 Compare April 4, 2025 14:46
@dkropachev dkropachev requested a review from Bouncheck April 4, 2025 14:47
@dkropachev dkropachev self-assigned this Apr 4, 2025
@dkropachev dkropachev changed the title Make Uuids use default random Make Uuids have better random distribution Apr 4, 2025
@dkropachev dkropachev changed the title Make Uuids have better random distribution Make Uuids have better random distribution Apr 4, 2025
Copy link
Collaborator

@Bouncheck Bouncheck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instances of SplittableRandom are not thread-safe. They are designed to be split, not shared, across threads.

Docs do not reflect changes.

I think current master is good enough. I've seen this test fail only once so far in all those years.

Before this commit `Uuids` used to initiate `Random `at the point when it needs
a random number.
Not only it is not effecient it also produced bad distribution which
resulted in `should_generate_unique_random_uuids_Random` to fail, due to
inability of `Uuids::random `generate unique vales.
Before this commit it used `random.nextBytes` which made it generate an `int`
and crop it to `byte`, since it is quasi random, it produces bad
distribution and lead to have more UUID collisions.
@dkropachev dkropachev force-pushed the dk/4.x-change-uuuid-random branch from 7a41ee5 to 7bc5cfc Compare April 4, 2025 17:52
@dkropachev
Copy link
Collaborator Author

Instances of SplittableRandom are not thread-safe. They are designed to be split, not shared, across threads.

Docs do not reflect changes.

I think current master is good enough. I've seen this test fail only once so far in all those years.

Thanks for pointing that out, I have moved it to use TrheadLocalRandom, please take a look

@dkropachev dkropachev requested a review from Bouncheck April 4, 2025 17:53
@dkropachev dkropachev merged commit 263f09d into scylladb:scylla-4.x Apr 4, 2025
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants