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

Jump to bottom

x/perf/cmd/benchstat: -geomean looks at all benchmarks instead of only matching ones #57648

Closed
greatroar opened this issue Jan 6, 2023 · 3 comments
Closed

x/perf/cmd/benchstat: -geomean looks at all benchmarks instead of only matching ones #57648

greatroar opened this issue Jan 6, 2023 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@greatroar
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.19 linux/amd64

More importantly, I'm using golang.org/x/perf v0.0.0-20221222172245-91a04616dc65.

What did you do?

Run the following benchmarks:

func benchmark(b *testing.B, n int64) {
	b.SetBytes(n)
	for i := 0; i < b.N; i++ {
		time.Sleep(time.Microsecond)
	}
}

func Benchmark1(b *testing.B)   { benchmark(b, 1) }
func Benchmark10(b *testing.B)  { benchmark(b, 10) }
func Benchmark100(b *testing.B) { benchmark(b, 100) }

... as follows:

$ go test -bench=. -count=3 > all.txt
$ go test -bench=10 -count=3 > big.txt
$ benchstat -geomean all.txt big.txt

What did you expect to see?

Geometric means over only the benchmarks 10 and 100, which occur in both files:

name        old time/op    new time/op    delta
10-8          13.6µs ± 3%    13.7µs ±21%    ~     (p=0.700 n=3+3)
100-8         13.6µs ± 3%    13.2µs ± 5%    ~     (p=0.700 n=3+3)
[Geo mean]    13.6µs         13.5µs       -1.05%

name        old speed      new speed      delta
10-8         733kB/s ± 4%   743kB/s ±19%    ~     (p=0.700 n=3+3)
100-8       7.36MB/s ± 3%  7.56MB/s ± 5%    ~     (p=0.700 n=3+3)
[Geo mean]  2.32MB/s       2.37MB/s       +2.08%

What did you see instead?

name        old time/op    new time/op    delta
10-8          13.6µs ± 3%    13.7µs ±21%      ~     (p=0.700 n=3+3)
100-8         13.6µs ± 3%    13.2µs ± 5%      ~     (p=0.700 n=3+3)
[Geo mean]    14.0µs         13.5µs         -3.35%

name        old speed      new speed      delta
10-8         733kB/s ± 4%   743kB/s ±19%      ~     (p=0.700 n=3+3)
100-8       7.36MB/s ± 3%  7.56MB/s ± 5%      ~     (p=0.700 n=3+3)
[Geo mean]   711kB/s       2371kB/s       +233.41%

These geometric means have been computed over the full set of results, as shown by

$ benchstat -geomean all.txt 
name        time/op
1-8           14.6µs ±13%
10-8          13.6µs ± 3%
100-8         13.6µs ± 3%
[Geo mean]    14.0µs     

name        speed
1-8         66.7kB/s ±10%
10-8         733kB/s ± 4%
100-8       7.36MB/s ± 3%
[Geo mean]   711kB/s     

$ benchstat -geomean big.txt 
name        time/op
10-8          13.7µs ±21%
100-8         13.2µs ± 5%
[Geo mean]    13.5µs     

name        speed
10-8         743kB/s ±19%
100-8       7.56MB/s ± 5%
[Geo mean]  2.37MB/s
@gopherbot gopherbot added this to the Unreleased milestone Jan 6, 2023
@thepudds
Copy link
Contributor

thepudds commented Jan 6, 2023 •
edited

Hi @greatroar, thanks for filing this. It sounds like you might be using the latest commit on x/perf master?

FWIW, there is a complete rewrite of benchstat that is in progress, including this CL:
https://go-review.googlesource.com/c/perf/+/309969

It could be interesting to try that on your example to see if you get better or different behavior:

git clone https://go.googlesource.com/perf xperf-benchstat-new
cd xperf-benchstat-new
git fetch https://go.googlesource.com/perf refs/changes/69/309969/11 && git checkout -b change-309969 FETCH_HEAD
cd cmd/benchstat/
go build -o benchstat-new

(Finally, there are some related CLs. I no longer know if CL 309969 is the best one to try, but it might be ;-)

@heschi heschi added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 6, 2023
@heschi
Copy link
Contributor

heschi commented Jan 6, 2023

cc @golang/runtime

@aclements
Copy link
Member

Duplicate of #19565.

The new version of benchstat that @thepudds pointed to should fix this. Sadly it's been on hold for a while, but it is fully functional.

(Finally, there are some related CLs. I no longer know if CL 309969 is the best one to try, but it might be ;-)

That's the right one. :)

@aclements aclements closed this as not planned Won't fix, can't repro, duplicate, stale Jan 6, 2023
@golang golang locked and limited conversation to collaborators Jan 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@aclements @gopherbot @thepudds @heschi @greatroar