Skip to content

Conversation

@rfourquet
Copy link
Member

In "random/RNGs.jl", some code was unsafe_wraping a pointer of an Array, which was apparently "completely invalid". reinterpret could work in some case as replacement, but it's not flexible enough, e.g. the following doesn't work: reinterpret(UInt128, UInt32[1, 2, 3]). So we use Ptr directly, which is the dSFMT API anyway, with an UnsafeView<:DenseArray wrapper which makes the code nicer. So this is a re-implementation of #9174 in disguise, and allows some nice speed-up for rand! on small arrays as a result of allocating less (unsafe_wrap allocates), for example a = UInt64[1]; rand!(a) has a speed-up of 85% on my machine.

@rfourquet rfourquet added bugfix This change fixes an existing bug performance Must go faster randomness Random number generation and the Random stdlib labels Dec 2, 2017
@rfourquet
Copy link
Member Author

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

@nanosoldier
Copy link
Collaborator

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

@ViralBShah
Copy link
Member

cc @andreasnoack @simonbyrne for review.

@rfourquet
Copy link
Member Author

@vtjnash even without reviewing in detail, would you mind skimming over this to check that this change indeed fixes the previously invalid code? Basically the rand! "kernel" receives a Ptr (wrapped in an UnsafeView), and other rand!(::Array) methods call to the kernel by sending a pointer the array with a length.

@rfourquet
Copy link
Member Author

I would like to merge this today or tomorrow, to make another PR touching this code. I believe this change is correct, and anyway the old one was "invalid" so it's unlikely to get worse.

@rfourquet rfourquet force-pushed the rf/rand/ptr branch 2 times, most recently from 372beb7 to 9ae6c8a Compare December 12, 2017 15:11
@StefanKarpinski
Copy link
Member

Please go for it then, @rfourquet!

The cumulated improvement in performance together with the
previous commit is of the order of 50%.
@rfourquet rfourquet added this to the 1.0 milestone Dec 13, 2017
@rfourquet rfourquet merged commit f1baa90 into master Dec 13, 2017
@rfourquet
Copy link
Member Author

Big thanks for your support Stefan with my PRs !

@rfourquet rfourquet deleted the rf/rand/ptr branch December 13, 2017 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This change fixes an existing bug 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.

5 participants