-
-
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
dSFMT: use fill_array_* API instead of genrand_* API #8808
Conversation
Both APIs are not very compatible, so let's use fill_array_* functions, which are often faster but require MersenneTwister to have an additional array field.
It looks like you are still mixing the two calls in order to get a general purpose implementation, on the same object. |
I hadn't understood what you were saying correctly. Assuming this works, this could get us back to the earlier performance level without the bug. |
Could you do the same for the global RNG too? And also add a test that ensures that #6573 doesn't happen again in both cases? |
We should make sure that |
Yes I think it should give back exactly the same performance as before. The difference is that for small arrays, instead of falling back to I will try to add relevant test to prevent #6573, but if RE global RNG: in #8380 I tried to minimize the portion of the code which discriminates between global RNG or local |
Please extend the current PR. |
The part of the #8399 PR where the global state is kept in Julia could be done as part of this PR too, if it makes it easier. In any case, let's get the various dSFMT related PRs merged. |
Yes it makes it easier, as using the global state array from libdSFMT together with a global array in Julia needed to extend this PR to work with the global RNG would seem artificial and be more complicated than just having the global RNG be an instance of |
+100000 to making the global RNG be an instance of MersenneTwister. That makes designing the whole RNG API much cleaner and simpler. Not having access to the global RNG as a first class value is a bit weird too. It's just "out there" somewhere. |
Superseded by #8832. |
Mixing both APIs led to bug #6573. As a result
fill_array_*
functions were no more used. This PR proposes instead to use them exclusively withMersenneTwister
objects (I could do the same for the global RNG if there is interest), for faster random generation (up to 2x or 3x on my machine).Looking at the dSFMT code, my understanding is that both take advantage of simd instructions, but use different algorithms:
fill_array_*
generates random values (considering onlyFloat64
values for now) into an array of size (at least)dsfmt_min_array_size
, using a state array of sizedsfmt_min_array_size
,genrand_*
cyclically overwrites a state array of sizedsfmt_min_array_size
filled with random values with new random values.All it takes to use
fill_array_*
functions is to add aFloat64
array of sizedsfmt_min_array_size
toMersenneTwister
instances (which then allocate roughly twice as much), to be cyclically filled with random values (viafill_array_*
), which allowsrand(r::MersenneTwister)
to pick those values one by one.I guess the same speed differences could be observed for integer generation, but currently
MersenneTwister
RNGs generate only floats (I would be happy to enable this if #8380 or equivalent got merged, which the use ofdsfmt_fill_array_close1_open2
anticipates here).