code review 154173: Adds benchmark support to gotest.

Adds benchmark support to gotest.

No benchmarks are run unless the --benchmarks=<regexp> flag
is specified on the gotest command line.  This change includes
sample benchmarks for regexp.

% gotest --benchmarks=.*
(standard test output redacted)
testing.BenchmarkSimpleMatch	200000	      7799 ns/op
testing.BenchmarkUngroupedMatch	20000	     76898 ns/op
testing.BenchmarkGroupedMatch	50000	     38148 ns/op

[trevor.strohman, 2009-11-17 06:01:59]

Hello r, rsc,

I'd like you to review the following change.

[rsc, 2009-11-17 07:34:43]

Thanks for doing this.  I like the approach.

Please hg upload and reply on the web form
(http://codereview.appspot.com/154173/publish)
once it's ready for another look.

http://codereview.appspot.com/154173/diff/1005/6
File src/cmd/gotest/gotest (right):

http://codereview.appspot.com/154173/diff/1005/6#newcode121
src/cmd/gotest/gotest:121: # benchmarks are named BenchFoo.
I appreciate the shorthand but everything else
says benchmark not bench.  this should probably
be BenchmarkFoo.

http://codereview.appspot.com/154173/diff/1005/7
File src/pkg/testing/Makefile (right):

http://codereview.appspot.com/154173/diff/1005/7#newcode11
src/pkg/testing/Makefile:11: benchmark.go
add a \ and sort the list

http://codereview.appspot.com/154173/diff/1005/8
File src/pkg/testing/benchmark.go (right):

http://codereview.appspot.com/154173/diff/1005/8#newcode17
src/pkg/testing/benchmark.go:17: // Describes a benchmark to run.  This
structure is usually created
Use the same phrasing as for the Test struct comment.

http://codereview.appspot.com/154173/diff/1005/8#newcode21
src/pkg/testing/benchmark.go:21: Function	func(b *B);
F func(*B)

(be like Test)

http://codereview.appspot.com/154173/diff/1005/8#newcode24
src/pkg/testing/benchmark.go:24: // A structure providing support for a single
benchmark run,
A doc comment should start with a complete sentence.
You can copy the phrasing from the type T struct.

// B is a type passed to Benchmark functions ...

http://codereview.appspot.com/154173/diff/1005/8#newcode27
src/pkg/testing/benchmark.go:27: Iterations	int;
Iterations seems very long.  It would be in 
keeping with the rest of this package to use N int;

http://codereview.appspot.com/154173/diff/1005/8#newcode28
src/pkg/testing/benchmark.go:28: nanoseconds	int64;
s/nanoseconds/ns/

http://codereview.appspot.com/154173/diff/1005/8#newcode32
src/pkg/testing/benchmark.go:32: // Start timing the test.  This method has
already been
Same doc comment comment applies to the rest of this file.
See http://golang.org/doc/effective_go.html#commentary

http://codereview.appspot.com/154173/diff/1005/8#newcode43
src/pkg/testing/benchmark.go:43: b.start = -1;
Why is the test above b.start > 0 but this uses -1?
They should be consistent.  0 seems fine.

http://codereview.appspot.com/154173/diff/1005/8#newcode48
src/pkg/testing/benchmark.go:48: b.start = -1;
Same comment about -1 vs 0.

http://codereview.appspot.com/154173/diff/1005/8#newcode54
src/pkg/testing/benchmark.go:54: return 1
0 seems like a better answer.

http://codereview.appspot.com/154173/diff/1005/8#newcode60
src/pkg/testing/benchmark.go:60: fmt.Printf("%40s %-10s %10s\n", "Benchmark",
"Iterations", "ns/Iter");
Please avoid the assumption of fixed sizes, right alignment, and ASCII art.
(Not everyone has a vt100 emulator handy.)

I'd just get rid of the whole header, actually.

http://codereview.appspot.com/154173/diff/1005/8#newcode64
src/pkg/testing/benchmark.go:64: func printBenchmark(benchmark Benchmark, b *B)
{
This can be rolled into runBenchmarkIterations,
which itself can be a method run on b.

http://codereview.appspot.com/154173/diff/1005/8#newcode65
src/pkg/testing/benchmark.go:65: fmt.Printf("%40s %10d %10d\n", benchmark.Name,
b.Iterations, b.nsPerIter())
fmt.Printf("%s\t%d\t%d ns/op\n", bm.Name, bm.N, bm.nsPerOp);

http://codereview.appspot.com/154173/diff/1005/8#newcode69
src/pkg/testing/benchmark.go:69: func runBenchmarkIterations(n int, benchmark
Benchmark, b *B) {
func (b *B) runN(n int)

http://codereview.appspot.com/154173/diff/1005/8#newcode77
src/pkg/testing/benchmark.go:77: func min(one, two int) int {
More traditional names would be x,y.

http://codereview.appspot.com/154173/diff/1005/8#newcode78
src/pkg/testing/benchmark.go:78: if one > two {
... because this reads very strangely.

http://codereview.appspot.com/154173/diff/1005/8#newcode86
src/pkg/testing/benchmark.go:86: log := math.Log10(float64(n));
I don't think the variables are helping:

return int(math.Pow10(int(math.Log10(float64(n)))));

http://codereview.appspot.com/154173/diff/1005/8#newcode105
src/pkg/testing/benchmark.go:105: func runBenchmark(benchmark Benchmark, b *B) {
func (b *B) run()

http://codereview.appspot.com/154173/diff/1005/8#newcode107
src/pkg/testing/benchmark.go:107: iters := 1;
s/iters/n/

http://codereview.appspot.com/154173/diff/1005/8#newcode146
src/pkg/testing/benchmark.go:146: b := new(B);
I suggest adding benchmark Benchmark; to B and then using

    b := &B{benchmark: Benchmark}
    b.run();

[r, 2009-11-17 07:36:34]

You need to update doc.go as well.

[trevor.strohman, 2009-11-17 09:30:12]

Thanks for the comments.

I added a package comment in testing.go and took some public documentation out
of benchmark.go (since those functions/types are implementation details of
gotest).  I've also updated the change description with the new tool output.

http://codereview.appspot.com/154173/diff/1005/6
File src/cmd/gotest/gotest (right):

http://codereview.appspot.com/154173/diff/1005/6#newcode121
src/cmd/gotest/gotest:121: # benchmarks are named BenchFoo.
On 2009/11/17 07:34:43, rsc wrote:
> I appreciate the shorthand but everything else
> says benchmark not bench.  this should probably
> be BenchmarkFoo.
> 
> 

Done.

http://codereview.appspot.com/154173/diff/1005/7
File src/pkg/testing/Makefile (right):

http://codereview.appspot.com/154173/diff/1005/7#newcode11
src/pkg/testing/Makefile:11: benchmark.go
On 2009/11/17 07:34:43, rsc wrote:
> add a \ and sort the list
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8
File src/pkg/testing/benchmark.go (right):

http://codereview.appspot.com/154173/diff/1005/8#newcode17
src/pkg/testing/benchmark.go:17: // Describes a benchmark to run.  This
structure is usually created
On 2009/11/17 07:34:43, rsc wrote:
> Use the same phrasing as for the Test struct comment.
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode21
src/pkg/testing/benchmark.go:21: Function	func(b *B);
On 2009/11/17 07:34:43, rsc wrote:
> F func(*B)
> 
> (be like Test)
> 
> 
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode24
src/pkg/testing/benchmark.go:24: // A structure providing support for a single
benchmark run,
On 2009/11/17 07:34:43, rsc wrote:
> A doc comment should start with a complete sentence.
> You can copy the phrasing from the type T struct.
> 
> // B is a type passed to Benchmark functions ...
> 

The new text is stolen from testing.go.

http://codereview.appspot.com/154173/diff/1005/8#newcode27
src/pkg/testing/benchmark.go:27: Iterations	int;
On 2009/11/17 07:34:43, rsc wrote:
> Iterations seems very long.  It would be in 
> keeping with the rest of this package to use N int;
> 
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode28
src/pkg/testing/benchmark.go:28: nanoseconds	int64;
On 2009/11/17 07:34:43, rsc wrote:
> s/nanoseconds/ns/
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode32
src/pkg/testing/benchmark.go:32: // Start timing the test.  This method has
already been
On 2009/11/17 07:34:43, rsc wrote:
> Same doc comment comment applies to the rest of this file.
> See http://golang.org/doc/effective_go.html#commentary
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode43
src/pkg/testing/benchmark.go:43: b.start = -1;
On 2009/11/17 07:34:43, rsc wrote:
> Why is the test above b.start > 0 but this uses -1?
> They should be consistent.  0 seems fine.
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode48
src/pkg/testing/benchmark.go:48: b.start = -1;
On 2009/11/17 07:34:43, rsc wrote:
> Same comment about -1 vs 0.
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode54
src/pkg/testing/benchmark.go:54: return 1
On 2009/11/17 07:34:43, rsc wrote:
> 0 seems like a better answer.

Returning 1 here made the runBenchmark loop cleaner (it avoids a division by
zero), but that's cheating.  I've made it 0.

http://codereview.appspot.com/154173/diff/1005/8#newcode60
src/pkg/testing/benchmark.go:60: fmt.Printf("%40s %-10s %10s\n", "Benchmark",
"Iterations", "ns/Iter");
On 2009/11/17 07:34:43, rsc wrote:
> Please avoid the assumption of fixed sizes, right alignment, and ASCII art.
> (Not everyone has a vt100 emulator handy.)
> 
> I'd just get rid of the whole header, actually.

Removed.

http://codereview.appspot.com/154173/diff/1005/8#newcode64
src/pkg/testing/benchmark.go:64: func printBenchmark(benchmark Benchmark, b *B)
{
On 2009/11/17 07:34:43, rsc wrote:
> This can be rolled into runBenchmarkIterations,
> which itself can be a method run on b.
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode65
src/pkg/testing/benchmark.go:65: fmt.Printf("%40s %10d %10d\n", benchmark.Name,
b.Iterations, b.nsPerIter())
On 2009/11/17 07:34:43, rsc wrote:
> fmt.Printf("%s\t%d\t%d ns/op\n", bm.Name, bm.N, bm.nsPerOp);
> 

Done.  (Note that new location is in run()).

http://codereview.appspot.com/154173/diff/1005/8#newcode69
src/pkg/testing/benchmark.go:69: func runBenchmarkIterations(n int, benchmark
Benchmark, b *B) {
On 2009/11/17 07:34:43, rsc wrote:
> func (b *B) runN(n int)
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode77
src/pkg/testing/benchmark.go:77: func min(one, two int) int {
On 2009/11/17 07:34:43, rsc wrote:
> More traditional names would be x,y.
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode86
src/pkg/testing/benchmark.go:86: log := math.Log10(float64(n));
On 2009/11/17 07:34:43, rsc wrote:
> I don't think the variables are helping:
> 
> return int(math.Pow10(int(math.Log10(float64(n)))));
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode105
src/pkg/testing/benchmark.go:105: func runBenchmark(benchmark Benchmark, b *B) {
On 2009/11/17 07:34:43, rsc wrote:
> func (b *B) run()
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode107
src/pkg/testing/benchmark.go:107: iters := 1;
On 2009/11/17 07:34:43, rsc wrote:
> s/iters/n/
> 

Done.

http://codereview.appspot.com/154173/diff/1005/8#newcode146
src/pkg/testing/benchmark.go:146: b := new(B);
On 2009/11/17 07:34:43, rsc wrote:
> I suggest adding benchmark Benchmark; to B and then using
> 
>     b := &B{benchmark: Benchmark}
>     b.run();
> 
> 

Very clean, thanks.  Done.

[rsc, 2009-11-18 07:16:49]

looks good to me.  thanks again.
will leave for r to LGTM and submit

http://codereview.appspot.com/154173/diff/2007/22
File src/cmd/gotest/gotest (right):

http://codereview.appspot.com/154173/diff/2007/22#newcode121
src/cmd/gotest/gotest:121: # benchmarks are named BenchmarkFoo.
looks like you are using spaces here instead of a tab

http://codereview.appspot.com/154173/diff/2007/24
File src/pkg/testing/benchmark.go (right):

http://codereview.appspot.com/154173/diff/2007/24#newcode61
src/pkg/testing/benchmark.go:61: // runN runs a single benchmark for the
specified number of iterations (n).
can drop (n)

http://codereview.appspot.com/154173/diff/2007/26
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/154173/diff/2007/26#newcode23
src/pkg/testing/testing.go:23: // The benchmark package will vary b.N to until
the benchmark function lasts
s/to //

[trevor.strohman, 2009-11-18 16:54:50]

I ran ./all.bash and "cd src/pkg/testing; gotest --benchmarks=.*" again too.

http://codereview.appspot.com/154173/diff/2007/22
File src/cmd/gotest/gotest (right):

http://codereview.appspot.com/154173/diff/2007/22#newcode121
src/cmd/gotest/gotest:121: # benchmarks are named BenchmarkFoo.
On 2009/11/18 07:16:49, rsc wrote:
> looks like you are using spaces here instead of a tab
> 

Done.

http://codereview.appspot.com/154173/diff/2007/24
File src/pkg/testing/benchmark.go (right):

http://codereview.appspot.com/154173/diff/2007/24#newcode61
src/pkg/testing/benchmark.go:61: // runN runs a single benchmark for the
specified number of iterations (n).
On 2009/11/18 07:16:49, rsc wrote:
> can drop (n)
> 

Done.

http://codereview.appspot.com/154173/diff/2007/26
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/154173/diff/2007/26#newcode23
src/pkg/testing/testing.go:23: // The benchmark package will vary b.N to until
the benchmark function lasts
On 2009/11/18 07:16:49, rsc wrote:
> s/to //
> 

Done.

[r, 2009-11-18 18:56:07]

i really like this change but it has some unpleasant but short-lived
implications.

http://codereview.appspot.com/154173/diff/3004/2013
File src/pkg/testing/benchmark.go (right):

http://codereview.appspot.com/154173/diff/3004/2013#newcode12
src/pkg/testing/benchmark.go:12: "time";
there are new dependencies here, which means:
1) Make.deps needs updating
2) more tests must be moved into the testing clique
3) this is annoying
is it avoidable?

for instance: math is absolutely unnecessary here; it's easy to do a simple
pow10 yourself.
time is of course unavoidable but you'll have to fix the timing test so it's not
in package time.  see e.g. fmt_test.go to find the solution: import . "time"

sorry.

http://codereview.appspot.com/154173/diff/3004/2015
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/154173/diff/3004/2015#newcode15
src/pkg/testing/testing.go:15: // are considered benchmarks, and are executed by
gotest when the --benchmarks=.*
drop the =.* thing (--benchmarks flag is fine).
document this flag in src/cmd/gotest/doc.go

http://codereview.appspot.com/154173/diff/3004/2015#newcode24
src/pkg/testing/testing.go:24: // long enough to time reliably.  The output
s/time/be timed/

[trevor.strohman, 2009-11-18 19:42:41]

http://codereview.appspot.com/154173/diff/3004/2013
File src/pkg/testing/benchmark.go (right):

http://codereview.appspot.com/154173/diff/3004/2013#newcode12
src/pkg/testing/benchmark.go:12: "time";
On 2009/11/18 18:56:07, r wrote:
> there are new dependencies here, which means:
> 1) Make.deps needs updating

Done.

> 2) more tests must be moved into the testing clique
> 3) this is annoying
> is it avoidable?
> 
> for instance: math is absolutely unnecessary here; it's easy to do a simple
> pow10 yourself.

Done.  The code looks cleaner too.

> time is of course unavoidable but you'll have to fix the timing test so it's
not
> in package time.  see e.g. fmt_test.go to find the solution: import . "time"

Apparently it was already moved.  It looks like Russ moved time_test.go and
tick_test.go out of the time package in change 5c14f2f476.

http://codereview.appspot.com/154173/diff/3004/2015
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/154173/diff/3004/2015#newcode15
src/pkg/testing/testing.go:15: // are considered benchmarks, and are executed by
gotest when the --benchmarks=.*
On 2009/11/18 18:56:07, r wrote:
> drop the =.* thing (--benchmarks flag is fine).
> document this flag in src/cmd/gotest/doc.go

Done.

http://codereview.appspot.com/154173/diff/3004/2015#newcode24
src/pkg/testing/testing.go:24: // long enough to time reliably.  The output
On 2009/11/18 18:56:07, r wrote:
> s/time/be timed/

Done.

[r, 2009-11-18 19:47:32]

we're down to comment trivia.

http://codereview.appspot.com/154173/diff/3018/2027
File src/cmd/gotest/doc.go (right):

http://codereview.appspot.com/154173/diff/3018/2027#newcode22
src/cmd/gotest/doc.go:22: Benchmarks should have signature
Before this:

Benchmark functions can be written as well; they will be run only when the
-benchmarks flag is provided.

http://codereview.appspot.com/154173/diff/3018/2027#newcode43
src/cmd/gotest/doc.go:43: 6.out [-v] [-match pattern] [-benchmarks]
-benchmarks pattern

http://codereview.appspot.com/154173/diff/3018/2027#newcode45
src/cmd/gotest/doc.go:45: The -v flag causes the tests to be logged as they run.
 The --match
s/--/-/
let's start being consistent.

http://codereview.appspot.com/154173/diff/3018/2027#newcode50
src/cmd/gotest/doc.go:50: code.  Benchmarks are only run if the -benchmarks flag
is given.
this sentence isn't right.
The -benchmarks flag is analogous to the -match flag, but applies to benchmarks.

http://codereview.appspot.com/154173/diff/3018/2033
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/154173/diff/3018/2033#newcode16
src/pkg/testing/testing.go:16: // flag is specified.
s/specified/provided/ or given

also add a blank line here

[trevor.strohman, 2009-11-18 19:57:03]

http://codereview.appspot.com/154173/diff/3018/2027
File src/cmd/gotest/doc.go (right):

http://codereview.appspot.com/154173/diff/3018/2027#newcode43
src/cmd/gotest/doc.go:43: 6.out [-v] [-match pattern] [-benchmarks]
On 2009/11/18 19:47:32, r wrote:
> -benchmarks pattern

Doesn't this conflict with your previous comment?  I just changed -benchmarks to
bool so that users could specify just -benchmarks, not -benchmarks=.*   If
-benchmarks is a pattern, we don't get that behavior.

[trevor.strohman, 2009-11-18 21:51:15]

Changed -benchmarks back to a pattern.

Re-ran ./deps.bash; ./clean.bash; ./all.bash; cd pkg/testing; gotest
--benchmarks=.*

http://codereview.appspot.com/154173/diff/3018/2027
File src/cmd/gotest/doc.go (right):

http://codereview.appspot.com/154173/diff/3018/2027#newcode22
src/cmd/gotest/doc.go:22: Benchmarks should have signature
On 2009/11/18 19:47:32, r wrote:
> Before this:
> 
> Benchmark functions can be written as well; they will be run only when the
> -benchmarks flag is provided.

Done.

http://codereview.appspot.com/154173/diff/3018/2027#newcode43
src/cmd/gotest/doc.go:43: 6.out [-v] [-match pattern] [-benchmarks]
On 2009/11/18 19:47:32, r wrote:
> -benchmarks pattern

Done.

http://codereview.appspot.com/154173/diff/3018/2027#newcode45
src/cmd/gotest/doc.go:45: The -v flag causes the tests to be logged as they run.
 The --match
On 2009/11/18 19:47:32, r wrote:
> s/--/-/
> let's start being consistent.

Done.

http://codereview.appspot.com/154173/diff/3018/2027#newcode50
src/cmd/gotest/doc.go:50: code.  Benchmarks are only run if the -benchmarks flag
is given.
On 2009/11/18 19:47:32, r wrote:
> this sentence isn't right.
> The -benchmarks flag is analogous to the -match flag, but applies to
benchmarks.

I changed it to:

The -benchmarks flag is analogous to the -match flag, but
applies to benchmarks.  No benchmarks run by default.

http://codereview.appspot.com/154173/diff/3018/2033
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/154173/diff/3018/2033#newcode16
src/pkg/testing/testing.go:16: // flag is specified.
On 2009/11/18 19:47:32, r wrote:
> s/specified/provided/ or given
> 
> also add a blank line here

Done, also changed s/--benchmarks/-benchmarks/ for consistency.

[r, 2009-11-18 21:54:38]

one keystroke away

http://codereview.appspot.com/154173/diff/3030/2039
File src/cmd/gotest/doc.go (right):

http://codereview.appspot.com/154173/diff/3030/2039#newcode22
src/cmd/gotest/doc.go:22: Benchmark functions can be written as well; they will
be run only when the
break the line please to be consistent with the rest of the file.

[rsc, 2009-11-19 23:27:54]

On 2009/11/18 21:54:38, r wrote:
> one keystroke away
> 
> http://codereview.appspot.com/154173/diff/3030/2039
> File src/cmd/gotest/doc.go (right):
> 
> http://codereview.appspot.com/154173/diff/3030/2039#newcode22
> src/cmd/gotest/doc.go:22: Benchmark functions can be written as well; they
will
> be run only when the
> break the line please to be consistent with the rest of the file.

In addition, please do
hg file -d 154173 Make.deps   # cut Make.deps from this CL
hg upload 154173

We can re-run deps.bash before submitting; I am worried
that if some other dep gets introduced elsewhere,
the patch won't apply to Make.deps cleanly, which will
make it hard to patch the whole CL in.

Thanks.  I'm looking forward to using this.

[trevor.strohman, 2009-11-19 23:42:55]

Also:

- took out Make.deps as Russ requested
- ran ./all.bash
- ran gotest --benchmarks=.* in src/pkg/testing

http://codereview.appspot.com/154173/diff/3030/2039
File src/cmd/gotest/doc.go (right):

http://codereview.appspot.com/154173/diff/3030/2039#newcode22
src/cmd/gotest/doc.go:22: Benchmark functions can be written as well; they will
be run only when the
On 2009/11/18 21:54:38, r wrote:
> break the line please to be consistent with the rest of the file.

Done.

[rsc, 2009-11-20 00:35:36]

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

Adds benchmark support to gotest.

No benchmarks are run unless the --benchmarks=<regexp> flag
is specified on the gotest command line.  This change includes
sample benchmarks for regexp.

% gotest --benchmarks=.*
(standard test output redacted)
testing.BenchmarkSimpleMatch	200000	      7799 ns/op
testing.BenchmarkUngroupedMatch	20000	     76898 ns/op
testing.BenchmarkGroupedMatch	50000	     38148 ns/op

R=r, rsc
http://codereview.appspot.com/154173

Committer: Russ Cox <rsc@golang.org>