code review 7002055: testing: add AllocsPerRun

testing: add AllocsPerRun This CL also replaces similar loops in other stdlib package tests with calls to AllocsPerRun. Fixes issue 4461.

[kevlar, 2012-12-23 02:20:46]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to http://code.google.com/p/go

[minux1, 2012-12-23 13:53:56]

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 

[kevlar, 2012-12-24 01:00:49]

> Hi Kyle, > We also have these tests about mallocs, I think they will all be benefit > from > AllocsPerRun. > pkg/math/big/nat_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. > pkg/net/http/chunked_test.go 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. > pkg/net/http/httputil/chunked_test.go ... hmm. Looks familiar ;-) ... > pkg/net/rpc/server_test.go Done. > pkg/path/filepath/path_test.go Done. > pkg/path/path_test.go Done. Also familiar looking ;-). > pkg/strconv/strconv_test.go Done. > pkg/time/time_test.go Done.

[minux1, 2012-12-24 07:12:36]

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#ne... 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#ne... src/pkg/testing/allocs.go:25: defer b.StartTimer() i wonder if this function should keep the state of the timer?

[kevlar, 2012-12-24 07:37:52]

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#ne... src/pkg/testing/allocs.go:23: func (b *B) Allocs(f func()) float64 { On 2012/12/24 07:12:36, minux wrote: > do you want to set GOMAXPROCS to 1 here? > if the user uses Allocs directly. 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#ne... src/pkg/testing/allocs.go:25: defer b.StartTimer() On 2012/12/24 07:12:36, minux wrote: > i wonder if this function should keep the state of the timer? 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?

[minux1, 2012-12-24 07:47:41]

On Monday, December 24, 2012, kevlar wrote: > 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. yes, so imo tests should use AllocsPerRun. > > > 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. > what if we mirror Start/StopTimer, and introduce Start/StopMallocsCount (the name is just quick thought) just for benchmarks? 

[kevlar, 2012-12-24 07:57:07]

On 2012/12/24 07:12:36, minux wrote: > i suggest we also return AllocBytes. 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. > 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? 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.

[kevlar, 2012-12-24 08:02:57]

On Mon, Dec 24, 2012 at 2:47 AM, minux <minux.ma@gmail.com> wrote: > On Monday, December 24, 2012, kevlar wrote: > > 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. > > yes, so imo tests should use AllocsPerRun. > >> >> 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. >> > 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. 

[minux1, 2012-12-24 08:39:30]

On Monday, December 24, 2012, Kyle Lemons wrote: > On Mon, Dec 24, 2012 at 2:47 AM, minux <minux.ma@gmail.com<javascript:_e({}, 'cvml', 'minux.ma@gmail.com');> > > wrote: > >> On Monday, December 24, 2012, kevlar wrote: >> >>> 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. >> >> yes, so imo tests should use AllocsPerRun. >> >>> >>> 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. >>> >> 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. 

[kevlar, 2012-12-25 07:57:17]

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.

[rsc, 2012-12-29 19:39:53]

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... src/pkg/exp/html/token_test.go:631: // TODO(kevlar): Enable -test.benchmem for these benchmarks What does this mean?

[kevlar, 2012-12-30 00:50:14]

On 2012/12/29 19:39:53, rsc wrote: > Why does the description mention (*B).Allocs? I don't see that 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 > File src/pkg/exp/html/token_test.go (right): > > https://codereview.appspot.com/7002055/diff/15001/src/pkg/exp/html/token_test... > src/pkg/exp/html/token_test.go:631: // TODO(kevlar): Enable -test.benchmem for > these benchmarks > What does this mean?

[minux1, 2012-12-30 20:01:26]

On Tue, Dec 25, 2012 at 3:57 PM, <kevlar@google.com> wrote: > 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. > 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. 

[kevlar, 2013-01-25 08:48:50]

PTAL There are two alloc-counting tests that didn't convert cleanly: "bytes" TestGrow - seems to be testing more than just the allocation "math/big" TestMulUnbalanced - the only instance in which the size is utilized, so it doesn't seem to justify the added API weight. It's also only used in a regression test to exclude runaway memory, whereas most AllocsPerRun instances are trying to ensure known, efficient allocation behavior.

[minux1, 2013-01-25 13:25:10]

https://codereview.appspot.com/7002055/diff/26001/api/next.txt File api/next.txt (right): https://codereview.appspot.com/7002055/diff/26001/api/next.txt#newcode1148 api/next.txt:1148: pkg testing, func AllocsPerRun(int, func()) float64 please don't update api/next.txt with the same CL that introduce the API. This will help to reduce merge conflicts when we latter cherry-pick the changesets. https://codereview.appspot.com/7002055/diff/26001/src/pkg/testing/allocs.go File src/pkg/testing/allocs.go (right): https://codereview.appspot.com/7002055/diff/26001/src/pkg/testing/allocs.go#n... src/pkg/testing/allocs.go:22: memstats := new(runtime.MemStats) no need to allocate, just declare a new variable. var memstats runtime.MemStats

[kevlar, 2013-01-26 01:16:36]

