FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
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


Description:
testing: add (*B).Allocs and AllocsPerRun

This CL also replaces similar loops in other stdlib
packages with calls to Allocs/AllocsPerRun.

Fixes issue 4461.

Please review this at https://codereview.appspot.com/7002055/

Affected files:
M src/pkg/encoding/gob/timing_test.go
M src/pkg/exp/html/parse_test.go
M src/pkg/exp/html/token_test.go
M src/pkg/fmt/fmt_test.go
M src/pkg/net/http/header_test.go
M src/pkg/reflect/all_test.go
A src/pkg/testing/allocs.go

Search Discussions

  • Minux at Dec 23, 2012 at 1:53 pm
    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 at Dec 24, 2012 at 1:00 am

    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.

    https://codereview.appspot.com/7002055/
  • Minux Ma at Dec 24, 2012 at 7:12 am
    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/
  • Kevlar at Dec 24, 2012 at 7:37 am
    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 {
    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#newcode25
    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?

    https://codereview.appspot.com/7002055/
  • Minux at Dec 24, 2012 at 7:47 am

    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?
  • Kyle Lemons at Dec 24, 2012 at 8:02 am

    On Mon, Dec 24, 2012 at 2:47 AM, minux 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.
  • Minux at Dec 24, 2012 at 8:39 am

    On Monday, December 24, 2012, Kyle Lemons wrote:

    On Mon, Dec 24, 2012 at 2:47 AM, minux ({}, '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 at Dec 24, 2012 at 7:57 am

    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.

    https://codereview.appspot.com/7002055/
  • Kevlar at Dec 25, 2012 at 7:57 am
    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/
  • Minux at Dec 30, 2012 at 8:01 pm

    On Tue, Dec 25, 2012 at 3:57 PM, 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.
  • Rsc at Dec 29, 2012 at 7:45 pm
    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/
  • Kevlar at Dec 30, 2012 at 1:18 am

    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.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/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 23, '12 at 2:20a
activeDec 30, '12 at 8:01p
posts13
users3
websitegolang.org

3 users in discussion

Kevlar: 7 posts Minux: 5 posts Rsc: 1 post

People

Translate

site design / logo © 2022 Grokbase