code review 4672050: sync: add fast paths to WaitGroup

sync: add fast paths to WaitGroup
benchmark                                        old ns/op    new ns/op    delta
BenchmarkWaitGroupUncontended                        93.50        33.60  -64.06%
BenchmarkWaitGroupUncontended-2                      44.30        16.90  -61.85%
BenchmarkWaitGroupUncontended-4                      21.80         8.47  -61.15%
BenchmarkWaitGroupUncontended-8                      12.10         4.86  -59.83%
BenchmarkWaitGroupUncontended-16                      7.38         3.35  -54.61%
BenchmarkWaitGroupAddDone                            58.40        33.70  -42.29%
BenchmarkWaitGroupAddDone-2                         293.00        85.80  -70.72%
BenchmarkWaitGroupAddDone-4                         243.00        51.10  -78.97%
BenchmarkWaitGroupAddDone-8                         236.00        52.20  -77.88%
BenchmarkWaitGroupAddDone-16                        215.00        43.30  -79.86%
BenchmarkWaitGroupAddDoneWork                       826.00       794.00   -3.87%
BenchmarkWaitGroupAddDoneWork-2                     450.00       424.00   -5.78%
BenchmarkWaitGroupAddDoneWork-4                     277.00       220.00  -20.58%
BenchmarkWaitGroupAddDoneWork-8                     440.00       116.00  -73.64%
BenchmarkWaitGroupAddDoneWork-16                    569.00        66.50  -88.31%
BenchmarkWaitGroupWait                               29.00         8.04  -72.28%
BenchmarkWaitGroupWait-2                             74.10         4.15  -94.40%
BenchmarkWaitGroupWait-4                            117.00         2.30  -98.03%
BenchmarkWaitGroupWait-8                            111.00         1.31  -98.82%
BenchmarkWaitGroupWait-16                           104.00         1.27  -98.78%
BenchmarkWaitGroupWaitWork                          802.00       792.00   -1.25%
BenchmarkWaitGroupWaitWork-2                        411.00       401.00   -2.43%
BenchmarkWaitGroupWaitWork-4                        210.00       199.00   -5.24%
BenchmarkWaitGroupWaitWork-8                        206.00       105.00  -49.03%
BenchmarkWaitGroupWaitWork-16                       334.00        54.40  -83.71%

[dvyukov, 2011-07-07 18:19:50]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-07-12 05:19:43]

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/atomic/Makefile
File src/pkg/sync/atomic/Makefile (right):

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/atomic/Makefile#...
src/pkg/sync/atomic/Makefile:9: atomic_$(GOARCH).go\
Given that 99% of the code here
is going to be in assembly anyway,
saving the 1% doesn't seem like a win.
Can we move the few implementations that are left
into the asm files and the documentation into
doc.go?

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/atomic/atomic_ar...
File src/pkg/sync/atomic/atomic_arm.go (right):

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/atomic/atomic_ar...
src/pkg/sync/atomic/atomic_arm.go:7: // TODO(dvyukov): These functions should be
re-implemented
Please do.

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/atomic/atomic_ar...
src/pkg/sync/atomic/atomic_arm.go:10: // LoadInt32 atomically loads *addr.
// LoadInt32 reads *addr atomically and returns its value.
same for the others
(+move to doc.go)

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/atomic/atomic_te...
File src/pkg/sync/atomic/atomic_test.go (right):

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/atomic/atomic_te...
src/pkg/sync/atomic/atomic_test.go:481: n := 1000000
Please don't make these bigger.
They are chosen to run in a reasonably short amount of time.

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/waitgroup.go
File src/pkg/sync/waitgroup.go (right):

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/waitgroup.go#new...
src/pkg/sync/waitgroup.go:8: "sync/atomic"
sort

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/waitgroup.go#new...
src/pkg/sync/waitgroup.go:83: wg.waiters--
Please make this atomic.AddInt32 too.
I don't want to have to go through the logic of
why certain read-modify-writes are okay to do
non-atomically, and you just acquired a lock so
the performance penalty is minimal.

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/waitgroup_test.go
File src/pkg/sync/waitgroup_test.go (right):

http://codereview.appspot.com/4672050/diff/7002/src/pkg/sync/waitgroup_test.g...
src/pkg/sync/waitgroup_test.go:136: go func() {
go wg.Done()

[dvyukov, 2011-07-18 14:11:00]

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

Please take another look.

[rsc, 2011-07-18 15:31:50]

please add this comment.  otherwise lgtm

http://codereview.appspot.com/4672050/diff/32001/src/pkg/sync/waitgroup.go
File src/pkg/sync/waitgroup.go (right):

http://codereview.appspot.com/4672050/diff/32001/src/pkg/sync/waitgroup.go#ne...
src/pkg/sync/waitgroup.go:81: atomic.AddInt32(&wg.waiters, 1)
// This code is racing with the unlocked path in Add above.
// The code above modifies counter and then reads waiters.
// We must modify waiters and then read counter (the opposite order)
// to avoid missing an Add.

[dvyukov, 2011-07-18 16:23:11]

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

Please take another look.

[rsc, 2011-07-18 16:36:02]

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

sync: add fast paths to WaitGroup
benchmark                                        old ns/op    new ns/op    delta
BenchmarkWaitGroupUncontended                        93.50        33.60  -64.06%
BenchmarkWaitGroupUncontended-2                      44.30        16.90  -61.85%
BenchmarkWaitGroupUncontended-4                      21.80         8.47  -61.15%
BenchmarkWaitGroupUncontended-8                      12.10         4.86  -59.83%
BenchmarkWaitGroupUncontended-16                      7.38         3.35  -54.61%
BenchmarkWaitGroupAddDone                            58.40        33.70  -42.29%
BenchmarkWaitGroupAddDone-2                         293.00        85.80  -70.72%
BenchmarkWaitGroupAddDone-4                         243.00        51.10  -78.97%
BenchmarkWaitGroupAddDone-8                         236.00        52.20  -77.88%
BenchmarkWaitGroupAddDone-16                        215.00        43.30  -79.86%
BenchmarkWaitGroupAddDoneWork                       826.00       794.00   -3.87%
BenchmarkWaitGroupAddDoneWork-2                     450.00       424.00   -5.78%
BenchmarkWaitGroupAddDoneWork-4                     277.00       220.00  -20.58%
BenchmarkWaitGroupAddDoneWork-8                     440.00       116.00  -73.64%
BenchmarkWaitGroupAddDoneWork-16                    569.00        66.50  -88.31%
BenchmarkWaitGroupWait                               29.00         8.04  -72.28%
BenchmarkWaitGroupWait-2                             74.10         4.15  -94.40%
BenchmarkWaitGroupWait-4                            117.00         2.30  -98.03%
BenchmarkWaitGroupWait-8                            111.00         1.31  -98.82%
BenchmarkWaitGroupWait-16                           104.00         1.27  -98.78%
BenchmarkWaitGroupWaitWork                          802.00       792.00   -1.25%
BenchmarkWaitGroupWaitWork-2                        411.00       401.00   -2.43%
BenchmarkWaitGroupWaitWork-4                        210.00       199.00   -5.24%
BenchmarkWaitGroupWaitWork-8                        206.00       105.00  -49.03%
BenchmarkWaitGroupWaitWork-16                       334.00        54.40  -83.71%

R=rsc
CC=golang-dev
http://codereview.appspot.com/4672050

Committer: Russ Cox <rsc@golang.org>