Issue 4625065: code review 4625065: runtime: add Semacquire/Semrelease benchmarks (Closed)
|
Created:
14 years, 2 months ago by dvyukov Modified:
14 years, 2 months ago Reviewers:
CC:
rsc, golang-dev Visibility:
Public. |
Descriptionruntime: add Semacquire/Semrelease benchmarks
Patch Set 1 #Patch Set 2 : diff -r 6f1145ee588d https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 6f1145ee588d https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 6f1145ee588d https://go.googlecode.com/hg/ #Patch Set 5 : diff -r b295b8bda20b https://go.googlecode.com/hg/ #
Total comments: 3
Patch Set 6 : diff -r b295b8bda20b https://go.googlecode.com/hg/ #Patch Set 7 : diff -r b295b8bda20b https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 8 : diff -r b295b8bda20b https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 9 : diff -r b295b8bda20b https://go.googlecode.com/hg/ #Patch Set 10 : diff -r b295b8bda20b https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 11 : diff -r 9e53a1312e25 https://go.googlecode.com/hg/ #Patch Set 12 : diff -r 607e0f74161f https://go.googlecode.com/hg/ #MessagesTotal messages: 22
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
CL description first line should be runtime: add Semacquire/Semrelease benchmark http://codereview.appspot.com/4625065/diff/12001/src/pkg/runtime/sema_test.go File src/pkg/runtime/sema_test.go (right): http://codereview.appspot.com/4625065/diff/12001/src/pkg/runtime/sema_test.go... src/pkg/runtime/sema_test.go:49: var sem uint32 = 0 sem := uint32(0) or var sem uint32 http://codereview.appspot.com/4625065/diff/12001/src/pkg/runtime/sema_test.go... src/pkg/runtime/sema_test.go:74: c <- (foo == 42) drop () http://codereview.appspot.com/4625065/diff/12001/src/pkg/runtime/sema_test.go... src/pkg/runtime/sema_test.go:77: if block { This is weird. If block is true above the code executes procs/2 Semacquires. But here it executes procs Semreleases. Shouldn't those two counts be the same? I think you should be able to write something inside the if block { } such that the part currently in the else { } can run always (no else).
Sign in to reply to this message.
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2011/06/27 16:19:36, rsc wrote: > CL description first line should be > > runtime: add Semacquire/Semrelease benchmark Done. > http://codereview.appspot.com/4625065/diff/12001/src/pkg/runtime/sema_test.go > File src/pkg/runtime/sema_test.go (right): > > http://codereview.appspot.com/4625065/diff/12001/src/pkg/runtime/sema_test.go... > src/pkg/runtime/sema_test.go:49: var sem uint32 = 0 > sem := uint32(0) > or > var sem uint32 Done. > http://codereview.appspot.com/4625065/diff/12001/src/pkg/runtime/sema_test.go... > src/pkg/runtime/sema_test.go:74: c <- (foo == 42) > drop () Done. > http://codereview.appspot.com/4625065/diff/12001/src/pkg/runtime/sema_test.go... > src/pkg/runtime/sema_test.go:77: if block { > This is weird. If block is true above > the code executes procs/2 Semacquires. > But here it executes procs Semreleases. > Shouldn't those two counts be the same? > > I think you should be able to write > something inside the if block { } > such that the part currently in the else { } > can run always (no else). How about "c <- <-c"? :)
Sign in to reply to this message.
http://codereview.appspot.com/4625065/diff/8002/src/pkg/runtime/sema_test.go File src/pkg/runtime/sema_test.go (right): http://codereview.appspot.com/4625065/diff/8002/src/pkg/runtime/sema_test.go#... src/pkg/runtime/sema_test.go:77: if block { I now have no idea what's going on here. Maybe it would be clearer with two different channels? Or a WaitGroup?
Sign in to reply to this message.
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2011/06/27 17:53:19, rsc wrote: > http://codereview.appspot.com/4625065/diff/8002/src/pkg/runtime/sema_test.go > File src/pkg/runtime/sema_test.go (right): > > http://codereview.appspot.com/4625065/diff/8002/src/pkg/runtime/sema_test.go#... > src/pkg/runtime/sema_test.go:77: if block { > I now have no idea what's going on here. > Maybe it would be clearer with two different channels? > Or a WaitGroup? The logic itself it quite complicated. I've added some comments and split "c<-<-c" to two separate statements. Two channels most likely will deadlock, because there are corner cases (b.N=1) where worker goroutines won't unblock all helper goroutines.
Sign in to reply to this message.
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
You have two different conditions here. Please use two different channels. http://codereview.appspot.com/4625065/diff/14002/src/pkg/runtime/sema_test.go File src/pkg/runtime/sema_test.go (right): http://codereview.appspot.com/4625065/diff/14002/src/pkg/runtime/sema_test.go... src/pkg/runtime/sema_test.go:89: // Note that we do it only once at least one worker goroutine s/once/after/
Sign in to reply to this message.
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2011/06/27 18:20:32, rsc wrote: > You have two different conditions here. > Please use two different channels. I've added separate channel for helper goroutines and worked around the deadlock. I like this variant.
Sign in to reply to this message.
Much nicer.
Now the first if can be
if block {
sem = -procs/2
}
Sign in to reply to this message.
On 2011/06/27 18:44:04, rsc wrote:
> Much nicer.
>
> Now the first if can be
>
> if block {
> sem = -procs/2
> }
It would be nice. But I afraid it won't work as expected.
Sign in to reply to this message.
>> if block {
>> sem = -procs/2
>> }
>
> It would be nice. But I afraid it won't work as expected.
Why not?
Sign in to reply to this message.
>> It would be nice. But I afraid it won't work as expected. > > Why not? never mind; i forgot that they take a uint32 not an int32
Sign in to reply to this message.
LGTM http://codereview.appspot.com/4625065/diff/13002/src/pkg/runtime/sema_test.go File src/pkg/runtime/sema_test.go (right): http://codereview.appspot.com/4625065/diff/13002/src/pkg/runtime/sema_test.go... src/pkg/runtime/sema_test.go:8: "sync/atomic" move down one line (sort)
Sign in to reply to this message.
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=c63f30ac7770 *** runtime: add Semacquire/Semrelease benchmarks R=rsc CC=golang-dev http://codereview.appspot.com/4625065 Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.
|
