Skip to content
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: support threads for GLOBAL_RNG access #32407

Merged
merged 1 commit into from Jul 19, 2019
Merged

Random: support threads for GLOBAL_RNG access #32407

merged 1 commit into from Jul 19, 2019

Conversation

@vtjnash
Copy link
Member

vtjnash commented Jun 24, 2019

No description provided.

@vtjnash vtjnash added the triage label Jun 27, 2019
@JeffBezanson

This comment has been minimized.

Copy link
Member

JeffBezanson commented Jul 3, 2019

From triage: make default global RNG per-thread for now. Document that explicit RNG objects are not thread-safe.

@StefanKarpinski

This comment has been minimized.

Copy link
Member

StefanKarpinski commented Jul 3, 2019

General thoughts from triage:

  • calling rand() should be thread-safe even if there's a performance hit
  • people are comfortable with changing the default rng to PCG64/Xorshift128whatever
    • including this with multithreading might help with popularity since it combines good with bad
  • using a PRNG with a small state might help since we can use atomics
  • explicit RNG objects are not thread-safe, if you want to use them from multiple threads, use locks
@vtjnash vtjnash removed the triage label Jul 8, 2019
@vtjnash vtjnash marked this pull request as ready for review Jul 12, 2019
@vtjnash vtjnash force-pushed the jn/trng branch from 137d004 to b984cd5 Jul 12, 2019
@StefanKarpinski

This comment has been minimized.

Copy link
Member

StefanKarpinski commented Jul 15, 2019

Calling the function to get the global RNG get_local_rng is a bit confusing. I know it's thread-local, but it's global in the sense of scope. Maybe get_thread_rng?

@StefanKarpinski

This comment has been minimized.

Copy link
Member

StefanKarpinski commented Jul 15, 2019

Or maybe just default_rng()?

@JeffBezanson JeffBezanson changed the title RFC: Random: support threads for GLOBAL_RNG access Random: support threads for GLOBAL_RNG access Jul 17, 2019
@JeffBezanson

This comment has been minimized.

Copy link
Member

JeffBezanson commented Jul 17, 2019

Rebased to pick up #32604.

I think we can merge this, and think about the naming convention for functions like this. I know people might start using it, but it's not exported and we'll have time to rename it if we want before 1.3.

#RNG = get(tls, :RNG, nothing)
#RNG isa MersenneTwister && return RNG
if length(THREAD_RNGs) < tid
resize!(THREAD_RNGs, Threads.nthreads())

This comment has been minimized.

Copy link
@JeffBezanson

JeffBezanson Jul 18, 2019

Member

I'm not sure whether this is thread safe.

This comment has been minimized.

Copy link
@vtjnash

vtjnash Jul 18, 2019

Author Member

Yeah, I still need to push my local fork which fixes some stuff like this. Was just waiting on the outcome of the naming discussion.

The `Random.GLOBAL_RNG` is now a singleton placeholder object
which implements the prior `Random` public API for MersenneTwister
as a shim to support existing clients until Julia v2.0.
@vtjnash vtjnash force-pushed the jn/trng branch from 84eca18 to bc4b2e8 Jul 18, 2019
@JeffBezanson JeffBezanson merged commit 5c7ed66 into master Jul 19, 2019
18 checks passed
18 checks passed
buildbot/analyzegc_linux64 Run complete
Details
buildbot/doctest_linux64 Run complete
Details
buildbot/llvmpasses_linux64 Run complete
Details
buildbot/package_freebsd64 Run complete
Details
buildbot/package_linux32 Run complete
Details
buildbot/package_linux64 Run complete
Details
buildbot/package_macos64 Run complete
Details
buildbot/package_win32 Run complete
Details
buildbot/package_win64 Run complete
Details
buildbot/tester_freebsd64 Run complete
Details
buildbot/tester_linux32 Run complete
Details
buildbot/tester_linux64 Run complete
Details
buildbot/tester_macos64 Run complete
Details
buildbot/tester_win32 Run complete
Details
buildbot/tester_win64 Run complete
Details
buildbot/whitespace_linux32 Run complete
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JeffBezanson JeffBezanson deleted the jn/trng branch Jul 19, 2019
@rfourquet

This comment has been minimized.

Copy link
Contributor

rfourquet commented Jul 20, 2019

Or maybe just default_rng()

I agree with this suggestion, and had the same idea when reading this PR the first time. I find both get_local_rng() and get_global_rng() confusing, while default_rng() conveys the practical meaning without implementation details (if I may!) considerations (default_rng() naming works independantly of the threading abilities of the runtime, and in fact there was at one point an unexported function defaultRNG() = GLOBAL_RNG).

@testset "GLOBAL_RNG" begin
local GLOBAL_RNG = Random.GLOBAL_RNG
local LOCAL_RNG = Random.get_local_rng()
@test VERSION < v"2" # deprecate this in v2

This comment has been minimized.

Copy link
@rfourquet

rfourquet Jul 20, 2019

Contributor

The _GLOBAL_RNG thing is rather clever! Do I get it right that it's only to avoid breaking code and that it's intended to be removed in v2?

@mauro3

This comment has been minimized.

Copy link
Contributor

mauro3 commented Jan 9, 2020

What is the reason that in this PR separate RNG stream are created (link to code) instead of using the "trick" advocated in the docs of using just one chain but sampling from different locations (implemented using the Future.randjump)?

Wikipedia (fourth bullet) suggests that at least for MCMC the randjump strategy should be used. Or are the seeds of the separate streams chosen such that inter-stream numbers are non-correlated too? This line suggest that no such approach is taken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.