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.

Sign up for GitHub

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/tools/cmd/benchcmp: Add -tolerance flag #20726

Closed
nicholasjackson opened this issue Jun 19, 2017 · 7 comments
Closed

x/tools/cmd/benchcmp: Add -tolerance flag #20726

nicholasjackson opened this issue Jun 19, 2017 · 7 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unreleased

Comments

@nicholasjackson
Copy link

nicholasjackson commented Jun 19, 2017 •
edited

Running benchcmp on a CI server to detect anomalies relies on the user to parse the output from the command in order to pick up any deltas. To make this process simpler I propose adding an additional flag to benchcmp -tolerance=[Float64] which would cause benchcmp to return a status code 1 when any of the delta percentages are greater than the tollerance.

var (
	changedOnly = flag.Bool("changed", false, "show only benchmarks that have changed")
	magSort     = flag.Bool("mag", false, "sort benchmarks by magnitude of change")
	best        = flag.Bool("best", false, "compare best times from old and new")
	tolerance   = flag.Float64("tolerance", math.MaxFloat64, "return error if a delta is outside given value")
)

The state would be tracked across multiple deltas and types of benchmark under comparison i.e:

if delta := cmp.DeltaAllocsPerOp(); !*changedOnly || delta.Changed() {
    if toleranceExceeded(delta, *tolerance) {
      tolerance_exceeded = true
    }

And if any delta is greater than the tolerance then exit code 1 and an error message would be returned.

if tolerance_exceeded {
	fatal(fmt.Sprintf("Tolerance %.2f%% for one or more benchmarks exceeded", *tolerance))
}

An example of the implementation can be found on my fork of the tools golang/tools@master...nicholasjackson:nic/tollerance_flag

Kind regards,

Nic
@gopherbot gopherbot added this to the Unreleased milestone Jun 19, 2017
@ALTree
Copy link
Member

ALTree commented Jun 19, 2017 •
edited

benchcmp was pretty much deprecated in favour of benchstat (golang.org/x/perf/cmd/benchstat), which is what you should use to compare benchmark results.

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 19, 2017
@nicholasjackson
Copy link
Author

Ok, cheers, the situation with benchstat is the same regarding an exit code and CI, shall I close this and re-open on benchstat?

@ALTree
Copy link
Member

ALTree commented Jun 19, 2017 •
edited

How would this work for benchstat?

Note that benchstat already has an -alpha flag that you can use to set the change significance threshold. Are you proposing that benchstat exits with code 1 everytime at least one benchmark is marked as 'changed in a statistically significant way'?

@nicholasjackson
Copy link
Author

Yes, exactly, that would work, however that might cause unexpected behaviour for backward compatibility?

@ALTree
Copy link
Member

ALTree commented Jun 19, 2017

I don't know if it'll be accepted, but sure. Please open a new issue with your updated request. Thanks!

@ALTree
Copy link
Member

ALTree commented Jun 19, 2017

(I don't think we guarantee backward compatibility on things like the exit code of this kind of helper tool).

@nicholasjackson
Copy link
Author

Thank you, I will open a new issue

@ALTree ALTree closed this as completed Jun 19, 2017
@golang golang locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@nicholasjackson @ALTree @gopherbot