Seed PRNGs from System source instead of time #4789
Merged
+28
−15
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ysbaddaden
approved these changes
Aug 4, 2017
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.
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.
Having a method call initialize feels weird, yet that works.
There was a problem hiding this comment.
oops, should be new_seed(...)
RX14
approved these changes
Aug 14, 2017
This was referenced Aug 24, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4596.
Now PCG32 has
newwithout parameters (that creates seed AND sequence from system source giving 128 bits of enthropy) andnewthat 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_seedis 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.