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: cmd/go: -benchformat option in go test #12826

Closed
nodirt opened this issue Oct 3, 2015 · 10 comments
Closed

proposal: cmd/go: -benchformat option in go test #12826

nodirt opened this issue Oct 3, 2015 · 10 comments

Comments

@nodirt
Copy link
Contributor

@nodirt nodirt commented Oct 3, 2015

Background

Currently go test -bench . prints out benchmark results in a certain format, but there is no guarantee that this format will not change. Thus a tool that parses go test output may break if an incompatible change to the output format is made.

Also currently there is no good way to distinguish benchmark results from the rest of go test output, other than checking that a line starts with "Benchmark".

Proposal

  1. Add -benchformat flag to go test. Its value is a text/template template with BenchmarkResult input, similar to -f flag in go list.
  2. Add Proc to BenchmarkResult. It will have the value of runtime.GOMAXPROCS at the beginning of B.launch call.

Example:

$ go test -run=^$  -bench . \
  -benchformat "BENCHMARK STARTS HERE:{{.Name}}:{{.Proc}}:{{.T}}:{{.N}}:{{.Bytes}}"
PASS
BENCHMARK STARTS HERE:BenchmarkFoo:8:1s:10000:0
BENCHMARK STARTS HERE:BenchmarkBar:8:1s:10000:0
@nodirt
Copy link
Contributor Author

@nodirt nodirt commented Oct 3, 2015

One may be concerned that adding -benchformat for benchmarks without adding -format for tests would be inconsistent. We can add -format for tests too.

Currently there is no struct for a test result template. It would need to be added. I don't see a strong reason to add testing.TestResult to that API, though, as we can just document the test result struct in go test -help.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 3, 2015

Considering that go list accepts a text/template format, this seems consistent. I think it's probably worth exploring and writing up a more formal proposal, unless somebody disagrees.

/cc @adg

@minux
Copy link
Member

@minux minux commented Oct 4, 2015

@nodirt
Copy link
Contributor Author

@nodirt nodirt commented Oct 4, 2015

Thanks for mentioning #2981.

Assuming implementation would use encoding/json package without manual concatenation of json chunks, one advantage of -format over -json is that go test can stream results to stdout, rather than waiting for completion of all tests/benchmarks and then dump a big json blob. This is important for tools that present test results to the user. In my case, I need to process test output and print it out (annotated) as soon as a benchmark completes. Without streaming UX would be worse.

Even if we concatenate json chunks, parsing them with encoding/json would not be convenient either.

Also from reading #2981 it seems that adding -json is hard, whereas adding -format is trivial, more consistent with go list and provides same, if not more, functionality.

@minux
Copy link
Member

@minux minux commented Oct 4, 2015

@nodirt
Copy link
Contributor Author

@nodirt nodirt commented Oct 5, 2015

Let's continue the discussion about -json in #2981.

adg added a commit to golang/proposal that referenced this issue Oct 8, 2015
During discussion in golang/go#12826
adg@, minux@ and nodir@ decided that always streaming would simplify
-json flag design.

Also this change makes `testing.Result` internal, which decreases
modifications to the public api.

Change-Id: I40444bb9b34db53ecf2d6ad606578a874c4bd473
Reviewed-on: https://go-review.googlesource.com/15603
Reviewed-by: Andrew Gerrand <adg@golang.org>
@rsc rsc added this to the Proposal milestone Oct 24, 2015
@rsc
Copy link
Contributor

@rsc rsc commented Oct 24, 2015

Currently go test -bench . prints out benchmark results in a certain format, but there is no guarantee that this format will not change.

OK. Counter-proposal: let's guarantee that this format will not change.

Specifically, I propose that the output format is exactly a sequence of lines containing space-separated fields:

<name> <iterations> (<value> <unit>)+

The number of on a particular line is not specified, nor is the space of possible s.

This describes all the current output and blesses (instead of forcing rewrites of) tools that parse the output already, like benchcmp and benchstat.

/cc @aclements

@rsc rsc added the Proposal label Oct 24, 2015
@aclements
Copy link
Member

@aclements aclements commented Oct 26, 2015

I don't have a strong opinion about this, but I'm inclined to think it's unnecessary as long as we simply bless the current format as suggested by @rsc and especially unnecessary if we add -json (and if I had to choose between the two, I would definitely choose -json).

Suppose we were to simply bless the current format. What future changes to go test -bench might that cause friction with? I have a few things on my list. I'd like an external tool (e.g., benchstat) to be able to do linearity testing. We could support that by outputting lines for each run-up step. I'd like go test -bench to be able to output many more instances of shorter runs so external tools can compute more robust statistics. Again, go test -bench can just output more instances of the same benchmark line. I've thought about adding custom outputs (# of GCs, max pause, etc). We could support that by adding more (value unit) pairs.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 12, 2016

Formal counter-proposal: #14313.

@adg
Copy link
Contributor

@adg adg commented Aug 15, 2016

Either #14313 or #2981 will go ahead in favor of this proposal. Marking as declined. Thanks for your time.

@adg adg closed this Aug 15, 2016
@golang golang locked and limited conversation to collaborators Aug 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants