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.

Sign up for GitHub

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

Jump to bottom

testing: b.Loop gives bogus results in some situations [1.24 backport] #72974

Closed
gopherbot opened this issue Mar 20, 2025 · 5 comments
Closed

testing: b.Loop gives bogus results in some situations [1.24 backport] #72974

gopherbot opened this issue Mar 20, 2025 · 5 comments
Labels
CherryPickApproved Used during the release process for point releases
Milestone
Go1.24.2

Comments

@gopherbot
Copy link
Contributor

@aclements requested issue #72971 to be considered for backport to the next 1.24 minor release.

@gopherbot, please open a backport to Go 1.24.

I'm on the fence about whether this should be backported. It makes b.Loop, which is new in Go 1.24, more robust to user mistakes, but those same mistakes could have been made with the old b.N style benchmarks, we just couldn't catch them. In support of backporting, it is a brand new feature and we're going to post a blog post about it soon so it'll probably get a spike in usage.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Mar 20, 2025
@gopherbot gopherbot mentioned this issue Mar 20, 2025
Closed
@gopherbot gopherbot added this to the Go1.24.2 milestone Mar 20, 2025
@aclements
Copy link
Member

If we do want to backport this, we need to land backport #72934 first.

@aclements aclements mentioned this issue Mar 25, 2025
Closed
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/660556 mentions this issue: [release-branch.go1.24] testing: separate b.Loop counter from b.N

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/660557 mentions this issue: [release-branch.go1.24] testing: detect early return from B.Loop

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/660558 mentions this issue: [release-branch.go1.24] testing: detect a stopped timer in B.Loop

@dmitshur dmitshur added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Mar 26, 2025
gopherbot pushed a commit that referenced this issue Mar 26, 2025
@aclements @gopherbot
[release-branch.go1.24] testing: separate b.Loop counter from b.N
9204aca 
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.

For #72974.

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>
Reviewed-on: https://go-review.googlesource.com/c/go/+/660556
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue Mar 26, 2025
@aclements @gopherbot
[release-branch.go1.24] testing: detect early return from B.Loop
1d755aa 
Currently, if a benchmark function returns prior to B.Loop() returning
false, we'll report a bogus result. While there was no way to detect
this with b.N-style benchmarks, one way b.Loop()-style benchmarks are
more robust is that we *can* detect it.

This CL adds a flag to B that tracks if B.Loop() has finished and
checks it after the benchmark completes. If there was an early exit
(not caused by another error), it reports a B.Error.

For #72974.

Change-Id: I731c1350e6df938c0ffa08fcedc11dc147e78854
Reviewed-on: https://go-review.googlesource.com/c/go/+/659656
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Austin Clements <austin@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Junyang Shao <shaojunyang@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/660557
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
@gopherbot
Copy link
Contributor Author

Closed by merging CL 660556 (commit 9204aca) to release-branch.go1.24.

gopherbot pushed a commit that referenced this issue Mar 26, 2025
@aclements @gopherbot
[release-branch.go1.24] testing: detect a stopped timer in B.Loop
f3a3023 
Currently, if the user stops the timer in a B.Loop benchmark loop, the
benchmark will run until it hits the timeout and fails.

Fix this by detecting that the timer is stopped and failing the
benchmark right away. We avoid making the fast path more expensive for
this check by "poisoning" the B.Loop iteration counter when the timer
is stopped so that it falls back to the slow path, which can check the
timer.

This causes b to escape from B.Loop, which is totally harmless because
it was already definitely heap-allocated. But it causes the
test/inline_testingbloop.go errorcheck test to fail. I don't think the
escape messages actually mattered to that test, they just had to be
matched. To fix this, we drop the debug level to -m=1, since -m=2
prints a lot of extra information for escaping parameters that we
don't want to deal with, and change one error check to allow b to
escape.

Fixes #72974.

Change-Id: I7d4abbb1ec1e096685514536f91ba0d581cca6b7
Reviewed-on: https://go-review.googlesource.com/c/go/+/659657
Auto-Submit: Austin Clements <austin@google.com>
Reviewed-by: Junyang Shao <shaojunyang@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/660558
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases
Projects
None yet
Milestone
Go1.24.2
Development

No branches or pull requests
3 participants
@dmitshur @aclements @gopherbot