PTAL On 2013/01/25 13:25:10, minux wrote: > https://codereview.appspot.com/7002055/diff/26001/api/next.txt > File api/next.txt (right): > > https://codereview.appspot.com/7002055/diff/26001/api/next.txt#newcode1148 > api/next.txt:1148: pkg testing, func AllocsPerRun(int, func()) float64 > please don't update api/next.txt with the same CL > that introduce the API. > This will help to reduce merge conflicts when we > latter cherry-pick the changesets. Ah, that makes sense. I think I knew that but forgot... > https://codereview.appspot.com/7002055/diff/26001/src/pkg/testing/allocs.go > File src/pkg/testing/allocs.go (right): > > https://codereview.appspot.com/7002055/diff/26001/src/pkg/testing/allocs.go#n... > src/pkg/testing/allocs.go:22: memstats := new(runtime.MemStats) > no need to allocate, just declare a new variable. > var memstats runtime.MemStats Done.

[minux1, 2013-01-26 01:25:03]

LGTM.

[rsc, 2013-01-30 17:55:41]

Thanks for working on this. It's definitely an improvement in some cases, but not in all. It's okay to leave the ones that don't get better alone. It's also okay to write a constant like 100 in one place instead of introducing a name for it. It's also okay not to write every test comparison using variables. In both cases the additional indirections do obscure the code a bit, so they have to be justified by making something else clearer. In many of the cases I've flagged, there is no such justification, so they should go. Thanks. https://codereview.appspot.com/7002055/diff/30001/src/pkg/math/big/nat_test.go File src/pkg/math/big/nat_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/math/big/nat_test.g... src/pkg/math/big/nat_test.go:169: func allocSizePerRun(runs int, f func()) float64 { I don't see any reason to change this file, and the change makes the test 100x slower. Please revert. https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/chunked_te... File src/pkg/net/http/chunked_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/chunked_te... src/pkg/net/http/chunked_test.go:59: allocs := testing.AllocsPerRun(N, func() { This file is not noticeably better. Please revert. https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/header_tes... File src/pkg/net/http/header_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/header_tes... src/pkg/net/http/header_test.go:179: for i, f := 0, headerWriteSubsetFunc(); i < b.N; i++ { var testHeader = Header{ "Content-Length": {"123"}, "Content-Type": {"text/plain"}, "Date": {"some date at some time Z"}, "Server": {"Go http package"}, } var buf bytes.Buffer func BenchmarkHeaderWriteSubset(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { buf.Reset() testHeader.WriteSubset(&buf, nil) } } func TestHeaderWriteSubsetMallocs(t *testing.T) { n := testing.AllocsPerRun(100, func() { buf.Reset() testHeader.WriteSubset(&buf, nil) } if n > 1 { // TODO blah blah } } https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/header_tes... src/pkg/net/http/header_test.go:188: if got, max := allocs, float64(1); got > max { As an aside, introducing the 'max' variable here makes the code verbose. It's okay to repeat the 1 once. That's less verbose and clearer. The same goes for the const N = 100. https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/httputil/c... File src/pkg/net/http/httputil/chunked_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/httputil/c... src/pkg/net/http/httputil/chunked_test.go:61: allocs := testing.AllocsPerRun(N, func() { This is not obviously clearer. Please revert. https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/rpc/server_test.go File src/pkg/net/rpc/server_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/rpc/server_test... src/pkg/net/rpc/server_test.go:449: const N = 100 Delete & s/N/100/ below. https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/filepath/path_... File src/pkg/path/filepath/path_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/filepath/path_... src/pkg/path/filepath/path_test.go:94: const N = 100 Delete and s/N/100/ below. https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/filepath/path_... src/pkg/path/filepath/path_test.go:113: allocs := testing.AllocsPerRun(N, func() { Might as well even join the lines. func() {filepath.Clean(test.result)} https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/path_test.go File src/pkg/path/path_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/path_test.go#n... src/pkg/path/path_test.go:66: const N = 100 Same. https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/path_test.go#n... src/pkg/path/path_test.go:78: allocs := testing.AllocsPerRun(N, func() { Same. https://codereview.appspot.com/7002055/diff/30001/src/pkg/strconv/strconv_tes... File src/pkg/strconv/strconv_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/strconv/strconv_tes... src/pkg/strconv/strconv_test.go:46: const N = 100 Same. https://codereview.appspot.com/7002055/diff/30001/src/pkg/time/time_test.go File src/pkg/time/time_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/time/time_test.go#n... src/pkg/time/time_test.go:1260: const N = 100 Same. https://codereview.appspot.com/7002055/diff/30001/src/pkg/time/time_test.go#n... src/pkg/time/time_test.go:1263: allocs := testing.AllocsPerRun(N, mt.fn) The old version of this did allocs := int(testing.AllocsPerRun(100, mt.fn)) Then you don't need the float64 conversion or the max variable on the next line.

[kevlar, 2013-01-31 06:14:02]

