code review 162044: sync.RWMutex: rewritten to add support for concurrent r...

sync.RWMutex: rewritten to add support for concurrent readers.

Also made sync.xadd public to help testing sync.RWMutex.

Also added unit tests for sync.RWMutex.

[pts, 2009-11-27 14:12:38]

Hello rsc,

I'd like you to review the following change.

[rsc, 2009-11-29 23:42:23]

Thanks.

Coincidentally, I wrote a real RWMutex on the
plane last night, but yours is nicer.

http://codereview.appspot.com/162044/diff/6/7
File src/pkg/sync/mutex.go (right):

http://codereview.appspot.com/162044/diff/6/7#newcode13
src/pkg/sync/mutex.go:13: // Thread-safe compare-and-swap operation.
delete
(not the right form for a doc comment,
not really necessary)

http://codereview.appspot.com/162044/diff/6/7#newcode20
src/pkg/sync/mutex.go:20: // same lock, a random waiter gets the lock as soon as
it's available.
It's not random.
In fact I think it's FIFO, but regardless it seems
better to leave this out and not overspecify the
behavior.

http://codereview.appspot.com/162044/diff/6/7#newcode26
src/pkg/sync/mutex.go:26: // Add delta to *val, and return the new *val in a
thread-safe way. If multiple
I would rather not export this, at least not in this form.

Instead, create a new file xadd_test.go that says

// copyright

package sync

func Xadd(val *uint32, delta int32) (new uint32) {
    return xadd(val, delta)
}

and then you can call Xadd from tests but leave
it hidden from ordinary clients.

http://codereview.appspot.com/162044/diff/6/7#newcode65
src/pkg/sync/mutex.go:65: // An RWMutex is a reader/writer mutual exclusion
lock.
Could you please move this to a new file named rwmutex.go?

http://codereview.appspot.com/162044/diff/6/7#newcode72
src/pkg/sync/mutex.go:72: // Readers cannot make waiting writers starve, i.e. no
RLock()s are granted
// Writers take priority over Readers: no new RLocks
// are granted while a blocked Lock call is waiting.

http://codereview.appspot.com/162044/diff/6/7#newcode75
src/pkg/sync/mutex.go:75: // for the same lock, a random waiter gets the lock as
soon as it's
Again, definitely not random.
Better to leave this sentence out.

http://codereview.appspot.com/162044/diff/6/7#newcode78
src/pkg/sync/mutex.go:78: access		Mutex;	// held if there are pending readers or
writers
"access" and "acquire" are not descriptive
enough for me.  Can we call them r and w?

http://codereview.appspot.com/162044/diff/6/8
File src/pkg/sync/mutex_test.go (right):

