golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
120.09k stars 17.24k forks source link

testing: clarify that b.N does not change during execution of the benchmark function #67137

Closed dolmen closed 2 weeks ago

dolmen commented 2 weeks ago

The testing documentation clearly mentions that the loop boundary variable might change during execution:

During benchmark execution, b.N is adjusted until the benchmark function lasts long enough to be timed reliably.

I noticed that range over integer loop have different behavior than for with clause loops when the loop boundary is changed by the loop body: https://go.dev/play/p/cObUT32FcXc

    n := 4
    for i := 0; i < n; i++ {
        fmt.Println(n)
        n--
    }

    fmt.Println("----")

    n = 4
    for range n {
        fmt.Println(n)
        n--
    }
4
3
----
4
3
2
1

I have not found this difference in behavior explicitely mentioned in the Go specification. The following sentence speaks about the first evaluation of the range expression, but doesn't mention later evaluations.

The range expression x is evaluated once before beginning the loop, with one exception: if at most one iteration variable is present and len(x) is constant, the range expression is not evaluated.

I wonder how a benchmark would be affected by the use of range-over-integer instead of the documented style.

I have seen no clear mention of forbidding the use of range-over-integer with b.N for benchmarks. I have not seen either an explicit mention that range-over-int is compatible with testing.B benchmarks which makes me wonder if this has been considered. I also did a code search on range b.N in the Go repository and the only result is a test in the database/sql package just added less that 2 months ago by @bradfitz in CL 572119. No tests in testing.

So it would be helpful to have explicit documentation about the use of b.N with range-over-integer.

If this is in fact an incompatibility, it would be helpful to have a go vet alert about use of range-over-int on testing.B.N.

Go version

go version go1.22.2 darwin/arm64

Output of go env in your module/workspace:

N/A (reproducible on play.golang.org)

What did you see happen?

$ go doc -all testing | grep -c 'range b.N'
0

What did you expect to see?

$ go doc -all testing | grep -c 'range b.N'
1
jub0bs commented 2 weeks ago

Following a discussion on Gophers Slack and a casual inspection of testing's source code, I don't think b.N changes during the execution of a benchmark subject. However, I agree with @dolmen that the documentation is ambiguous/misleading in that respect.

dolmen commented 2 weeks ago

I've been a fulltime Go developer for 9 years. I've read the documentation about benchmarks multiple times. I never thought that the whole benchmark function was called multiple times because this is different that Test functions which are called just once. Instead, because of that sentence in the documentation, I thought that b.N was changed dynamically (with some supposed magic locking that I didn't investigate: after all code coverage via source instrumentation is already magic of go test).

Jorropo commented 2 weeks ago

I have not found this difference in behavior explicitely mentioned in the Go specification. The following sentence speaks about the first evaluation of the range expression, but doesn't mention later evaluations.

The range expression x is evaluated once before beginning the loop, with one exception: if at most one iteration variable is present and len(x) is constant, the range expression is not evaluated.

This is explicitly mentioned: « once before beginning the loop » describe precisely that behavior. I'm not saying this shouldn't be made clearer or something, just it's working as expected and has been for years and changing this would be a language breaking change.

gopherbot commented 2 weeks ago

Change https://go.dev/cl/582835 mentions this issue: testing: improve the documentation around b.N