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

proposal: testing: automatically scale benchmark metrics in results output #33328

Closed
jpap opened this issue Jul 28, 2019 · 9 comments
Closed

Comments

@jpap
Copy link
Contributor

jpap commented Jul 28, 2019

When performing a benchmark, we can currently use SetBytes(int64) to set the number of input or output bytes per run, and get a measurement like X MB/s in the output.

It would be nice to be able to set an arbitrary unit instead of "bytes", and to provide multiple scales instead of just "mega" (1e6).

Example 1: pixels per second (px/s): b.SetUnits(1920*1080, "px/s") when benchmarking a full-HD image would print to the output one of the following, depending on the scale of the result:

  • "X px/s"
  • "X kpx/s"
  • "X Mpx/s"
  • "X Gpx/s"

Example 2: frames per second (fps): b.SetUnits(1, "fps") when benchmarking one frame operation.

The existing implementation of SetBytes(b int64) could just call SetUnits(b, "B/s") for backwards compatibility.

@agnivade
Copy link
Contributor

I believe this https://tip.golang.org/pkg/testing/#B.ReportMetric is what you want.

@toothrot toothrot added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jul 29, 2019
@toothrot toothrot added this to the Unplanned milestone Jul 29, 2019
@toothrot
Copy link
Contributor

@jpap Thanks for writing this up. Does @agnivade's suggestion work for you?

@jpap
Copy link
Contributor Author

jpap commented Jul 29, 2019

Thanks @agnivade, it's great that arbitrary units of work are supported in Go 1.13+.

@toothrot, the second part to this issue, auto-scaling the results {_, k, M, G} instead of scaling by 1e6 (M) uniformly as is done now, still remains and doesn't appear to be already implemented (having just checked tip).

@toothrot toothrot changed the title testing: proposal: allow benchmark to use arbitrary units of work per run testing: proposal: automatically scale MB/s metric in benchmark results Jul 29, 2019
@toothrot toothrot added Proposal and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jul 29, 2019
@toothrot
Copy link
Contributor

Great, glad to hear the arbitrary units work (#26037) will help! @jpap Let me know if I captured your request correctly in the title.

@aclements Thoughts on this?

@jpap
Copy link
Contributor Author

jpap commented Jul 29, 2019

@toothrot, it would be great if the unit-scaling applied to all reported metrics in the benchmark data format, not just bytes/sec (MB/s).

@toothrot toothrot changed the title testing: proposal: automatically scale MB/s metric in benchmark results testing: proposal: automatically scale benchmark metrics in results output Jul 30, 2019
@rsc
Copy link
Contributor

rsc commented Sep 25, 2019

/cc @aclements for thoughts

@aclements
Copy link
Member

@jpap, just to clarify, custom metrics reported using ReportMetric aren't autoscaled by 1e6 (I'm not sure if that's what you were saying or not). Only SetBytes scales that way.

It's an interesting question whether we could scale other things this way automatically. Our position has been to keep the benchmark output format very machine-readable (while still being reasonably user-friendly), and to depend on downstream tools to present a much more processed view to users. For example, benchstat does scale the units automatically. You almost always need to do some statistical analysis of the benchmark results anyway, so looking at the raw benchmark output can be misleading at best.

I'm also concerned with how this would interact with custom units. For example, the sync/pool benchmarks emit custom tail latency metrics with units like "p95-ns/STW". Automatically putting a metric prefix on that unit would be the wrong thing to do.

Finally, this would be a break from the benchmark format and would thus require changes to many tools that consume that format.

Given all of this, I do not think the testing package should be autoscaling these units. Instead, we should leverage the existing standard format to build tools to better process and present benchmark results.

@rsc
Copy link
Contributor

rsc commented Oct 2, 2019

Based on the discussion above and past discussions about how hard it is to do arbitrary units correctly in general, this seems like a likely decline.

For a truly amazing programming language that fully embraces the idea of getting units right, see https://frinklang.org/.

Leaving open for a week for final comments.

@rsc
Copy link
Contributor

rsc commented Oct 9, 2019

No final comments, so declined.

@rsc rsc closed this as completed Oct 9, 2019
@andybons andybons changed the title testing: proposal: automatically scale benchmark metrics in results output proposal: testing: automatically scale benchmark metrics in results output Oct 9, 2019
@golang golang locked and limited conversation to collaborators Oct 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants