code review 168041: Make the operations on the global rng thread safe.

Make the operations on the global rng thread safe.

[rog, 2009-12-09 13:48:27]

Hello r, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review the following change.

[r, 2009-12-09 17:59:50]

http://codereview.appspot.com/168041/diff/1003/1005
File src/pkg/rand/ts.go (right):

http://codereview.appspot.com/168041/diff/1003/1005#newcode25
src/pkg/rand/ts.go:25: tssrc := tssource{make(chan int64), make(chan int64)};
these should certainly be buffered.

[rsc, 2009-12-09 18:11:16]

I'm not convinced about this.
I think it's a bug that the top-level functions aren't
safe to be called from multiple goroutines, but this
change won't fix that.  I also think that once you're
willing to construct a specific instance of the source
(i.e., call ThreadSafe) you might as well create one
per goroutine and not have any sharing at all.

My counterproposal would be to put a lock in
the global top-level versions and leave the rest alone.

Russ

[r2, 2009-12-09 18:21:04]

On Dec 9, 2009, at 10:11 AM, Russ Cox wrote:

> I'm not convinced about this.
> I think it's a bug that the top-level functions aren't
> safe to be called from multiple goroutines, but this
> change won't fix that.  I also think that once you're
> willing to construct a specific instance of the source
> (i.e., call ThreadSafe) you might as well create one
> per goroutine and not have any sharing at all.
> 
> My counterproposal would be to put a lock in
> the global top-level versions and leave the rest alone.

yup

-rob

[rog, 2009-12-09 18:21:30]

2009/12/9 Russ Cox <rsc@golang.org>:
> My counterproposal would be to put a lock in
> the global top-level versions and leave the rest alone.

this would certainly be easier to use, just as
long as it doesn't have any significant performance
impact.

if performance is an issue, then perhaps NewSource could
take a threadsafe bool argument, and globalRand
would be initialised threadsafe.

that way at least the default case is safe - if you
want speed, at the cost of some risk, you can
get an unlocked version.

[rsc, 2009-12-09 18:31:01]

My proposal was that the top-level funcs are locked (safe)
but that the methods on the Rand and Source types are not;
if you care enough about performance, then you
create your own object and share it safely just as you
would any other object.

Russ

[rog, 2009-12-09 18:44:21]

http://codereview.appspot.com/168041/diff/1003/1005
File src/pkg/rand/ts.go (right):

http://codereview.appspot.com/168041/diff/1003/1005#newcode25
src/pkg/rand/ts.go:25: tssrc := tssource{make(chan int64), make(chan int64)};
On 2009/12/09 17:59:51, r wrote:
> these should certainly be buffered.

if they were, then you'd lose the property that if you set a seed, then you know
that the next value random value has been seeded accordingly.

an alternative take is to make Seed a no-op or an error on a concurrent Source -
it's not very useful anyway, because ordering of reads is unpredictable between
processes.

in that case, only one channel would be used, and it would certainly be
buffered.

http://codereview.appspot.com/168041/diff/1003/1005#newcode25
src/pkg/rand/ts.go:25: tssrc := tssource{make(chan int64), make(chan int64)};
On 2009/12/09 17:59:51, r wrote:
> these should certainly be buffered.

on second thoughts, it would be possible to use a buffered channel even with
seed setting, by draining the channel of values when a seed is set. given that
setting a seed is synchronous, it wouldn't make any difference to buffer seedc.

[rog, 2009-12-09 18:52:36]

Hello r, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review the following change.

[rog, 2009-12-09 18:55:58]

2009/12/9 Russ Cox <rsc@golang.org>:
> My proposal was that the top-level funcs are locked (safe)
> but that the methods on the Rand and Source types are not;
> if you care enough about performance, then you
> create your own object and share it safely just as you
> would any other object.

rather than put a lock around all top level funcs,
which would be tedious, i've changed things to use
the threadsafe function internally.

i believe that this might easily be faster too, as
it will need less qlocks per operation due to
the buffered channel.

YMMV!

[rog, 2009-12-09 19:19:14]

after some brief testing, it appears that the mutex-based
version is about 100x faster than the channel-based version.

so that's what i've changed it to.

2009/12/9 roger peppe <rogpeppe@gmail.com>:
> 2009/12/9 Russ Cox <rsc@golang.org>:
>> My proposal was that the top-level funcs are locked (safe)
>> but that the methods on the Rand and Source types are not;
>> if you care enough about performance, then you
>> create your own object and share it safely just as you
>> would any other object.
>
> rather than put a lock around all top level funcs,
> which would be tedious, i've changed things to use
> the threadsafe function internally.
>
> i believe that this might easily be faster too, as
> it will need less qlocks per operation due to
> the buffered channel.
>
> YMMV!
>

[rsc, 2009-12-09 19:26:38]

looks pretty good, thanks.

http://codereview.appspot.com/168041/diff/2006/16
File src/pkg/rand/rand.go (right):

http://codereview.appspot.com/168041/diff/2006/16#newcode154
src/pkg/rand/rand.go:154: type tssource struct {
i suggest

type lockedSource struct {
    lk Lock;
    src Source;
}

and then use &lockedSource{src: xxx} above.
(No need for a one-use local constructor,
and the lock should be associated with the
instance in case there is ever a second.)

[rog, 2009-12-09 19:57:06]

2009/12/9  <rsc@golang.org>:
> type lockedSource struct {
>    lk Lock;
>    src Source;
> }

done. this actually speeded things up by 1%
(passing the pointer rather than the 64 bit interface).

i get another 0.8% speed up by making lockedSource
work directly on rngSource; not worth it, i think.
0.8% overhead for calling an interface really isn't
bad!

[rog, 2009-12-09 20:12:30]

FWIW, i get 90ns per rand.Int32(),
compared to 26.2ns for the thread-unsafe version.


2009/12/9 roger peppe <rogpeppe@gmail.com>:
> 2009/12/9  <rsc@golang.org>:
>> type lockedSource struct {
>>    lk Lock;
>>    src Source;
>> }
>
> done. this actually speeded things up by 1%
> (passing the pointer rather than the 64 bit interface).
>
> i get another 0.8% speed up by making lockedSource
> work directly on rngSource; not worth it, i think.
> 0.8% overhead for calling an interface really isn't
> bad!
>

[r2, 2009-12-09 20:29:23]

On Dec 9, 2009, at 11:56 AM, roger peppe wrote:

> 2009/12/9  <rsc@golang.org>:
>> type lockedSource struct {
>>    lk Lock;
>>    src Source;
>> }
>
> done. this actually speeded things up by 1%
> (passing the pointer rather than the 64 bit interface).
>
> i get another 0.8% speed up by making lockedSource
> work directly on rngSource; not worth it, i think.
> 0.8% overhead for calling an interface really isn't
> bad!

that means you have benchmarks. please add them to the test.  see fmt  
or bytes for examples.

-rob

[rsc, 2009-12-09 20:55:21]

*** Submitted as http://code.google.com/p/go/source/detail?r=4a235a08b490 ***

Make the operations on the global rng thread safe.

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/168041

Committer: Russ Cox <rsc@golang.org>

[rog, 2009-12-09 21:09:25]

2009/12/9 Rob 'Commander' Pike <r@google.com>:
> that means you have benchmarks. please add them to the test.  see fmt or
> bytes for examples.

done. although does it need a new issue now that russ has submitted it?

[rsc, 2009-12-09 21:14:01]

> done. although does it need a new issue now that russ has submitted it?

yes, sorry.
please run "hg sync", which will
close your current issue, and
then you can create a new one.