Skip to content

Fix Issue 18595 & Issue 18596: replace MindstdRand0 w/SplitMix & templatize unpredictableSeed!UIntType#6580

Merged
dlang-bot merged 1 commit intodlang:masterfrom
n8sh:unpredictableSeed-18595-18596
Nov 14, 2018
Merged

Fix Issue 18595 & Issue 18596: replace MindstdRand0 w/SplitMix & templatize unpredictableSeed!UIntType#6580
dlang-bot merged 1 commit intodlang:masterfrom
n8sh:unpredictableSeed-18595-18596

Conversation

@n8sh
Copy link
Member

@n8sh n8sh commented Jun 12, 2018

Slicing the elephant into smaller pieces. This PR differs from #6388 in that it omits entirely system entropy.

The PR's visible change is that one may now write unpredictableSeed!ulong and get a ulong. The PR's under-the-hood change is that instead of thread-local MinstdRand0 unpredictableSeed uses a global SplitMix.

Q: Why prefer global shared (atomic) instance to thread-locals?
A: It gives us an easy guarantee that the same ulong seed will not be produced by more than 1 thread except if unpredictableSeed!ulong is called 2^^64 times during the life of the application. The question should rather be, what advantage would thread-local storage give us here?

Q: What about cryptography?
A: This PR changes nothing about the current situation. unpredictableSeed is not in any sense unpredictable to an adversary willing to use mathematics.

Q: What about speed?
A: This shouldn't be a main consideration, but this implementation happens to be faster. The new unpredictableSeed takes about 1/3 the time with ldc2 -O and about 1/2 the time with dmd -O -inline. The story is even better if you compare the new unpredictableSeed!ulong to calling the old unpredictableSeed twice to produce a 64-bit value.

@n8sh n8sh requested a review from wilzbach as a code owner June 12, 2018 22:09
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
18595 enhancement std.random: add unpredictableSeedOf!UIntType for non-uint unpredictableSeed

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6580"

@n8sh n8sh force-pushed the unpredictableSeed-18595-18596 branch 4 times, most recently from 93e75f5 to 2ba81a4 Compare June 12, 2018 22:47
@n8sh n8sh force-pushed the unpredictableSeed-18595-18596 branch from 2ba81a4 to ccbbca5 Compare August 19, 2018 09:24
@n8sh n8sh force-pushed the unpredictableSeed-18595-18596 branch from ccbbca5 to d6e2c1f Compare October 16, 2018 02:48
@n8sh n8sh force-pushed the unpredictableSeed-18595-18596 branch 2 times, most recently from 7f6358b to 038b0d8 Compare October 25, 2018 16:13
@thewilsonator
Copy link
Contributor

@n8sh does this need a changelog entry?

@n8sh
Copy link
Member Author

n8sh commented Nov 14, 2018

@thewilsonator I'll add one.

@thewilsonator
Copy link
Contributor

Thanks!

@n8sh n8sh force-pushed the unpredictableSeed-18595-18596 branch from 038b0d8 to c75d6f7 Compare November 14, 2018 10:41
@n8sh
Copy link
Member Author

n8sh commented Nov 14, 2018

Added.

updateResult(cast(ulong) getpid());
updateResult(cast(ulong) MonoTime.currTime.ticks);
result = (result ^ (result >>> 47)) * m;
return result ^ (result >>> 47);
Copy link
Contributor

@kinke kinke Jan 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the LLVM optimizer, this whole function is optimized to calling Thread.getThis(), getpid() and MonoTime.currTime().ticks() (for potential side effects) and then returning an undefined value (i.e., no updateResult() calls, no final mixing) - due to the final value of result depending on its initial value, which is explicitly undefined (= void).

The intent probably was to make the bootstrap seed somewhat more random, but this doesn't work here. Would it be okay to initialize result with 0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting Johan:

Using an uninitialized value is UB in D. https://dlang.org/spec/declaration.html#void_init

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be okay to initialize result with 0?

Yes.

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.

4 participants