Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(579)

Issue 3770045: code review 3770045: sync: Proposal for barrier implementation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 2 months ago by niemeyer
Modified:
14 years, 1 month ago
Reviewers:
CC:
rsc, rog, r, golang-dev
Visibility:
Public.

Description

sync: Proposal for barrier implementation As discussed in the mailing list, this adds a simple barrier implementation to the sync package which enables one or more goroutines to wait for a counter to go down to zero.

Patch Set 1 #

Patch Set 2 : code review 3770045: sync: Proposal for barrier implementation #

Patch Set 3 : code review 3770045: sync: Proposal for barrier implementation #

Patch Set 4 : code review 3770045: sync: Proposal for barrier implementation #

Patch Set 5 : code review 3770045: sync: Proposal for barrier implementation #

Total comments: 2

Patch Set 6 : code review 3770045: sync: Proposal for barrier implementation #

Total comments: 2

Patch Set 7 : code review 3770045: sync: Proposal for barrier implementation #

Patch Set 8 : code review 3770045: sync: Proposal for barrier implementation #

Patch Set 9 : code review 3770045: sync: Proposal for barrier implementation #

Patch Set 10 : code review 3770045: sync: Proposal for barrier implementation #

Patch Set 11 : code review 3770045: sync: Proposal for barrier implementation #

Patch Set 12 : code review 3770045: sync: Proposal for barrier implementation #

Total comments: 19

Patch Set 13 : code review 3770045: sync: Proposal for barrier implementation #

Patch Set 14 : code review 3770045: sync: Proposal for barrier implementation #

Total comments: 3

Patch Set 15 : code review 3770045: sync: Proposal for barrier implementation #

Total comments: 7

Patch Set 16 : diff -r 138fb74f2651 https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 17 : diff -r 138fb74f2651 https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 18 : diff -r 138fb74f2651 https://go.googlecode.com/hg/ #

Patch Set 19 : diff -r 138fb74f2651 https://go.googlecode.com/hg/ #

Patch Set 20 : diff -r 138fb74f2651 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+151 lines, -4 lines) Patch
M src/pkg/sync/Makefile View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/sync/mutex.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +4 lines, -4 lines 0 comments Download
A src/pkg/sync/waitgroup.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +86 lines, -0 lines 0 comments Download
A src/pkg/sync/waitgroup_test.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +60 lines, -0 lines 0 comments Download

Messages

