testing: exit B.Loop early upon saturation

There's a cap of 1 billion benchmark iterations because more than that
is usually not going to give more useful data. Unfortunately, the
existing implementation neglected to check whether the 1e9 cap had
already been exceeded when it adjusted the number of iterations in the
B.Loop slow path (stopOrScaleBLoop), since it's only when that cap is hit
that it needed to terminate early.

As a result, for _very_ cheap benchmarks (e.g. testing assembly
implementations with just a few instructions), the B.Loop would stop
incrementing the number of iterations, but wouldn't terminate early,
making it re-enter the slow-path _every_ iteration until the benchmark
time was exhausted.

This wasn't normally visible with the default -benchtime 2s, but when
raised to 5s, it would cause benchmarks that took <5ns/op to be reported
as exactly 5ns/op. (which looks a bit suspicious)

Notably, one can use -count for larger groupings to compute statistics.
golang.org/x/perf/cmd/benchstat is valuable for coalescing larger
run-counts from -count into more useful statistics.

Add a test which allows for fewer iterations on slow/contended
platforms but guards against reintroducing a bug of this nature.

Fixes #75210

Change-Id: Ie7f0b2e6c737b064448434f3ed565bfef8c4f020
Reviewed-on: https://go-review.googlesource.com/c/go/+/700275
Reviewed-by: Junyang Shao <shaojunyang@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Sean Liao <sean@liao.dev>
Auto-Submit: Sean Liao <sean@liao.dev>

diff --git a/src/testing/benchmark.go b/src/testing/benchmark.go
index cedd75ea6699a957e0bce79782f874b2cb0ae0dd..437c2ec741b18245b3d4645cbc8747711dbc01ab 100644 (file)
--- a/src/testing/benchmark.go
+++ b/src/testing/benchmark.go
@@ -298,6 +298,9 @@ func (b *B) doBench() BenchmarkResult {
        return b.result
 }
 
+// Don't run more than 1e9 times. (This also keeps n in int range on 32 bit platforms.)
+const maxBenchPredictIters = 1_000_000_000
+
 func predictN(goalns int64, prevIters int64, prevns int64, last int64) int {
        if prevns == 0 {
                // Round up to dodge divide by zero. See https://go.dev/issue/70709.
@@ -317,7 +320,7 @@ func predictN(goalns int64, prevIters int64, prevns int64, last int64) int {
        // Be sure to run at least one more than last time.
        n = max(n, last+1)
        // Don't run more than 1e9 times. (This also keeps n in int range on 32 bit platforms.)
-       n = min(n, 1e9)
+       n = min(n, maxBenchPredictIters)
        return int(n)
 }
 
@@ -403,7 +406,9 @@ func (b *B) stopOrScaleBLoop() bool {
                // in big trouble.
                panic("loop iteration target overflow")
        }
-       return true
+       // predictN may have capped the number of iterations; make sure to
+       // terminate if we've already hit that cap.
+       return uint64(prevIters) < b.loop.n
 }
 
 func (b *B) loopSlowPath() bool {

diff --git a/src/testing/loop_test.go b/src/testing/loop_test.go
index 743cbe64f0613b03d544edf08d2face62945ad7c..71dbd35dc2ddf1187dbc6d712c428e4a47846ca2 100644 (file)
--- a/src/testing/loop_test.go
+++ b/src/testing/loop_test.go
@@ -7,6 +7,7 @@ package testing
 import (
        "bytes"
        "strings"
+       "time"
 )
 
 // See also TestBenchmarkBLoop* in other files.
@@ -75,6 +76,42 @@ func TestBenchmarkBLoop(t *T) {
        }
 }
 
+func TestBenchmarkBLoopCheapEarlyTerminate(t *T) {
+       if Short() {
+               t.Skip("B.Loop test needs to run for > 1s to saturate 1e9 iterations")
+       }
+       runCnt := 0
+       // Set the benchmark time high enough that we're likely to hit the 1B
+       // iteration limit even on very slow hardware.
+       // (on an AMD Ryzen 5900X, this benchmark runs in just over a second)
+       //
+       // Notably, the assertions below shouldn't fail if a test-run is slow
+       // enough that it doesn't saturate the limit.
+       const maxBenchTime = time.Second * 30
+       res := Benchmark(func(b *B) {
+               // Set the benchmark time _much_ higher than required to hit 1e9 iterations.
+               b.benchTime.d = maxBenchTime
+               for b.Loop() {
+                       runCnt++
+               }
+       })
+       if runCnt > maxBenchPredictIters {
+               t.Errorf("loop body ran more than max (%d) times: %d", maxBenchPredictIters, runCnt)
+               if res.T >= maxBenchTime {
+                       t.Logf("cheap benchmark exhausted time budget: %s; ran for %s", maxBenchTime, res.T)
+               }
+       }
+
+       if res.N != runCnt {
+               t.Errorf("disagreeing loop counts: res.N reported %d, while b.Loop() iterated %d times", res.N, runCnt)
+       }
+
+       if res.N > maxBenchPredictIters {
+               t.Errorf("benchmark result claims more runs than max (%d) times: %d", maxBenchPredictIters, res.N)
+       }
+
+}
+
 func TestBenchmarkBLoopBreak(t *T) {
        var bState *B
        var bLog bytes.Buffer