-
Notifications
You must be signed in to change notification settings - Fork 173
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Erik Westra <e.s.westra.95@gmail.com>
Signed-off-by: Erik Westra <e.s.westra.95@gmail.com>
Line 585 in 96c4edf
Lines 578 to 580 in 96c4edf
Lines 644 to 647 in 96c4edf
Do these codes need to be changed together? Can they be replaced with |
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>
@peterbourgon Question for you. 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.... |
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.