Skip to content
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

Merged
merged 1 commit into from Aug 14, 2017
Merged

Conversation

konovod
Copy link
Contributor

@konovod konovod commented Aug 4, 2017

Fixes #4596.
Now PCG32 has new without parameters (that creates seed AND sequence from system source giving 128 bits of enthropy) and new 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.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Looks good to me. A long awaited improvement.

@@ -37,14 +37,22 @@ class Random::PCG32
@state : UInt64
@inc : UInt64

def initialize(initstate = UInt64.new(Random.new_seed), initseq = 0_u64)
def initialize
Copy link
Contributor

@ysbaddaden ysbaddaden Aug 4, 2017

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.

# 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))
Copy link
Contributor

@ysbaddaden ysbaddaden Aug 4, 2017

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.

Copy link
Contributor Author

@konovod konovod Aug 4, 2017

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(...)

RX14
RX14 approved these changes Aug 14, 2017
@RX14 RX14 merged commit d638238 into crystal-lang:master Aug 14, 2017
2 checks passed
@RX14 RX14 added this to the Next milestone Aug 14, 2017
@konovod konovod deleted the rng_seed branch Aug 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants