-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Random: always use SamplerRangeFast for MersenneTwister #27560
Conversation
cc. @mschauer |
We can break some things between |
I am still fascinated that a single division is so expensive compared to our MersenneTwister entropy generation. Anyway I think this is a sane change. |
Thanks for your answers! So I will mark this for triage.
Me too, indeed! SIMD is certainly crucial for that... If someone likes to test the performance on her machine, it should be rather straigthforward, with this using Random
for T in Base.BitInteger_types
@eval Random Sampler(rng::MersenneTwister, r::UnitRange{$T}, ::Val{Inf}) = SamplerRangeFast(r)
end |
@rfourquet: triage agrees that you're likely the only person in a position to really make a call on this. It's fine to make a breaking change now if you deem it worthwhile. Merge if you see fit. |
Thanks for having discussed this. I did few more benchmark to try to get an idea what would be the expected speed-up for a range of "random length" (not so trivial to measure that in a meaningful way; I did what I could). Benchmarking only the step 2) mentioned in the OP (which is equivalent to benchmarking If no objection comes, I will therefore merge this week-end. |
The improvement even on asymptotically large arrays settles it. |
547b257
to
0c698be
Compare
The Travis failure is irrelated to this change and happens in other PRs too (concerns |
This is a "breaking" change concerning the numbers generated in a call like
rand(1:3, 10)
. It may be too late for 0.7, but on the other hand the resistance to change may be too high in a later release for such non-essential efficiency improvements.To generate a random value in
1:3
, there are 2 distinct steps:Sampler
object, which involves one-time computationsAs the number of generated random values increases, the cost of step 1) becomes negligible (amortization).
We currently have two
Sampler
types with different compromises on the costs of both steps:SamplerRangeInt
(SRI
) which is more costly at 1), but is more efficient at using as few entropy bits as possible, in step 2)SamplerRangeFast
(SRF
) which is cheap at 1), but wastes more entropy bits (depending on the length of the range)By default, an RNG will use
SRI
.MersenneTwister
uses:SRF
for scalar calls, likerand(1:3)
:MersenneTwister
is fast enough at generating entropy that wasting some bits is preferable in this caseSRI
for array calls, likerand(1:3, 10)
: this was the the original method, and was not updated whenSRF
was introduced, as the status-quo was/is faster in some cases.I propose now to use
SRF
in all cases forMersenneTwister
, for more uniformity (e.g.srand(0); [rand(1:10), rand(1:10)]
will give the same result assrand(0); rand(1:10, 2)
), and for efficiency, as "most (e.g. 90%) of the time" this will give improved speed.For a given length of array, the speed of the
SRI
method doesn't vary much with the lengthL
of the range, unlike withSRF
:L<=2^n
withL
close to2^n
,SRF
can be between 2 and 3 times as fast asSRI
L = k + 2^n
withk>0
"small",SRF
is slower thanSRI
by a small margin, e.g. 10%. Ask
grows,SRF
gets faster, and becomes again faster thanSRI
e.g. whenk ≈ (2^n)/10
(which means thatSRF
is slightly slower thanSRI
for 10 percent of input ranges of length between2^n+1
and2^(n+1)
).I lack time now to do advanced performance analysis and graphics, but here is a representative benchmark session (assume the range is
$
-escaped):