[go] testing: with -benchtime=1x, run the benchmark loop exactly once
Caleb Spare (Gerrit)
Caleb Spare has uploaded this change for review.
testing: with -benchtime=1x, run the benchmark loop exactly once
Like with -benchtime=1ns, if we find that the "discovery" round (run1)
has already crossed the -benchtime threshold, we skip running more
iterations.
Fixes #32051
Change-Id: I76aaef2ba521ba8ad6bbde2b14977e191aada5e4
---
A src/cmd/go/testdata/script/test_benchmark_1x.txt
M src/testing/benchmark.go
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/src/cmd/go/testdata/script/test_benchmark_1x.txt b/src/cmd/go/testdata/script/test_benchmark_1x.txt
new file mode 100644
index 0000000..deeff54
--- /dev/null
+++ b/src/cmd/go/testdata/script/test_benchmark_1x.txt
@@ -0,0 +1,25 @@
+# Test that -benchtime 1x only runs a total of 1 loop iteration.
+# See golang.org/issue/32051.
+
+go test -run ^$ -bench . -benchtime 1x
+
+-- go.mod --
+module bench
+
+go 1.16
+-- x_test.go --
+package bench
+
+import "testing"
+
+var called = false
+
+func Benchmark(b *testing.B) {
+ if b.N > 1 {
+ b.Fatalf("called with b.N=%d; want b.N=1 only", b.N)
+ }
+ if called {
+ b.Fatal("called twice")
+ }
+ called = true
+}
diff --git a/src/testing/benchmark.go b/src/testing/benchmark.go
index 15b4426..01d91a4 100644
--- a/src/testing/benchmark.go
+++ b/src/testing/benchmark.go
@@ -298,7 +298,9 @@
// Run the benchmark for at least the specified amount of time.
if b.benchTime.n > 0 {
- b.runN(b.benchTime.n)
+ if b.benchTime.n > 1 {
+ b.runN(b.benchTime.n)
+ }
} else {
d := b.benchTime.d
for n := int64(1); !b.failed && b.duration < d && n < 1e9; {
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.
Caleb Spare (Gerrit)
Patch set 1:Run-TryBot +1
1 comment:
Patchset:
R=go1.18
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.
Caleb Spare (Gerrit)
Attention is currently required from: Ian Lance Taylor.
1 comment:
Patchset:
Ian could I get you to review this one as well?
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Caleb Spare.
2 comments:
File src/cmd/go/testdata/script/test_benchmark_1x.txt:
Patch Set #1, Line 15:
var called = falseThis test should ideally also test that the benchmark is in fact run once. I think it should work to have a TestMain that verifies that called is true after m.Run.
File src/testing/benchmark.go:
Patch Set #1, Line 301:
if b.benchTime.n > 1 {I think this needs a comment.
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.
Caleb Spare (Gerrit)
Attention is currently required from: Caleb Spare.
Caleb Spare uploaded patch set #2 to this change.
testing: with -benchtime=1x, run the benchmark loop exactly once
Like with -benchtime=1ns, if we find that the "discovery" round (run1)
has already crossed the -benchtime threshold, we skip running more
iterations.
Fixes #32051
Change-Id: I76aaef2ba521ba8ad6bbde2b14977e191aada5e4
---
A src/cmd/go/testdata/script/test_benchmark_1x.txt
M src/testing/benchmark.go
2 files changed, 43 insertions(+), 1 deletion(-)
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.
Caleb Spare (Gerrit)
Attention is currently required from: Ian Lance Taylor.
Patch set 2:Run-TryBot +1
2 comments:
File src/cmd/go/testdata/script/test_benchmark_1x.txt:
Patch Set #1, Line 15:
var called = false
This test should ideally also test that the benchmark is in fact run once. […]
Good point.
File src/testing/benchmark.go:
Patch Set #1, Line 301:
if b.benchTime.n > 1 {I think this needs a comment.
Done
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Attention is currently required from: Caleb Spare, Ian Lance Taylor.
2 comments:
Patchset:
Thank you for this change Caleb, thank you for the review Ian.
Caleb, I’ve made a suggestion to just list out the cases individually to make for much easier reading, otherwise looks good; please take a look.
File src/testing/benchmark.go:
Patch Set #2, Line 300:
if b.benchTime.n > 0 {We could list out these cases for much easier reading, so perhaps:
if b.benchTime == 1 {
// We already ran a single iteration
……
} else if b.benchTime > 1 {
b.run(b.benchTime.n)
} else {
..
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.
Caleb Spare (Gerrit)
Attention is currently required from: Ian Lance Taylor, Emmanuel Odeke.
1 comment:
File src/testing/benchmark.go:
Patch Set #2, Line 300:
if b.benchTime.n > 0 {
We could list out these cases for much easier reading, so perhaps: […]
I prefer the current structure of the code because the top-level 'if' separates the two modes of operation (iterations vs. duration).
There is also a sort of parallelism to the code paths of each arm of that 'if': in both the 'iterations' and 'duration' mode, we only run more iterations if the run1 we already did wasn't enough.
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.
Caleb Spare (Gerrit)
Attention is currently required from: Ian Lance Taylor, Emmanuel Odeke.
Patch set 2:Trust +1
1 comment:
File src/testing/benchmark.go:
Patch Set #2, Line 300:
if b.benchTime.n > 0 {
I prefer the current structure of the code because the top-level 'if' separates the two modes of ope […]
Emmanuel -- ping.
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Attention is currently required from: Caleb Spare, Ian Lance Taylor.
Patch set 2:Code-Review +2
2 comments:
Patchset:
LGTM, thank you Caleb. Please rebase from master and let’s rerun the trybots then submit.
File src/testing/benchmark.go:
Patch Set #2, Line 300:
if b.benchTime.n > 0 {
Emmanuel -- ping.
No biggie then, thanks.
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.
Caleb Spare (Gerrit)
Attention is currently required from: Ian Lance Taylor.
Patch set 3:Run-TryBot +1
Emmanuel Odeke (Gerrit)
Emmanuel Odeke submitted this change.
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
testing: with -benchtime=1x, run the benchmark loop exactly once
Like with -benchtime=1ns, if we find that the "discovery" round (run1)
has already crossed the -benchtime threshold, we skip running more
iterations.
Fixes #32051
Change-Id: I76aaef2ba521ba8ad6bbde2b14977e191aada5e4
Reviewed-on: https://go-review.googlesource.com/c/go/+/331089
Trust: Caleb Spare <ces...@gmail.com>
Run-TryBot: Caleb Spare <ces...@gmail.com>
TryBot-Result: Go Bot <go...@golang.org>
Reviewed-by: Emmanuel Odeke <emma...@orijtech.com>
---
M src/testing/benchmark.go
A src/cmd/go/testdata/script/test_benchmark_1x.txt
2 files changed, 63 insertions(+), 1 deletion(-)
diff --git a/src/cmd/go/testdata/script/test_benchmark_1x.txt b/src/cmd/go/testdata/script/test_benchmark_1x.txt
new file mode 100644
index 0000000..b1d4c39
--- /dev/null
+++ b/src/cmd/go/testdata/script/test_benchmark_1x.txt
@@ -0,0 +1,37 @@
+# Test that -benchtime 1x only runs a total of 1 loop iteration.
+# See golang.org/issue/32051.
+
+go test -run ^$ -bench . -benchtime 1x
+
+-- go.mod --
+module bench
+
+go 1.16
+-- x_test.go --
+package bench
+
+import (
+ "fmt"
+ "os"
+ "testing"
+)
+
+var called = false
+
+func TestMain(m *testing.M) {
+ m.Run()
+ if !called {
+ fmt.Println("benchmark never called")
+ os.Exit(1)
+ }
+}
+
+func Benchmark(b *testing.B) {
+ if b.N > 1 {
+ b.Fatalf("called with b.N=%d; want b.N=1 only", b.N)
+ }
+ if called {
+ b.Fatal("called twice")
+ }
+ called = true
+}
diff --git a/src/testing/benchmark.go b/src/testing/benchmark.go
index 30fa106..1ce637e 100644
--- a/src/testing/benchmark.go
+++ b/src/testing/benchmark.go
@@ -299,7 +299,12 @@
// Run the benchmark for at least the specified amount of time.
if b.benchTime.n > 0 {
- b.runN(b.benchTime.n)
+ // We already ran a single iteration in run1.
+ // If -benchtime=1x was requested, use that result.
+ // See https://golang.org/issue/32051.
+ if b.benchTime.n > 1 {
+ b.runN(b.benchTime.n)
+ }
} else {
d := b.benchTime.d
for n := int64(1); !b.failed && b.duration < d && n < 1e9; {
To view, visit change 331089. To unsubscribe, or for help writing mail filters, visit settings.