Skip to content

Migrate default entropy to rand/v2 #124

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

webstradev
Copy link

@webstradev webstradev commented Mar 2, 2025

As discussed a bit in #123 it is looking like it is worth migrating to rand/v2 and advising people to use this in the README as well.

@peterbourgon and myself had both run some benchmarks and it does seem it is ever so slightly faster in most cases and somewhat slower (±6ns) in one of the benchmarks.

This PR changes the examples, tests and benchmarks to use math/rand/v2 instead of math.rand.
I use a chacha8 source which is the only one in there that implements an io.Reader.

The difference with randV1.NewSource is that it requires a [32]byte seed instead of a uint64. For this I currently just put the time in nanoseconds as the first 8 bytes and leave the rest empty.

I've also updated the README examples suggesting the usage of math/rand/v2 instead of math/rand.

Below are some benchmark results on my machine, I deliberately did not yet update these in the README.md as I'm not sure if you wanted to keep that as the same machine you had them on previously.

benchmark.txt

Let me know if you have any questions or require any further changes.

Signed-off-by: Erik Westra <e.s.westra.95@gmail.com>
Signed-off-by: Erik Westra <e.s.westra.95@gmail.com>
@cyberxnomad
Copy link

ulid/ulid.go

Line 585 in 96c4edf

type rng interface{ Int63n(n int64) int64 }

ulid/ulid.go

Lines 578 to 580 in 96c4edf

if rng, ok := entropy.(rng); ok {
m.rng = rng
}

ulid/ulid.go

Lines 644 to 647 in 96c4edf

if m.rng != nil {
// Range: [1, m.inc)
return 1 + uint64(m.rng.Int63n(int64(m.inc))), nil
}

Do these codes need to be changed together? Can they be replaced with rand.Int64N()

@webstradev
Copy link
Author

ulid/ulid.go

Line 585 in 96c4edf

type rng interface{ Int63n(n int64) int64 }

ulid/ulid.go

Lines 578 to 580 in 96c4edf

if rng, ok := entropy.(rng); ok {
m.rng = rng
}

ulid/ulid.go

Lines 644 to 647 in 96c4edf

if m.rng != nil {
// Range: [1, m.inc)
return 1 + uint64(m.rng.Int63n(int64(m.inc))), nil
}

Do these codes need to be changed together? Can they be replaced with rand.Int64N()

good catch, I'll have a look at whether that's possible.

Also noticed were still using mathrand in the cli so i'll fix that as well

Signed-off-by: Erik Westra <e.s.westra.95@gmail.com>
@webstradev
Copy link
Author

ulid/ulid.go

Line 585 in 96c4edf

type rng interface{ Int63n(n int64) int64 }

ulid/ulid.go

Lines 578 to 580 in 96c4edf

if rng, ok := entropy.(rng); ok {
m.rng = rng
}

ulid/ulid.go

Lines 644 to 647 in 96c4edf

if m.rng != nil {
// Range: [1, m.inc)
return 1 + uint64(m.rng.Int63n(int64(m.inc))), nil
}

Do these codes need to be changed together? Can they be replaced with rand.Int64N()

good catch, I'll have a look at whether that's possible.

Also noticed were still using mathrand in the cli so i'll fix that as well

@peterbourgon Question for you.
@cyberxnomad has pointed out a small issue here, which is that the Monotonic Reader checks if the entropy is castable to the rng interface (Int63n) and if so it will use that.
That check will never evalate to true when using a v2 rand source because in math v2 Int63n has become Int64N. If I change the interface this will break backwards compatability with math/v1 which isn't ideal.
and more specifically the only source in math/v2 that implements io.Reader doesn't have Int64N as a method (though it does have Uint64.

Not entirely sure how to proceed, I could add a second interface for Uint64 and check for that as well, but it seems a little hacky

We could also ignore it meaning we don't use the fast path, but not sure if there are any consequences to that. It would be similar behaviour as to when a cryptorand.Reader is passed as that doens't implement the rng interface eather.

Let me know what you think!

@webstradev
Copy link
Author

ulid/ulid.go

Line 585 in 96c4edf

type rng interface{ Int63n(n int64) int64 }

ulid/ulid.go

Lines 578 to 580 in 96c4edf

if rng, ok := entropy.(rng); ok {
m.rng = rng
}

ulid/ulid.go

Lines 644 to 647 in 96c4edf

if m.rng != nil {
// Range: [1, m.inc)
return 1 + uint64(m.rng.Int63n(int64(m.inc))), nil
}

Do these codes need to be changed together? Can they be replaced with rand.Int64N()

good catch, I'll have a look at whether that's possible.
Also noticed were still using mathrand in the cli so i'll fix that as well

@peterbourgon Question for you. @cyberxnomad has pointed out a small issue here, which is that the Monotonic Reader checks if the entropy is castable to the rng interface (Int63n) and if so it will use that. That check will never evalate to true when using a v2 rand source because in math v2 Int63n has become Int64N. If I change the interface this will break backwards compatability with math/v1 which isn't ideal. and more specifically the only source in math/v2 that implements io.Reader doesn't have Int64N as a method (though it does have Uint64.

Not entirely sure how to proceed, I could add a second interface for Uint64 and check for that as well, but it seems a little hacky

We could also ignore it meaning we don't use the fast path, but not sure if there are any consequences to that. It would be similar behaviour as to when a cryptorand.Reader is passed as that doens't implement the rng interface eather.

Let me know what you think!

Just to add to the above, potentially this is why the monotonic reads, were slightly slower using randv2 vs v1 and all the others werent, because it is skipping the fast path, not sure if there is much of a performance gain with that fast path....

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