Skip to content

change uuid to use Random.RandomDevice() instead of Random.default_rng() #35872

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
merged 11 commits into from
May 27, 2020
6 changes: 4 additions & 2 deletions stdlib/UUIDs/src/UUIDs.jl
Original file line number Diff line number Diff line change
@@ -35,6 +35,8 @@ const namespace_url = UUID(0x6ba7b8119dad11d180b400c04fd430c8) # 6ba7b811-9dad-
const namespace_oid = UUID(0x6ba7b8129dad11d180b400c04fd430c8) # 6ba7b812-9dad-11d1-80b4-00c04fd430c8
const namespace_x500 = UUID(0x6ba7b8149dad11d180b400c04fd430c8) # 6ba7b814-9dad-11d1-80b4-00c04fd430c8

uuid_rng = Random.RandomDevice()

"""
uuid1([rng::AbstractRNG=Random.RandomDevice()]) -> UUID

@@ -53,7 +55,7 @@ UUID("cfc395e8-590f-11e8-1f13-43a2532b2fa8")
!!! compat "Julia 1.6"
The default rng has been changed to use Random.RandomDevice as of Julia 1.6
"""
function uuid1(rng::AbstractRNG=Random.RandomDevice())
function uuid1(rng::AbstractRNG=uuid_rng)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you make this change? Creating a new RandomDevice() is extremely cheap on non-Windows, and on Windows having a global uuid_rng is not thread-safe...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rfourquet I guess misunderstood @JeffBezanson 's comment above?

How about using a dedicated default RNG for UUIDs that's initialized from RandomDevice at startup?

Copy link
Member

Choose a reason for hiding this comment

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

My interpretation of it was using an RNG like default_rng() (currently MersenneTwister() which is thread-safely initialized), which is therefore immune against the unqualified Random.seed!() but way faster than RandomDevice(), but "initialized from RandomDevice()" means it's seeded from RandomDevice() at startup.

Copy link
Member

Choose a reason for hiding this comment

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

But Stefan's last comment suggests to keep it how it was, because the poor performance of RandomDevice might considered as a non-problem until someone complains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! I reverted 507b454 . I'll leave it alone for now until there's something I should explicitly add/change/remove.

u = rand(rng, UInt128)

# mask off clock sequence and node
@@ -92,7 +94,7 @@ UUID("196f2941-2d58-45ba-9f13-43a2532b2fa8")
!!! compat "Julia 1.6"
The default rng has been changed to use Random.RandomDevice as of Julia 1.6
"""
function uuid4(rng::AbstractRNG=Random.RandomDevice())
function uuid4(rng::AbstractRNG=uuid_rng)
u = rand(rng, UInt128)
u &= 0xffffffffffff0fff3fffffffffffffff
u |= 0x00000000000040008000000000000000