-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Disable fill_array calls in DSFMT to fix 6573 #6581
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
Conversation
Remove unused functions in librandom.jl.
There appears to be a small slowdown as a result when generating large rand arrays.
Disable fill_array calls in DSFMT to fix 6573
|
Here's the crux of the issue: From dSFMT.c:439 (the comment header to Seems like a very strange limitation. But it's exactly the requirement I noted in #6573. Very unfortunate… I wonder how many other users of dSFMT might be unknowingly hitting this. |
|
Thanks for tracking that down. That's a pretty terrible limitation that doesn't seem to be noted prominently enough at all. |
|
You are okay if you use either the |
|
Oh wow, so DSFMT is essentially useless. |
|
Right, as I understand it, they both update the state, but they do so in ways that are incompatible with the other. |
|
This is such an utter and complete failure of both API and documentation. These bits of dangerously conflicting functionality don't belong in the same library – they should be split into separate libraries so they can't trample each other like this. We may need a new RNG in the longer term. It's unfortunate that initial testing of Random123 has shown it not to be nearly as fast. |
|
Internally, I believe it still generates random numbers in batches, and the performance of calling genrand repeatedly seems OK. Don't know how I missed that warning. |
used together, without reinitializing the state. This commit removes wrappers around DSFMT's fill_array functions. It is easy enough to add them back if required at a later point.
|
No worries, I really don't think that burying a DIRE warning like that in some C file is acceptable. That sort of thing should be plastered all over the homepage of the library, not hidden away somewhere. |
|
Or they could have just used a different struct for the state. We had tested the quality of random numbers from DSFMT, but not by mixing the individual and vectorised generation. |
|
I guess one good thing might be that in real production code people may not be so likely to mix these. |
|
I've not fully read the dSFMT papers, so I don't know if this is just unfounded paranoia, but the possibility of having the two states end up parallel to each other is worrying to me. Really, the performance regression here isn't as bad as I expected. The iterative genrand is only 2x (with 400 elements) to 3x (with 100_000_000 elts) slower than fill_array on my machine. Making the opposite choice (only calling the fill_array flavors) is more problematic: the one element genrand smokes the minimum fill_array by 25x. |
|
The whole selling point of dSFMT in the first place is that it uses SIMD instructions to get a 2x performance improvement. If we're stuck using their non-SIMD code which is 2-3x slower than the SIMD version then what the hell is the point? This experience leaves me inclined to ditch dSFMT altogether. |
|
How about asking them about the traps you suspect are left in their code? ;-) |
|
We are still using the SIMD code and I haven't been able to see a performance drop yet, much to my amazement. |
|
Didn't see the comments above. I Will try to replicate the performance. |
For #6573 I looked through the code but it appears that we are calling the
fill_arrayroutines correctly. For now, to fix this issue, I am disabling thefill_arrayroutines, so that we only calldsfmt_gv_genrand_close_openfor all random number generation.