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

x/perf/benchstat: geomean includes benchmarks that are in only one file #19565

Open
josharian opened this issue Mar 16, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@josharian
Copy link
Contributor

commented Mar 16, 2017

$ cat b
BenchmarkA 10 100 ns/op
BenchmarkA 10 100 ns/op
BenchmarkA 10 100 ns/op
BenchmarkA 10 100 ns/op
BenchmarkA 10 100 ns/op
BenchmarkA 10 100 ns/op
BenchmarkB 10 10000 ns/op
BenchmarkB 10 10000 ns/op
BenchmarkB 10 10000 ns/op
BenchmarkB 10 10000 ns/op
BenchmarkB 10 10000 ns/op
BenchmarkB 10 10000 ns/op
$ cat a
BenchmarkA 10 100 ns/op
BenchmarkA 10 100 ns/op
BenchmarkA 10 100 ns/op
BenchmarkA 10 100 ns/op
BenchmarkA 10 100 ns/op
BenchmarkA 10 100 ns/op
$ benchstat -geomean b a
name        old time/op  new time/op  delta
A            100ns ± 0%   100ns ± 0%     ~     (all equal)
[Geo mean]  1.00µs       0.10µs       -90.00%

The geomean should be 0.

cc @aclements @rsc @quentinmit

@josharian josharian added this to the Unreleased milestone Mar 16, 2017

@aclements

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

@aclements

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

I'm not positive what the "right" behavior here is, though the current behavior is clearly not it. :)

Should we show benchmarks that don't match or not? It's kind of annoying that we silently drop that information, and it makes benchstat somewhat less predictable. That suggests we should show such benchmarks in the appropriate column. But that makes the geomeans definitely less useful, and makes the geomean delta completely meaningless.

Perhaps we should show non-matching benchmarks in their own group after the geomean row, probably separated by a blank line?

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

This just bit another person (private correspondence).

Your solution seems fine, Austin. (I assume that you would not include the non-matching benchmarks in the geomean.)

@ALTree ALTree added the NeedsFix label Sep 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.