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
Seed PRNGs from System source instead of time #4789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. A long awaited improvement.
src/random/pcg32.cr
Outdated
@@ -37,14 +37,22 @@ class Random::PCG32 | |||
@state : UInt64 | |||
@inc : UInt64 | |||
|
|||
def initialize(initstate = UInt64.new(Random.new_seed), initseq = 0_u64) | |||
def initialize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use a self.new
constructor, as we usually do in the stdlib.
src/random/pcg32.cr
Outdated
# initialize to zeros to prevent compiler complains | ||
@state = 0_u64 | ||
@inc = 0_u64 | ||
new_seed(initstate, initseq) | ||
end | ||
|
||
def new_seed(initstate = UInt64.new(Random.new_seed), initseq = 0_u64) | ||
def new_seed | ||
initialize(Random::System.rand(UInt64::MIN..UInt64::MAX), Random::System.rand(UInt64::MIN..UInt64::MAX)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a method call initialize
feels weird, yet that works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, should be new_seed(...)
Fixes #4596.
Now PCG32 has
new
without parameters (that creates seed AND sequence from system source giving 128 bits of enthropy) andnew
that was here before, that receives 64-bits seed and sequence that is 0 by default.For an ISAAC I just replaced default value in constructor. Perhaps complex initialization isn't needed because seed values are already "random" enough (so they can be directly copied to state), but it won't hurt and copying won't improve the performance much (as slowest thing is getting random from system source).
Random.new_seed
is removed, because PCG32 needs 64-bit int, ISAAC needs a buffer, so I think it is cleaner to generate them in corresponding modules.About performance - as was mentioned in a #4596 this change slows down creating of PRNGs a lot (up to x50 times). But i think improved safety pays for it because creation of PRNG doesn't seem time-critical operation. Creating from given seed isn't affected, only from "unpredictable" seed.