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

Jump to bottom

x/perf/cmd/benchstat: output format compatability breaks #42371

Closed
changkun opened this issue Nov 4, 2020 · 3 comments
Closed

x/perf/cmd/benchstat: output format compatability breaks #42371

changkun opened this issue Nov 4, 2020 · 3 comments
Labels
FrozenDueToAge 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.
Milestone
Unreleased

Comments

@changkun
Copy link
Member

changkun commented Nov 4, 2020 •
edited

After CL 263804, the format of benchmark results contains pkg and cpu information.
This breaks the compatibility with benchstat such that outputs from previous versions cannot be consumed and compared to the outputs from the tip (9848e93). For instance,

$ benchstat 115.txt 116.txt

Expect a compared result but no output at the moment. A workaround could just delete the output regarding these two lines:

pkg: main
cpu: Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz

But a fix to the benchstat that ignores the change from the CL would be great.
@gopherbot gopherbot added this to the Unreleased milestone Nov 4, 2020
@toothrot
Copy link
Contributor

toothrot commented Nov 6, 2020

/cc @martisch

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 6, 2020
@martisch
Copy link
Contributor

martisch commented Nov 7, 2020 •
edited

The pkg label was not introduced with CL 263804, it has been there before.

I cannot reproduce the problem. Could it be that the problem is that the pkg label is missing from 115.txt in the example given but that it is missing is unrelated to CL 263804? Please post how those benchmark files were created.

Running on tip in /src:

cd strconv

go test -bench=BenchmarkAtof64 -count=5 > ~/go116.txt

cd ..

git checkout go1.15.4

./make.bash

cd strconv

go test -bench=BenchmarkAtof64 -count=5 > ~/go115.txt

name                   old time/op  new time/op  delta
Atof64Decimal-72       28.7ns ± 0%  28.2ns ± 1%   -1.61%  (p=0.008 n=5+5)
Atof64Float-72         34.5ns ± 0%  33.6ns ± 0%   -2.78%  (p=0.008 n=5+5)
Atof64FloatExp-72      61.6ns ± 0%  41.0ns ± 0%  -33.48%  (p=0.016 n=4+5)
Atof64Big-72           95.5ns ± 0%  82.0ns ± 0%  -14.16%  (p=0.000 n=5+4)
Atof64RandomBits-72     186ns ± 0%   112ns ± 0%  -39.78%  (p=0.000 n=5+4)
Atof64RandomFloats-72   113ns ± 0%    85ns ± 0%  -25.08%  (p=0.016 n=4+5)

@martisch martisch added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 7, 2020
@changkun
Copy link
Member Author

changkun commented Nov 7, 2020

Interesting. I can't reproduce the issue anymore, and not sure how the pkg tag was lost from the 115 outputs.

Close and sorry...

@changkun changkun closed this as completed Nov 7, 2020
@golang golang locked and limited conversation to collaborators Nov 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge 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.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@toothrot @martisch @changkun @gopherbot