Total messages: 22
niemeyer
Hello rsc (cc: golang-dev@googlegroups.com), I'd like you to review this change.
14 years, 2 months ago (2011-01-05 12:11:52 UTC) #1
niemeyer
Added Done() and DoneWait() to the API, using the same primitives. The former is just ...
14 years, 2 months ago (2011-01-05 12:52:25 UTC) #2
niemeyer
I've reduced the proposal to reduce the API to the part that is not being ...
14 years, 2 months ago (2011-01-05 22:14:01 UTC) #3
rog
http://codereview.appspot.com/3770045/diff/17001/src/pkg/sync/barrier_test.go File src/pkg/sync/barrier_test.go (right): http://codereview.appspot.com/3770045/diff/17001/src/pkg/sync/barrier_test.go#newcode16 src/pkg/sync/barrier_test.go:16: max = i what's this trying to test? it's ...
14 years, 2 months ago (2011-01-06 15:22:53 UTC) #4
niemeyer
- Now based on semaphores (faster, reduced memory use). - Renamed to WaitGroup as requested. ...
14 years, 2 months ago (2011-01-06 16:38:43 UTC) #5
niemeyer
Problem fixed in the latest version, and other improvements. http://codereview.appspot.com/3770045/diff/17001/src/pkg/sync/barrier_test.go File src/pkg/sync/barrier_test.go (right): http://codereview.appspot.com/3770045/diff/17001/src/pkg/sync/barrier_test.go#newcode16 src/pkg/sync/barrier_test.go:16: ...
14 years, 2 months ago (2011-01-06 16:39:43 UTC) #6
rog
http://codereview.appspot.com/3770045/diff/39001/src/pkg/sync/waitgroup.go File src/pkg/sync/waitgroup.go (right): http://codereview.appspot.com/3770045/diff/39001/src/pkg/sync/waitgroup.go#newcode9 src/pkg/sync/waitgroup.go:9: // to wait for a task which is being ...
14 years, 2 months ago (2011-01-06 20:38:43 UTC) #7
niemeyer
Comments addressed. Thanks. http://codereview.appspot.com/3770045/diff/39001/src/pkg/sync/waitgroup.go File src/pkg/sync/waitgroup.go (right): http://codereview.appspot.com/3770045/diff/39001/src/pkg/sync/waitgroup.go#newcode9 src/pkg/sync/waitgroup.go:9: // to wait for a task ...
14 years, 2 months ago (2011-01-06 21:31:39 UTC) #8
rsc
Please read golang.org/doc/effective_go.html#commentary The doc comments should begin with nouns. Russ
14 years, 2 months ago (2011-01-06 22:19:32 UTC) #9
rog
http://codereview.appspot.com/3770045/diff/39001/src/pkg/sync/waitgroup.go File src/pkg/sync/waitgroup.go (right): http://codereview.appspot.com/3770045/diff/39001/src/pkg/sync/waitgroup.go#newcode42 src/pkg/sync/waitgroup.go:42: // becomes zero, all goroutines blocked on Wait() are ...
14 years, 2 months ago (2011-01-06 22:44:03 UTC) #10
niemeyer
> Please read golang.org/doc/effective_go.html#commentary > > The doc comments should begin with nouns. Missed that, ...
14 years, 2 months ago (2011-01-06 23:00:46 UTC) #11
rog
http://codereview.appspot.com/3770045/diff/47001/src/pkg/sync/waitgroup.go File src/pkg/sync/waitgroup.go (right): http://codereview.appspot.com/3770045/diff/47001/src/pkg/sync/waitgroup.go#newcode12 src/pkg/sync/waitgroup.go:12: // given number of goroutines have finished. Although this ...
14 years, 2 months ago (2011-01-07 10:49:01 UTC) #12
niemeyer
Updated tests to reflect recent changes on non-blocking channel receiving. Also tweaked some docstrings following ...
14 years, 1 month ago (2011-02-01 10:06:06 UTC) #13
rsc
looks good http://codereview.appspot.com/3770045/diff/13001/src/pkg/sync/barrier.go File src/pkg/sync/barrier.go (right): http://codereview.appspot.com/3770045/diff/13001/src/pkg/sync/barrier.go#newcode4 src/pkg/sync/barrier.go:4: // Synchronization primitive which enables blocking one ...
14 years, 1 month ago (2011-02-03 04:10:20 UTC) #14
niemeyer
All addressed, with the following details. PTAL http://codereview.appspot.com/3770045/diff/13001/src/pkg/sync/barrier.go File src/pkg/sync/barrier.go (right): http://codereview.appspot.com/3770045/diff/13001/src/pkg/sync/barrier.go#newcode4 src/pkg/sync/barrier.go:4: // Synchronization ...
14 years, 1 month ago (2011-02-03 14:28:54 UTC) #15
rsc
http://codereview.appspot.com/3770045/diff/59001/src/pkg/sync/waitgroup.go File src/pkg/sync/waitgroup.go (right): http://codereview.appspot.com/3770045/diff/59001/src/pkg/sync/waitgroup.go#newcode39 src/pkg/sync/waitgroup.go:39: // G3: Add(1) G3 still comes out of nowhere. ...
14 years, 1 month ago (2011-02-03 15:32:19 UTC) #16
niemeyer
> There needs to be a reason that G3 would think that > the waitgroup ...
14 years, 1 month ago (2011-02-03 16:27:28 UTC) #17
rsc
Seems fine to me. Adding r to reviewers for his comments.
14 years, 1 month ago (2011-02-03 18:24:36 UTC) #18
r
looks good but the package comment will need to be updated http://codereview.appspot.com/3770045/diff/61001/src/pkg/sync/waitgroup.go File src/pkg/sync/waitgroup.go (right): ...
14 years, 1 month ago (2011-02-03 20:18:07 UTC) #19
niemeyer
Thanks for all the reviews. Both done, and also added the copyright header into the ...
14 years, 1 month ago (2011-02-03 20:31:07 UTC) #20
r
LGTM
14 years, 1 month ago (2011-02-03 20:38:30 UTC) #21
r
14 years, 1 month ago (2011-02-03 20:39:27 UTC) #22
*** Submitted as http://code.google.com/p/go/source/detail?r=85f202d81c64 ***

sync: Proposal for barrier implementation

As discussed in the mailing list, this adds a simple barrier
implementation to the sync package which enables one or more
goroutines to wait for a counter to go down to zero.

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

Committer: Rob Pike <r@golang.org>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b