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

math/rand: Calling single rand.Rand object from multiple threaded gorouttiness panics #8308

Closed
gopherbot opened this issue Jul 1, 2014 · 1 comment

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 1, 2014

by suokkos:

Sharing single rand.Rand object using mutex synchronization kills parallel performance
if code is using random numbers heavily. To avoid synchronization over-head one can
allocate rand.Rand object per goroutine. But rand.Rand object has pretty large internal
state that causes memory to be scaling bottleneck.

I looked the generator implementation and noticed that it is very simple to make thread
safe without mutex locking. This bug report includes my test cases and actual working
implementation change.

What does 'go version' print?
go version devel +423bfcb6b3ce Tue Jul 01 18:04:46 2014 +0300 linux/amd64

(I originally noticed this scaling issue with gccgo 4.9.0 20140405 (experimental) [trunk
revision 209157])

What steps reproduce the problem?

1. Download attached gambling.go  (parallel implementation of pig dice game example;
http://golang.org/doc/codewalk/functions/)
2. go run gambling.go -p unlimitedshared

What happened?
panic: runtime error: index out of range

goroutine 3 [running]:
main.roll
    gambling.go:51
main.play
    gambling.go:99
main.$nested1
    gambling.go:180
created by main.main
    gambling.go:326


What should have happened instead?
Program could complete without errors with minor changes to rngSource implementation.

Please provide any additional information below.

Other attachments include an unit test case for the issue and possible fix.

Changes feel like breaking ABI so I have no idea. But I don't have yet enough
understanding how go works to be sure if that kind ABI change is allowed or not.

Changes generate deterministic number sequence in random order as long as there is less
than _TAP number of simultaneous calls to rngSource.Int63. The change panics at _LEN
number of parallel calls even tough any number above _TAP could be handled easily
without panic. Possible alternative would be:

for feed < 0 {
   feed -= _LEN
}

If someone sees enough value in these changes they can be freely taken and modified to
acceptable form. I'm unlikely to try to submit them to review any time soon.

ATTETION! Change doesn't use explicit atomic writes for rngSource.vec because I assumed
that writes to vec index don't affect neighbour entries. But that might not be true in
all cpu architectures.

Attachments:

  1. gambling.go (8076 bytes)
  2. 0001-test-case-for-threaded-rand-use.patch (11680 bytes)
  3. 0002-allow-parallel-calls-to-rngSource-Int63.patch (1923 bytes)
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 1, 2014

Comment 1:

Thanks.  We can only accept patches that go through the code review process described at
http://golang.org/doc/contribute.html .  In this case the change should be discussed on
the golang-dev list first.

Status changed to WontFix.

@gopherbot gopherbot added the wontfix label Jul 1, 2014
@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.