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

testing: print more precision in benchmark timings #34626

Closed
voronaam opened this issue Sep 30, 2019 · 11 comments
Closed

testing: print more precision in benchmark timings #34626

voronaam opened this issue Sep 30, 2019 · 11 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@voronaam
Copy link

voronaam commented Sep 30, 2019

When a user supplied high testing.count parameter, they want to look at the statistics in addition to a long stream of numbers in the output.

PoC code available in #34479 along with sample output.

The main reason to do this inside go test is the prettyPrint output which causes loss of precision in any tool that computes the statistics based on the output.

The PoC computes mean and 95% Confidence Interval for any benchmark with the count at or higher than 5. I believe those are reasonable defaults and do not propose to make them configurable.

If this proposal is accepted I can complete the PoC code reasonably fast.

@gopherbot gopherbot added this to the Proposal milestone Sep 30, 2019
@mvdan
Copy link
Member

mvdan commented Oct 1, 2019

Have you looked at https://godoc.org/golang.org/x/perf/cmd/benchstat? Is there a reaeson why you need this to be part of go test directly?

@voronaam
Copy link
Author

voronaam commented Oct 1, 2019

The reason is outlined in the proposal.

The main reason to do this inside go test is the prettyPrint output which causes loss of precision in any tool that computes the statistics based on the output.

But if everybody is happy with this loss, I am fine with the proposal being rejected.

@mvdan
Copy link
Member

mvdan commented Oct 1, 2019

Sorry, I missed that bit from your text. Have you found real scenarios where the loss of precision is a real cause of problems? I've used benchmarks from the scale of nanoseconds to multiple seconds, and I've never really been bothered by the precision. Variance is the usual problem, at least that I've seen.

@voronaam
Copy link
Author

voronaam commented Oct 5, 2019

I do not think it was a cause of any problems. It is just I do not like loosing precision.

I should clarify that I am find keeping this as a local patch just for myself if general userbase is satisfied with the current state.

@rsc
Copy link
Contributor

rsc commented Nov 13, 2019

go test should not be in the business of statistical analysis.
There are so many analyses you might want to do on the numbers.
Benchstat is one set; keeping them outside go test means you can write your own just as easily.

If the problem is that go test does not print enough significant figures for you to write your own analysis tool, let's talk about that instead.

It sounds like you want to see at least four significant digits instead of three,
so that instead of 12.3 ns/op we'd print, say, 12.35 ns/op. I'm skeptical that the 0.05 ns/op has any accuracy behind it, but I also wouldn't object to adding a digit for these small numbers if you can show that it's important.

/cc @aclements

@rsc rsc added this to Incoming in Proposals (old) Nov 27, 2019
@rsc
Copy link
Contributor

rsc commented Nov 27, 2019

If I could paraphrase, the original proposal was "move statistics calculations into the go command, because it has more accurate numbers than it prints".

If we shift the proposal to "print more accurate numbers", implemented by printing an extra digit of precision for the tiny ns/op values, then external tools can still be written to do whatever statistics are needed - we're not going to get them all in the go command.

Would that ("print more accurate numbers") address the problems you were seeing @voronaam?

@voronaam
Copy link
Author

Yes, I think this would be a better solution. I agree that getting everything that external tools may do with the performance test data straight into the go's main testing package may not be a good idea.

Instead it would be nice to enable external tools to have access to enough precision.

Thank you.

@rsc rsc changed the title proposal: testing: Compute benchmark statistics proposal: testing: print more precision in benchmark timings Nov 27, 2019
@rsc rsc moved this from Incoming to Likely Accept in Proposals (old) Dec 4, 2019
@rsc rsc moved this from Likely Accept to Active in Proposals (old) Dec 4, 2019
@rsc
Copy link
Contributor

rsc commented Dec 4, 2019

Based on the discussion, it sounds like "print more precision in benchmark timings" (instead of moving stats into package testing) is a likely accept.

Leaving open for a week for final comments.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Dec 4, 2019
@rsc
Copy link
Contributor

rsc commented Dec 11, 2019

No change in consensus (and checked with @aclements offline), so accepted.

@rsc rsc modified the milestones: Proposal, Go1.15 Dec 11, 2019
@rsc rsc changed the title proposal: testing: print more precision in benchmark timings testing: print more precision in benchmark timings Dec 11, 2019
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Dec 11, 2019
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Dec 11, 2019
@gopherbot
Copy link

Change https://golang.org/cl/210979 mentions this issue: testing: print extra precision in the benchmark output

@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Backlog May 19, 2020
@gopherbot
Copy link

Change https://golang.org/cl/267102 mentions this issue: testing: increase benchmark output to four significant figures

@golang golang locked and limited conversation to collaborators Nov 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants