code review 68030043: testing: diagnose a potential misuse of RunParallel

testing: diagnose a potential misuse of RunParallel

[dvyukov, 2014-02-24 09:06:13]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[dvyukov, 2014-02-24 16:22:41]

I've changed :
	if n < uint64(b.N) {
to:
	if n == 0 {
because a benchmark can exit early due to failures.
Strictly saying, n == 0 is also possible if all worker goroutines exit due to
failures before first pb.Next call. But I think it's still useful diagnostic.

[bradfitz, 2014-02-24 16:30:51]

LGTM

https://codereview.appspot.com/68030043/diff/80001/src/pkg/testing/benchmark.go
File src/pkg/testing/benchmark.go (right):

https://codereview.appspot.com/68030043/diff/80001/src/pkg/testing/benchmark....
src/pkg/testing/benchmark.go:421: b.Fatal("RunParallel body does not call
PB.Next")
s/does not/did not/

[dvyukov, 2014-02-24 16:32:08]

https://codereview.appspot.com/68030043/diff/80001/src/pkg/testing/benchmark.go
File src/pkg/testing/benchmark.go (right):

https://codereview.appspot.com/68030043/diff/80001/src/pkg/testing/benchmark....
src/pkg/testing/benchmark.go:421: b.Fatal("RunParallel body does not call
PB.Next")
On 2014/02/24 16:30:51, bradfitz wrote:
> s/does not/did not/

Done.

[dvyukov, 2014-02-24 16:32:33]

*** Submitted as https://code.google.com/p/go/source/detail?r=daff95a797cc ***

testing: diagnose a potential misuse of RunParallel

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/68030043

[rsc, 2014-02-24 16:52:02]

https://codereview.appspot.com/68030043/diff/120001/src/pkg/testing/benchmark.go
File src/pkg/testing/benchmark.go (right):

https://codereview.appspot.com/68030043/diff/120001/src/pkg/testing/benchmark...
src/pkg/testing/benchmark.go:420: if n == 0 {
Seems like this can be strengthened.

if n < b.N && !b.Failed() {
    b.Fatal("RunParallel body did not complete execution")
}

[dvyukov, 2014-02-24 16:57:04]

https://codereview.appspot.com/68030043/diff/120001/src/pkg/testing/benchmark.go
File src/pkg/testing/benchmark.go (right):

https://codereview.appspot.com/68030043/diff/120001/src/pkg/testing/benchmark...
src/pkg/testing/benchmark.go:420: if n == 0 {
On 2014/02/24 16:52:02, rsc wrote:
> Seems like this can be strengthened.
> 
> if n < b.N && !b.Failed() {

agree

>     b.Fatal("RunParallel body did not complete execution")
> }

this may be non-actionable for newcomers

to be explicit this is to protect against:
b.RunParallel(func(pb *testing.PB) {
  doSomething()
})

can you please suggest a rephrasing that mentions PB.Next (maybe an addition in
brackets)?

[rsc, 2014-02-24 16:59:23]

b.Fatal("RunParallel: body exited without PB.Next() == false")

[rsc, 2014-02-24 16:59:50]

s/PB/pb/

[gobot, 2014-02-24 22:50:47]

This CL appears to have broken the darwin-386 builder.