PTAL Thanks for the comments! https://codereview.appspot.com/7002055/diff/30001/src/pkg/math/big/nat_test.go File src/pkg/math/big/nat_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/math/big/nat_test.g... src/pkg/math/big/nat_test.go:169: func allocSizePerRun(runs int, f func()) float64 { On 2013/01/30 17:55:41, rsc wrote: > I don't see any reason to change this file, and the change makes the test 100x > slower. Please revert. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/chunked_te... File src/pkg/net/http/chunked_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/chunked_te... src/pkg/net/http/chunked_test.go:59: allocs := testing.AllocsPerRun(N, func() { On 2013/01/30 17:55:41, rsc wrote: > This file is not noticeably better. Please revert. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/header_tes... File src/pkg/net/http/header_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/header_tes... src/pkg/net/http/header_test.go:179: for i, f := 0, headerWriteSubsetFunc(); i < b.N; i++ { On 2013/01/30 17:55:41, rsc wrote: > var testHeader = Header{ > "Content-Length": {"123"}, > "Content-Type": {"text/plain"}, > "Date": {"some date at some time Z"}, > "Server": {"Go http package"}, > } > > var buf bytes.Buffer > > func BenchmarkHeaderWriteSubset(b *testing.B) { > b.ReportAllocs() > for i := 0; i < b.N; i++ { > buf.Reset() > testHeader.WriteSubset(&buf, nil) > } > } > > func TestHeaderWriteSubsetMallocs(t *testing.T) { > n := testing.AllocsPerRun(100, func() { > buf.Reset() > testHeader.WriteSubset(&buf, nil) > } > if n > 1 { > // TODO blah blah > } > } > Much nicer, thanks. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/header_tes... src/pkg/net/http/header_test.go:188: if got, max := allocs, float64(1); got > max { On 2013/01/30 17:55:41, rsc wrote: > As an aside, introducing the 'max' variable here makes the code verbose. It's > okay to repeat the 1 once. That's less verbose and clearer. The same goes for > the const N = 100. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/httputil/c... File src/pkg/net/http/httputil/chunked_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/http/httputil/c... src/pkg/net/http/httputil/chunked_test.go:61: allocs := testing.AllocsPerRun(N, func() { On 2013/01/30 17:55:41, rsc wrote: > This is not obviously clearer. Please revert. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/rpc/server_test.go File src/pkg/net/rpc/server_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/net/rpc/server_test... src/pkg/net/rpc/server_test.go:449: const N = 100 On 2013/01/30 17:55:41, rsc wrote: > Delete & s/N/100/ below. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/filepath/path_... File src/pkg/path/filepath/path_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/filepath/path_... src/pkg/path/filepath/path_test.go:94: const N = 100 On 2013/01/30 17:55:41, rsc wrote: > Delete and s/N/100/ below. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/filepath/path_... src/pkg/path/filepath/path_test.go:113: allocs := testing.AllocsPerRun(N, func() { On 2013/01/30 17:55:41, rsc wrote: > Might as well even join the lines. > > func() {filepath.Clean(test.result)} Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/path_test.go File src/pkg/path/path_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/path_test.go#n... src/pkg/path/path_test.go:66: const N = 100 On 2013/01/30 17:55:41, rsc wrote: > Same. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/path/path_test.go#n... src/pkg/path/path_test.go:78: allocs := testing.AllocsPerRun(N, func() { On 2013/01/30 17:55:41, rsc wrote: > Same. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/strconv/strconv_tes... File src/pkg/strconv/strconv_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/strconv/strconv_tes... src/pkg/strconv/strconv_test.go:46: const N = 100 On 2013/01/30 17:55:41, rsc wrote: > Same. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/time/time_test.go File src/pkg/time/time_test.go (right): https://codereview.appspot.com/7002055/diff/30001/src/pkg/time/time_test.go#n... src/pkg/time/time_test.go:1260: const N = 100 On 2013/01/30 17:55:41, rsc wrote: > Same. Done. https://codereview.appspot.com/7002055/diff/30001/src/pkg/time/time_test.go#n... src/pkg/time/time_test.go:1263: allocs := testing.AllocsPerRun(N, mt.fn) On 2013/01/30 17:55:41, rsc wrote: > The old version of this did > > allocs := int(testing.AllocsPerRun(100, mt.fn)) > > Then you don't need the float64 conversion or the max variable on the next line. Done.

[rsc, 2013-02-03 03:47:15]

LGTM https://codereview.appspot.com/7002055/diff/30003/src/pkg/testing/allocs.go File src/pkg/testing/allocs.go (right): https://codereview.appspot.com/7002055/diff/30003/src/pkg/testing/allocs.go#n... src/pkg/testing/allocs.go:1: package testing missing copyright; i'll add it before submitting

[rsc, 2013-02-03 03:52:43]

*** Submitted as https://code.google.com/p/go/source/detail?r=b7911b093227 *** testing: add AllocsPerRun This CL also replaces similar loops in other stdlib package tests with calls to AllocsPerRun. Fixes issue 4461. R=minux.ma, rsc CC=golang-dev https://codereview.appspot.com/7002055 Committer: Russ Cox <rsc@golang.org>

[remyoudompheng, 2013-07-20 20:02:21]

R=close