[golang-dev] code review 7002055: testing: add (*B).Allocs and AllocsPerRun (issue 7002055)

Hi Kyle,
We also have these tests about mallocs, I think they will all be benefit
from
AllocsPerRun.
pkg/math/big/nat_test.go
pkg/net/http/chunked_test.go
pkg/net/http/httputil/chunked_test.go
pkg/net/rpc/server_test.go
pkg/path/filepath/path_test.go
pkg/path/path_test.go
pkg/strconv/strconv_test.go
pkg/time/time_test.go

For this one, would you suggest adding a second return parameter from
Allocs(PerRun) that returns the average amount of memory allocated per
run? This would be the only invocation that would use it.
I've modified this one; it's less of a transliteration than the others,
so some scrutiny is probably merited to make sure it's testing what it
was originally intended to test.
Done.

https://codereview.appspot.com/7002055/

i suggest we also return AllocBytes.

instead of (*B).Allocs, i think we just need a way to enable
-test.benchmem for a single benchmark, then most of the
mallocs benchmarks could be simplified or eliminated
entirely. what do you think?


https://codereview.appspot.com/7002055/diff/9001/src/pkg/testing/allocs.go
File src/pkg/testing/allocs.go (right):

https://codereview.appspot.com/7002055/diff/9001/src/pkg/testing/allocs.go#newcode23
src/pkg/testing/allocs.go:23: func (b *B) Allocs(f func()) float64 {
do you want to set GOMAXPROCS to 1 here?
if the user uses Allocs directly.

https://codereview.appspot.com/7002055/diff/9001/src/pkg/testing/allocs.go#newcode25
src/pkg/testing/allocs.go:25: defer b.StartTimer()
i wonder if this function should keep the state of the timer?

https://codereview.appspot.com/7002055/

A lot of the code that's trying to test allocations has to perform some
amount of setup or preallocation in order to reduce the scope of each
run to encompass only the code that should be being measured.

There are a few (fmt, strconv) which use table driven tests wherein each
row has a different number of allowable allocs. A few more (path,
path/filepath) are table-driven and want zero allocs (these could be
separated away from the earlier parts of the test that do allocate and
so would work), but I suspect that it would be most helpful to know
precisely which of the paths caused Clean to allocate.

Adding the ability for a test (or benchmark) to specify that it wants a
memory profile to be printed out whenever it's run certainly sounds like
it would be useful, but I'm not sure if it addresses the cases here.


https://codereview.appspot.com/7002055/diff/9001/src/pkg/testing/allocs.go
File src/pkg/testing/allocs.go (right):

https://codereview.appspot.com/7002055/diff/9001/src/pkg/testing/allocs.go#newcode23
src/pkg/testing/allocs.go:23: func (b *B) Allocs(f func()) float64 {
Hmm. I was thinking that the test harness only runs benchmarks with
GOMAXPROCS=1, which now that I look at the code turns out to not be the
case.

Should I set/restore for both?

https://codereview.appspot.com/7002055/diff/9001/src/pkg/testing/allocs.go#newcode25
src/pkg/testing/allocs.go:25: defer b.StartTimer()
I wanted the timer to be as close as possible to what it would be if you
just called the function b.N times. The difference between calling the
code b.N times and running it through a closure via this method should
theoretically amortize out to be just that of the closure call (which,
depending on what you're measuring, could be statistically significant).
Is there a different approach that makes more sense?

https://codereview.appspot.com/7002055/

what if we mirror Start/StopTimer, and introduce Start/StopMallocsCount
(the name is just quick thought) just for benchmarks?

That would certainly be doable; the benchmark is already keeping track. It
seems best to do that in a separate CL though, as the changes at that point
would be largely orthogonal to this and it seems like a bigger expansion of
the exported API.

i think my proposal introduces too much flexibility as the code
benchmarked for time might not be the code benchmarked
for mallocs (i was worrying about the code after the main loop
introducing more mallocs).

maybe just a (*B).EnableMallocReport() suffice, and this can be
added in a new CL.

I just tried this out... I'm not sure I like it. I think I'd need to
make one argument or the other a uint64 instead of a float64 to make it
harder to confuse the return values. Another possibility would be to
return a struct with run statistics, so the code would read
testing.AllocsPerRun(N, f).Count or similar. I'm open to suggestions.
Oh, I just re-read your suggestion; not sure exactly what I saw the
first time around. I'll take a look at the uses of b.Allocs tomorrow; I
think you may be right.

https://codereview.appspot.com/7002055/

I did some more experimenting today.

I haven't figured out a way to return both the count and the average
bytes with an API that I like unless I make two separate functions. The
main reason is that there are a dozen instances where we are caring
about the number of allocations and only one in which we care about the
number of bytes, and it's a regression test about a very specific bug.
It looks like I'm discarding an error if I make the byte size the second
argument, and it seems strange to always discard the first argument as
it should be the more important one. For what it's worth, I'd prefer to
leave it the way it is, and copy/paste AllocsPerRun with the requisite
changes in the one place it's needed; if it turns out to be a common
test in the future, it could be promoted to the testing package.

I also tried getting rid of b.Allocs. This seems the right way to go,
as benchmem reports identical data to what Allocs reports:
BenchmarkParser 500 4875246 ns/op 16.03 MB/s 591554 B/op
7995 allocs/op
--- BENCH: BenchmarkParser
parse_test.go:390: 1 iterations, 8099 mallocs per iteration
parse_test.go:390: 100 iterations, 8053.72 mallocs per iteration
parse_test.go:390: 500 iterations, 7995.888 mallocs per iteration
BenchmarkRawLevelTokenizer 5000 734455 ns/op 106.42 MB/s
4957 B/op 12 allocs/op
--- BENCH: BenchmarkRawLevelTokenizer
token_test.go:675: 1 iterations, 12 mallocs per iteration
token_test.go:675: 100 iterations, 12.01 mallocs per iteration
token_test.go:675: 5000 iterations, 12.0296 mallocs per iteration
BenchmarkLowLevelTokenizer 2000 1001182 ns/op 78.07 MB/s
5060 B/op 25 allocs/op
--- BENCH: BenchmarkLowLevelTokenizer
token_test.go:675: 1 iterations, 25 mallocs per iteration
token_test.go:675: 100 iterations, 25.01 mallocs per iteration
token_test.go:675: 2000 iterations, 25.029 mallocs per iteration
BenchmarkHighLevelTokenizer 1000 1636230 ns/op 47.77 MB/s
103299 B/op 3221 allocs/op
--- BENCH: BenchmarkHighLevelTokenizer
token_test.go:675: 1 iterations, 3223 mallocs per iteration
token_test.go:675: 100 iterations, 3221.4 mallocs per iteration
token_test.go:675: 1000 iterations, 3221.277 mallocs per iteration

I think I agree that an EnableMallocReport-like API will be sufficient
for the benchmarks. Only one (the parser benchmark in exp/html) seems
to do significant allocation outside the loop (reading in the text
file), and that can probably be done in an init() that is then used by
the benchmark.

https://codereview.appspot.com/7002055/

As we already stop the malloc counts when timer is stopped, i think this
is a non-issue now.

I created https://codereview.appspot.com/7027046 for this proposal
and converted the exp/html benchmarks to use the new API.

Why does the description mention (*B).Allocs? I don't see that anywhere.



https://codereview.appspot.com/7002055/diff/15001/src/pkg/exp/html/token_test.go
File src/pkg/exp/html/token_test.go (right):

https://codereview.appspot.com/7002055/diff/15001/src/pkg/exp/html/token_test.go#newcode631
src/pkg/exp/html/token_test.go:631: // TODO(kevlar): Enable
-test.benchmem for these benchmarks
What does this mean?

https://codereview.appspot.com/7002055/

anywhere.

Per minux's suggestion, instead of having two functions (AllocsPerRun
and (*B).Allocs) separately for testing and benchmarking, I removed
(*B).Allocs and have marked where it was used with the TODO you mention
below. I'll update the description.

The malloc-counting loops that currently exist in the stdlib benchmarks
print out results that are essentially the same as what is printed when
you benchmark with -test.benchmem enabled, so the suggestion there was
to provide (in another CL, so if we decide this is the route to go I'll
file an issue and mention it from the TODOs) a (*B) method which enables
that extra information to be printed for the benchmark within which it
is called (vs test.benchmem. which enables it globally), similar to how
calling (*B).SetBytes causes MB/s to be printed.


https://codereview.appspot.com/7002055/diff/15001/src/pkg/exp/html/token_test.go

https://codereview.appspot.com/7002055/diff/15001/src/pkg/exp/html/token_test.go#newcode631


https://codereview.appspot.com/7002055/