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

Open
voronaam opened this issue Sep 30, 2019 · 10 comments
Open

testing: print more precision in benchmark timings #34626

voronaam opened this issue Sep 30, 2019 · 10 comments

Comments

@voronaam
Copy link

@voronaam 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
@gopherbot gopherbot added the Proposal label Sep 30, 2019
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan 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

This comment has been minimized.

Copy link
Author

@voronaam 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

This comment has been minimized.

Copy link
Member

@mvdan 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

This comment has been minimized.

Copy link
Author

@voronaam 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

This comment has been minimized.

Copy link
Contributor

@rsc 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 Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Author

@voronaam voronaam commented Nov 27, 2019

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 Dec 4, 2019
@rsc rsc moved this from Likely Accept to Active in Proposals Dec 4, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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 Dec 4, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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
@rsc rsc moved this from Likely Accept to Accepted in Proposals Dec 11, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 11, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.