Hi Kyle, We also have these tests about mallocs, I think they will all be ...
11 years, 3 months ago
(2012-12-23 13:53:56 UTC)
#2
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
> Hi Kyle, > We also have these tests about mallocs, I think they will ...
11 years, 3 months ago
(2012-12-24 01:00:49 UTC)
#3
> 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.
A lot of the code that's trying to test allocations has to perform some amount ...
11 years, 3 months ago
(2012-12-24 07:37:52 UTC)
#5
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?
On Monday, December 24, 2012, kevlar wrote: > A lot of the code that's trying ...
11 years, 3 months ago
(2012-12-24 07:47:41 UTC)
#6
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?
On 2012/12/24 07:12:36, minux wrote: > i suggest we also return AllocBytes. I just tried ...
11 years, 3 months ago
(2012-12-24 07:57:07 UTC)
#7
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.
On Mon, Dec 24, 2012 at 2:47 AM, minux <minux.ma@gmail.com> wrote: > On Monday, December ...
11 years, 3 months ago
(2012-12-24 08:02:57 UTC)
#8
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.
On Monday, December 24, 2012, Kyle Lemons wrote: > On Mon, Dec 24, 2012 at ...
11 years, 3 months ago
(2012-12-24 08:39:30 UTC)
#9
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.
I did some more experimenting today. I haven't figured out a way to return both ...
11 years, 3 months ago
(2012-12-25 07:57:17 UTC)
#10
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.
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): ...
11 years, 3 months ago
(2012-12-29 19:39:53 UTC)
#11
On 2012/12/29 19:39:53, rsc wrote: > Why does the description mention (*B).Allocs? I don't see ...
11 years, 3 months ago
(2012-12-30 00:50:14 UTC)
#12
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?
On Tue, Dec 25, 2012 at 3:57 PM, <kevlar@google.com> wrote: > I think I agree ...
11 years, 3 months ago
(2012-12-30 20:01:26 UTC)
#13
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.
PTAL There are two alloc-counting tests that didn't convert cleanly: "bytes" TestGrow - seems to ...
11 years, 2 months ago
(2013-01-25 08:48:50 UTC)
#14
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.
Issue 7002055: code review 7002055: testing: add AllocsPerRun
Created 11 years, 3 months ago by kevlar
Modified 10 years, 8 months ago
Reviewers:
Base URL:
Comments: 34