http://codereview.appspot.com/162044/diff/6/8#newcode94
src/pkg/sync/mutex_test.go:94: func parallelReader(m *RWMutex, clocked, cunlock,
cdone chan bool) {
Please move this into a new file rwmutex_test.go.

http://codereview.appspot.com/162044/diff/6/8#newcode102
src/pkg/sync/mutex_test.go:102: func doTestParallelReaders(num_readers,
gomaxprocs int) {
s/num_readers/numReaders/g

http://codereview.appspot.com/162044/diff/6/8#newcode176
src/pkg/sync/mutex_test.go:176: for i := -2; i < num_readers; i++ {
-2?
Much clearer to write

for i := 0; i < 2+numReaders; i++ {

[pts, 2009-11-30 13:00:59]

Done everything requested. Please have a look again.

http://codereview.appspot.com/162044/diff/6/7
File src/pkg/sync/mutex.go (right):

http://codereview.appspot.com/162044/diff/6/7#newcode13
src/pkg/sync/mutex.go:13: // Thread-safe compare-and-swap operation.
On 2009/11/29 23:42:23, rsc wrote:
> delete
> (not the right form for a doc comment,
> not really necessary)
> 

Done.

http://codereview.appspot.com/162044/diff/6/7#newcode20
src/pkg/sync/mutex.go:20: // same lock, a random waiter gets the lock as soon as
it's available.
On 2009/11/29 23:42:23, rsc wrote:
> It's not random.
> In fact I think it's FIFO, but regardless it seems
> better to leave this out and not overspecify the
> behavior.
> 

Done.

http://codereview.appspot.com/162044/diff/6/7#newcode20
src/pkg/sync/mutex.go:20: // same lock, a random waiter gets the lock as soon as
it's available.
By the way, my test indicates that it's not a perfect FIFO:

http://code.google.com/p/pts-mini-gpl/source/browse/trunk/go/lockorder.go

Or my test is not perfect?

http://codereview.appspot.com/162044/diff/6/7#newcode26
src/pkg/sync/mutex.go:26: // Add delta to *val, and return the new *val in a
thread-safe way. If multiple
On 2009/11/29 23:42:23, rsc wrote:
> I would rather not export this, at least not in this form.
> 
> Instead, create a new file xadd_test.go that says
> 
> // copyright
> 
> package sync
> 
> func Xadd(val *uint32, delta int32) (new uint32) {
>     return xadd(val, delta)
> }
> 
> and then you can call Xadd from tests but leave
> it hidden from ordinary clients.
> 
> 

Done.

http://codereview.appspot.com/162044/diff/6/7#newcode65
src/pkg/sync/mutex.go:65: // An RWMutex is a reader/writer mutual exclusion
lock.
On 2009/11/29 23:42:23, rsc wrote:
> Could you please move this to a new file named rwmutex.go?
> 

Done.

http://codereview.appspot.com/162044/diff/6/7#newcode72
src/pkg/sync/mutex.go:72: // Readers cannot make waiting writers starve, i.e. no
RLock()s are granted
On 2009/11/29 23:42:23, rsc wrote:
> // Writers take priority over Readers: no new RLocks
> // are granted while a blocked Lock call is waiting.
> 

Done.

http://codereview.appspot.com/162044/diff/6/7#newcode75
src/pkg/sync/mutex.go:75: // for the same lock, a random waiter gets the lock as
soon as it's
On 2009/11/29 23:42:23, rsc wrote:
> Again, definitely not random.
> Better to leave this sentence out.
> 

Done.

http://codereview.appspot.com/162044/diff/6/7#newcode78
src/pkg/sync/mutex.go:78: access		Mutex;	// held if there are pending readers or
writers
On 2009/11/29 23:42:23, rsc wrote:
> "access" and "acquire" are not descriptive
> enough for me.  Can we call them r and w?
> 
> 

Done.

http://codereview.appspot.com/162044/diff/6/8
File src/pkg/sync/mutex_test.go (right):

http://codereview.appspot.com/162044/diff/6/8#newcode94
src/pkg/sync/mutex_test.go:94: func parallelReader(m *RWMutex, clocked, cunlock,
cdone chan bool) {
On 2009/11/29 23:42:23, rsc wrote:
> Please move this into a new file rwmutex_test.go.
> 

Done.

http://codereview.appspot.com/162044/diff/6/8#newcode102
src/pkg/sync/mutex_test.go:102: func doTestParallelReaders(num_readers,
gomaxprocs int) {
On 2009/11/29 23:42:23, rsc wrote:
> s/num_readers/numReaders/g
> 
> 

Done.

http://codereview.appspot.com/162044/diff/6/8#newcode176
src/pkg/sync/mutex_test.go:176: for i := -2; i < num_readers; i++ {
On 2009/11/29 23:42:23, rsc wrote:
> -2?
> Much clearer to write
> 
> for i := 0; i < 2+numReaders; i++ {
> 

Done.

[rsc, 2009-11-30 16:55:14]

> By the way, my test indicates that it's not a perfect FIFO:
> http://code.google.com/p/pts-mini-gpl/source/browse/trunk/go/lockorder.go
>
> Or my test is not perfect?

Oh right - it's not quite FIFO because a fast goroutine
might come in and swipe the lock instead of queuing.
Still, definitely not random.  ;-)

Russ

[pts, 2009-11-30 19:51:25]

Any update on this code review. Do I get LGTM?

[rsc, 2009-11-30 19:52:59]

On Mon, Nov 30, 2009 at 11:51,  <pts@google.com> wrote:
> Any update on this code review. Do I get LGTM?
>
> http://codereview.appspot.com/162044

i'll get there - i was offline most of last week
and have a ton of code reviews piled up.
sorry for the delay.

[rsc, 2009-11-30 20:06:07]

About to submit this.
I tweaked the Makefile to be
in the standard one-file-per-line form,
so you will want to revert your change
before sync to avoid a conflict:

cd $GOROOT
hg revert src/pkg/sync/Makefile
hg sync

[rsc, 2009-11-30 20:10:59]

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

sync.RWMutex: rewritten to add support for concurrent readers.

Also made sync.xadd public to help testing sync.RWMutex.

Also added unit tests for sync.RWMutex.

R=rsc
http://codereview.appspot.com/162044

Committer: Russ Cox <rsc@golang.org>

[pts, 2009-11-30 20:19:24]

> i'll get there - i was offline most of last week
> and have a ton of code reviews piled up.
> sorry for the delay.

Thanks for the quick response.