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

MersenneTwister: more efficient Float64 scalar generation with caching #25197

Merged
merged 1 commit into from
Dec 24, 2017

Conversation

rfourquet
Copy link
Member

The is branched off and inspired from #25058. There seem to be a sweet spot at randomizing arrays of size 8016 bytes for dSFMT: beyond this threshold, the generation rate increases, and for Float64, this shows a speed-up of about 30% (compared to the current Float64 cache size of 3056 bytes, the minimum possible for dSFMT). This again would change the generated streams, so is breaking. Let's see if Nanosoldier agrees this time:
@nanosoldier runbenchmarks("random", vs=":master")

@rfourquet rfourquet added performance Must go faster randomness Random number generation and the Random stdlib labels Dec 19, 2017
@ararslan
Copy link
Member

@nanosoldier runbenchmarks("random", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@rfourquet
Copy link
Member Author

Nanosoldier still not seeing improvements... let see with the "problem" benchmarks:

@nanosoldier runbenchmarks("problem", vs=":master")

@rfourquet
Copy link
Member Author

Oups, forgot to quote: @nanosoldier runbenchmarks("problem", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@rfourquet
Copy link
Member Author

The regressions in Nanosoldier's report are expected: those two problems use the samerand function from "BaseBenchmarks.jl/utils", which for some reason deepcopies a MersenneTwister object at each invocation. which dominates totally the benchmarks. In this PR, it's more expensive to copy an RNG because we make it bigger, so no mystery here. The mystery is why the benchmarks needs to deepcopy an RNG... I think this is wrong, as it represents 99% percent of the time spent in the benchmark.

@ViralBShah
Copy link
Member

I don't think we guarantee the same stream across releases - so it should be perfectly ok to do this if it gives better performance.

@rfourquet
Copy link
Member Author

The performance improvements are confirmed on at least 4 different computers, so I will merge tomorrow unless there are objections.

Like for integers, a cache of size 8016 bytes seems to be optimal.
@StefanKarpinski
Copy link
Member

I don't think we guarantee the same stream across releases

We should not change the behavior of the same RNG type across releases, but we are allowed to change default RNGs with minor releases. If we change the stream for MersenneTwister in the future, it would require a major version bump of the package that provides it.

@rfourquet
Copy link
Member Author

We should not change the behavior of the same RNG type across releases,

Do you mean minor releases? (as opposed to "major version bump")

@rfourquet rfourquet merged commit 99b8dc3 into master Dec 24, 2017
@fredrikekre fredrikekre deleted the rf/rand/cache-float branch December 24, 2017 11:25
@StefanKarpinski
Copy link
Member

I guess it depends and it's a choice we have to make. We need to make sure that code can request a specific behavior of an RNG over time, which means that RNGs must be in packages and that one can continue to ask for MersenneTwister v"1.*" no matter what version of Julia one is using. Perhaps applying SemVer to RNG package behavior is also sensible. In that case, we can make breaking changes to the behavior of an RNG, but we can only do so if we bump the major version.

@bjarthur
Copy link
Contributor

w.r.t. the comment above about SemVer of RNG, is it possible to recreate the julia 0.6 sequence of random numbers in 0.7/1.0? thanks.

@rfourquet
Copy link
Member Author

There is the (registered, I think) RandomV06 package for that.

@bjarthur
Copy link
Contributor

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants