-
-
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 #8832
Conversation
62e8a9c
to
0a88117
Compare
I would say do it in two parts, so yes, keep this change minimal. |
Both APIs are not very compatible, so let's use fill_array_* functions, which are faster but require MersenneTwister to have an additional array field.
0a88117
to
d005415
Compare
Rebased to make the global RNG |
The state of the global RNG was previously handled by libdSFMT. This commit allows the global RNG to benefit speed improvements resulting from the use of fill_array_* functions (cf. previous commit). However, scalar calls to rand() (i.e. producing one value instead of filling an array) using the global RNG are now slower, as well as those filling non-Float64 arrays (less than twice as slow though).
d005415
to
3f6bd03
Compare
dSFMT: use fill_array_* API instead of genrand_* API
I think this can be backported without breaking any APIs. |
I just realized - we can remove all the |
Yes I'll do. How about |
Yes, we should remove those too. Does the code become any faster if we specialize with methods like these, so that for the versions using the GLOBAL_RNG, one doesn't have to pass the MersenneTwitser object around across multiple function calls?
|
I will test, but I had hoped that |
Comparing the state before and after this merge, I find:
After
So |
Yes, as a consequence of using a global Julia object for the global RNG. I will try to restore previous performances as much as I can (#8854 is a first step, and the same kind of optimization as in |
@@ -86,28 +112,28 @@ function srand(filename::String, n::Integer) | |||
end | |||
srand(filename::String) = srand(filename, 4) | |||
|
|||
## Global RNG | |||
|
|||
const GLOBAL_RNG = MersenneTwister() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should add a type annotation on GLOBAL_RNG too.
Adding a type annotation on GLOBAL_RNG doesn't seem to improve performances, nor what you suggested above (#8832 (comment)). |
There isn't yet any mechanism for specializing methods on argument values – even default argument values, although that is quite obviously one of the first things you'd want to do with such an optimization. |
The global versions of |
@rfourquet @ViralBShah |
@rfourquet Would it be possible just to get the array fill improvements without any of the other API changes backported? |
I could do this, but issue #9037 should be resolved first, so this seems difficult for 0.3.3. |
This is the same as PR #8808 (but on a branch in the main repo) updated to work also with the global RNG, which becomes an instance of
MersenneTwister
.Consequently,
rand[!]
functions involving the global RNG become faster when filling arrays ofFloat64
, but slower when producingFloat64
one by one (because of the use of a global Julia object), and, worse, when filling arrays of other types: the problem is that there is a call torand(T)
at each iteration. The solution is to add arand!{T}(mt::MersenneTwister, A::Array{T})
(which implies havingrand(mt, T)
methods) and haverand!{T}(A::Array{T}) = rand!(GLOBAL_RNG, A)
. So should I do this here, or leave that to (a re-write of) PR #8380 (hence keeping the current PR minimal)?WRT points raised by @ViralBShah in #8808:
dsfmt_genrand_*
functions in dSFMT.jl be removed, or left for users who know what they do (a warning could be added in this file)?In order to preserve previous performances when generating unique values or filling small arrays, I had to add a bunch of
@inline
annotations (I'm not sure that all of them are strictly necessary though), which I find noisy, what do you think?What about exporting a function
globalRNG
which returns the GLOBAL_RNG object?