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

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

jpap opened this issue Jul 28, 2019 · 9 comments

Comments

@jpap
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Jul 28, 2019

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

@toothrot

This comment has been minimized.

Copy link
Contributor

@toothrot toothrot commented Jul 29, 2019

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

@jpap

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

@toothrot toothrot commented Jul 29, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 25, 2019

/cc @aclements for thoughts

@aclements

This comment has been minimized.

Copy link
Member

@aclements aclements commented Sep 25, 2019

@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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 9, 2019

No final comments, so declined.

@rsc rsc closed this 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.