[go] Fix #49973 benchtime too long leads timeout
205 views
Skip to first unread message
Gerrit Dou (Gerrit)
Dec 15, 2021, 1:52:41 PM12/15/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Gerrit Dou has uploaded this change for review.
Fix #49973 benchtime too long leads timeout
Fix #49973
When benchmark function execute time is very short, set benchtime=5s / 10s / 20s is ok.
```
BenchmarkNewObject-2 1000000000 0.279 ns/op
```
But set benchtime=120s will execute time out.
```
*** Test killed with quit: ran too long (11m0s).
```
1. benchtime short
The `B.n` maybe 1 , 100 , 10000, 1000000, 100000000, 100000000 and exist.
2. benchtime long
But set benchtime=120s, `n = goalns * prevIters / prevns` may be **overflow**(< 0) int64, and the `B.n` maybe 1, 100, 10000, 1000000, 100000000, 100000001, 100000002 .... and timeout.
Change-Id: I07093153cda11387d88b57c0349b04d64320185a
GitHub-Last-Rev: d5e53fad8b4870e76ea1310fb4a7994a97d329e2
GitHub-Pull-Request: golang/go#50023
---
M src/testing/benchmark.go
M src/testing/benchmark_test.go
2 files changed, 85 insertions(+), 28 deletions(-)
diff --git a/src/testing/benchmark.go b/src/testing/benchmark.go
index d8ec217..117af30 100644
--- a/src/testing/benchmark.go
+++ b/src/testing/benchmark.go
@@ -323,10 +323,17 @@
// which can hide an order of magnitude in execution time.
// So multiply first, then divide.
n = goalns * prevIters / prevns
- // Run more iterations than we think we'll need (1.2x).
- n += n / 5
- // Don't grow too fast in case we had timing errors previously.
- n = min(n, 100*last)
+ // Can't restore goalns means overflows.
+ if n*prevns/prevIters != goalns {
+ // Overflows means need to be executed many many times,
+ // otherwise it is easy to be timeout.
+ n = 100 * last
+ } else {
+ // Run more iterations than we think we'll need (1.2x).
+ n += n / 5
+ // Don't grow too fast in case we had timing errors previously.
+ n = min(n, 100*last)
+ }
// 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.)
diff --git a/src/testing/benchmark_test.go b/src/testing/benchmark_test.go
index 3b1dc82..ee8d99f 100644
--- a/src/testing/benchmark_test.go
+++ b/src/testing/benchmark_test.go
@@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
-package testing_test
+package testing
import (
"bytes"
@@ -10,7 +10,6 @@
"sort"
"strings"
"sync/atomic"
- "testing"
"text/template"
"time"
)
@@ -32,19 +31,19 @@
{0.0000999949999, " 0.0001000 x"},
}
-func TestPrettyPrint(t *testing.T) {
+func TestPrettyPrint(t *T) {
for _, tt := range prettyPrintTests {
buf := new(strings.Builder)
- testing.PrettyPrint(buf, tt.v, "x")
+ PrettyPrint(buf, tt.v, "x")
if tt.expected != buf.String() {
t.Errorf("prettyPrint(%v): expected %q, actual %q", tt.v, tt.expected, buf.String())
}
}
}
-func TestResultString(t *testing.T) {
+func TestResultString(t *T) {
// Test fractional ns/op handling
- r := testing.BenchmarkResult{
+ r := BenchmarkResult{
N: 100,
T: 240 * time.Nanosecond,
}
@@ -68,15 +67,15 @@
}
}
-func TestRunParallel(t *testing.T) {
- if testing.Short() {
+func TestRunParallel(t *T) {
+ if Short() {
t.Skip("skipping in short mode")
}
- testing.Benchmark(func(b *testing.B) {
+ Benchmark(func(b *B) {
procs := uint32(0)
iters := uint64(0)
b.SetParallelism(3)
- b.RunParallel(func(pb *testing.PB) {
+ b.RunParallel(func(pb *PB) {
atomic.AddUint32(&procs, 1)
for pb.Next() {
atomic.AddUint64(&iters, 1)
@@ -91,9 +90,9 @@
})
}
-func TestRunParallelFail(t *testing.T) {
- testing.Benchmark(func(b *testing.B) {
- b.RunParallel(func(pb *testing.PB) {
+func TestRunParallelFail(t *T) {
+ Benchmark(func(b *B) {
+ b.RunParallel(func(pb *PB) {
// The function must be able to log/abort
// w/o crashing/deadlocking the whole benchmark.
b.Log("log")
@@ -102,9 +101,9 @@
})
}
-func TestRunParallelFatal(t *testing.T) {
- testing.Benchmark(func(b *testing.B) {
- b.RunParallel(func(pb *testing.PB) {
+func TestRunParallelFatal(t *T) {
+ Benchmark(func(b *B) {
+ b.RunParallel(func(pb *PB) {
for pb.Next() {
if b.N > 1 {
b.Fatal("error")
@@ -114,9 +113,9 @@
})
}
-func TestRunParallelSkipNow(t *testing.T) {
- testing.Benchmark(func(b *testing.B) {
- b.RunParallel(func(pb *testing.PB) {
+func TestRunParallelSkipNow(t *T) {
+ Benchmark(func(b *B) {
+ b.RunParallel(func(pb *PB) {
for pb.Next() {
if b.N > 1 {
b.SkipNow()
@@ -128,11 +127,11 @@
func ExampleB_RunParallel() {
// Parallel benchmark for text/template.Template.Execute on a single object.
- testing.Benchmark(func(b *testing.B) {
+ Benchmark(func(b *B) {
templ := template.Must(template.New("test").Parse("Hello, {{.}}!"))
// RunParallel will create GOMAXPROCS goroutines
// and distribute work among them.
- b.RunParallel(func(pb *testing.PB) {
+ b.RunParallel(func(pb *PB) {
// Each goroutine has its own bytes.Buffer.
var buf bytes.Buffer
for pb.Next() {
@@ -144,8 +143,8 @@
})
}
-func TestReportMetric(t *testing.T) {
- res := testing.Benchmark(func(b *testing.B) {
+func TestReportMetric(t *T) {
+ res := Benchmark(func(b *B) {
b.ReportMetric(12345, "ns/op")
b.ReportMetric(0.2, "frobs/op")
})
@@ -164,7 +163,7 @@
func ExampleB_ReportMetric() {
// This reports a custom benchmark metric relevant to a
// specific algorithm (in this case, sorting).
- testing.Benchmark(func(b *testing.B) {
+ Benchmark(func(b *B) {
var compares int64
for i := 0; i < b.N; i++ {
s := []int{5, 4, 3, 2, 1}
@@ -178,3 +177,23 @@
b.ReportMetric(float64(compares)/float64(b.N), "compares/op")
})
}
+
+func TestBenchmarkLaunch(t *T) {
+ tmp := benchTime
+ t.cleanups = append(t.cleanups, func() {
+ t.Logf("reset benchTime")
+ benchTime = tmp
+ })
+ // Set a long benchtime.
+ benchTime = durationOrCountFlag{
+ d: 150 * time.Second,
+ }
+ var try int32 = 0
+ Benchmark(func(b *B) {
+ c := atomic.AddInt32(&try, 1)
+ t.Logf("try %d %d\n", c, b.N)
+ if c > 6 {
+ t.Fatalf("benchmark try to many times %d", c)
+ }
+ })
+}
To view, visit change 372395. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Feb 24, 2022, 12:13:24 PM2/24/22
to Gerrit Bot, goph...@pubsubhelper.golang.org, Bryan Mills, Marcel van Lohuizen, Gopher Robot, golang-co...@googlegroups.com
8 comments:
Commit Message:
Fix #49973 benchtime too long leads timeout
Fix #49973
See https://go.dev/doc/contribute#commit_messages for conventions about how we format Go commit messages.
File src/testing/benchmark.go:
Patch Set #1, Line 327:
n*prevns/prevIters != goalns4It isn't obvious to me why this expression itself is correct.
- How do we know that goalns is an integer multiple of prevIters?
- If n overflowed, how do we know that this computation won't itself overflow in a way that wraps back to goalns?
It seems much simpler to me to avoid the overflow using a conventional range check:
if goalns >= math.MaxInt64 / prevIters {
// goalns * prevIters could overflow.
n = …
} else {
n = goalns * prevIters / prevns
}
Patch Set #1, Line 327:
if n*prevns/prevIters != goalns {M
Patch Set #1, Line 330:
n = 100 * lastHow do we know that `100 * last` is not a gross overestimate?
At line 329 in the baseline it is used as an upper bound, not an absolute growth factor, and here the risk of overflow occurs with a high `prevIters` — which suggests a long `prevns` that may already be close to `goalns`.
Perhaps it would be more robust to do the computation in `float64` if it would overflow `int64`?
(Or, maybe we should *always* do the computation in `float64`?)
File src/testing/benchmark_test.go:
Patch Set #1, Line 5:
package testing_testPlease leave this part of the test as it was.
t.cleanups = append(t.cleanups, func() {
t.Logf("reset benchTime")
benchTime = tmp
})It should not be necessary to access the unexported `cleanups` field or benchTime variable.
Instead, either use `flag.Lookup` to set the flag (and `t.Cleanup` to reset it), or add the test as an integration test in cmd/go/testdata/script instead of a unit-test here.
Patch Set #1, Line 189:
d: 150 * time.Second,Does this test actually take 150s to run?
If so, that's much too long. Can the fix be verified without actually running the test for that long?
If not, why not? (A code comment would be helpful here.)
Patch Set #1, Line 195:
if c > 6 {How do you know that the number of tries will be <6 here?
(`testing.Benchmark` does not appear to make any guarantees as to how many times the function is invoked.)
To view, visit change 372395. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
May 12, 2022, 11:32:53 AM5/12/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Gerrit Bot uploaded patch set #2 to this change.
Fix #49973 benchtime too long leads timeout
Fix #49973
When benchmark function execute time is very short, set benchtime=5s / 10s / 20s is ok.
```
BenchmarkNewObject-2 1000000000 0.279 ns/op
```
But set benchtime=120s will execute time out.
```
*** Test killed with quit: ran too long (11m0s).
```
1. benchtime short
The `B.n` maybe 1 , 100 , 10000, 1000000, 100000000, 100000000 and exist.
2. benchtime long
But set benchtime=120s, `n = goalns * prevIters / prevns` may be **overflow**(< 0) int64, and the `B.n` maybe 1, 100, 10000, 1000000, 100000000, 100000001, 100000002 .... and timeout.
Change-Id: I07093153cda11387d88b57c0349b04d64320185a
GitHub-Last-Rev: affbe9624f3ff28f245c28884162ecc18ad57366
GitHub-Pull-Request: golang/go#50023
---
M src/testing/benchmark.go
M src/testing/benchmark_test.go
2 files changed, 64 insertions(+), 7 deletions(-)
To view, visit change 372395. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
May 12, 2022, 11:45:28 AM5/12/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Gerrit Bot uploaded patch set #3 to this change.
testing: fix benchmark overflow
Fix #49973
When benchmark function execute time is very short, set benchtime=5s / 10s / 20s is ok.
```
BenchmarkNewObject-2 1000000000 0.279 ns/op
```
But set benchtime=120s will execute time out.
```
*** Test killed with quit: ran too long (11m0s).
```
1. benchtime short
The `B.n` maybe 1 , 100 , 10000, 1000000, 100000000, 100000000 and exist.
2. benchtime long
But set benchtime=120s, `n = goalns * prevIters / prevns` may be **overflow**(< 0) int64, and the `B.n` maybe 1, 100, 10000, 1000000, 100000000, 100000001, 100000002 .... and timeout.
Change-Id: I07093153cda11387d88b57c0349b04d64320185a
GitHub-Last-Rev: affbe9624f3ff28f245c28884162ecc18ad57366
GitHub-Pull-Request: golang/go#50023
---
M src/testing/benchmark.go
M src/testing/benchmark_test.go
2 files changed, 64 insertions(+), 7 deletions(-)
To view, visit change 372395. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
May 12, 2022, 11:53:43 AM5/12/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Gerrit Bot uploaded patch set #4 to this change.
testing: fix benchmark overflow
The original implementation could cause timeouts if a long benchmark was set and the test function took a short time, so float64 was used and overflow detection was added.
Fixes #49973
Change-Id: I07093153cda11387d88b57c0349b04d64320185a
GitHub-Last-Rev: affbe9624f3ff28f245c28884162ecc18ad57366
GitHub-Pull-Request: golang/go#50023
---
M src/testing/benchmark.go
M src/testing/benchmark_test.go
2 files changed, 48 insertions(+), 7 deletions(-)
To view, visit change 372395. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
May 12, 2022, 11:57:57 AM5/12/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Gerrit Bot uploaded patch set #5 to this change.
testing: fix benchmark overflow
The original implementation could cause timeouts if a long benchmark was set and the test function took a short time, so float64 was used and overflow detection was added.
Fixes #49973
Change-Id: I07093153cda11387d88b57c0349b04d64320185a
GitHub-Last-Rev: 999da1be993396aafafcf9a9310f7d72929e140c
GitHub-Pull-Request: golang/go#50023
---
M src/testing/benchmark.go
M src/testing/benchmark_test.go
2 files changed, 48 insertions(+), 7 deletions(-)
To view, visit change 372395. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Feb 15, 2025, 5:30:56 AM2/15/25
to Gerrit Bot, goph...@pubsubhelper.golang.org, Marcel van Lohuizen, golang-co...@googlegroups.com
Gopher Robot abandoned this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages