Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: early break in a testing.B.Loop benchmark loop should be reported. #72933

Closed
JunyangShao opened this issue Mar 18, 2025 · 2 comments
Closed
Assignees
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted.
Milestone

Comments

@JunyangShao
Copy link
Contributor

Go version

go version go1.24.1 darwin/arm64

Output of go env in your module/workspace:

AR='ar'
CC='clang'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='clang++'
GCCGO='gccgo'
GO111MODULE=''
GOARCH='arm64'
GOARM64='v8.0'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/Users/shaojunyang/Library/Caches/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/Users/shaojunyang/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/6t/d5y6mrc90rzd1gm2q2k99lb400zg0c/T/go-build3077995788=/tmp/go-build -gno-record-gcc-switches -fno-common'
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMOD='/dev/null'
GOMODCACHE='/Users/shaojunyang/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/shaojunyang/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/shaojunyang/Library/Application Support/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.24.1'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

For benchmark that breaks with its own logic, -benchtime won't work and the users should be notified.

What did you see happen?

If we run the following benchmark with go test -bench=. -benchtime=1s:

func BenchmarkFoo(b *testing.B) {
        i := 0
        for b.Loop() {
                fmt.Printf("iter %d\n", i)
                i += 1
		if i > 5 {
			break
		}
                time.Sleep(100 * time.Millisecond)
        }
}

The output would be

iter 0
iter 1
iter 2
iter 3
iter 4
iter 5
goos: darwin
goarch: arm64
cpu: Apple M1 Pro
BenchmarkFoo-10    	      10	  50555750 ns/op
PASS
ok  	command-line-arguments	0.732s

What did you expect to see?

Instead of allowing the benchmark to pass with bogus numbers, we should let the user know that the benchmark failed.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/659655 mentions this issue: testing: separate b.Loop counter from b.N

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/659656 mentions this issue: testing: detect early return from B.Loop

gopherbot pushed a commit that referenced this issue Mar 21, 2025
Currently, b.Loop uses b.N as the iteration count target. However,
since it updates the target as it goes, the behavior is quite
different from a b.N-style benchmark. To avoid user confusion, this CL
gives b.Loop a separate, unexported iteration count target. It ensures
b.N is 0 within the b.Loop loop to help catch misuses, and commits the
final iteration count to b.N only once the loop is done (as the
documentation states "After Loop returns false, b.N contains the total
number of iterations that ran, so the benchmark may use b.N to compute
other average metrics.")

Since there are now two variables used by b.Loop, we put them in an
unnamed struct. Also, we rename b.loopN to b.loop.i because this
variable tracks the current iteration index (conventionally "i"), not
the target (conventionally "n").

Unfortunately, a simple renaming causes B.Loop to be too large for the
inliner. Thus, we make one simplification to B.Loop to keep it under
the threshold. We're about to lean into that simplification anyway in
a follow-up CL, so this is just temporary.

Prep for #72933 and #72971.

Change-Id: Ide1c4f1b9ca37f300f3beb0e60ba6202331b183e
Reviewed-on: https://go-review.googlesource.com/c/go/+/659655
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Junyang Shao <shaojunyang@google.com>
Auto-Submit: Austin Clements <austin@google.com>
@dmitshur dmitshur added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Mar 21, 2025
@dmitshur dmitshur added this to the Go1.25 milestone Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted.
Projects
None yet
Development

No branches or pull requests

3 participants