crypto/ecdsa, crypto/elliptic: changes to benchmarks dramatically improve results on ppc64le with no change in implementation #23137
Comments
|
Yeah, I'm not sure why that CL added parallelization. Seems unrelated. |
|
Cl was inspired by https://blog.cloudflare.com/go-dont-collect-my-garbage/ which complained about performance of parallelized p256 performance, so it add parallelization to better replicate the issue. Go test has -parallel and -cpu flags if you want to run single-threaded version. We already run a lot of benches in parallel after e.g CL 36724,36831,36810 etc so it is impossible to compare benchmarks across go versions. That being said, I'm also ok with reverting those benchmarks to single thread. |
|
If changing parallelism in benchmarks has been done in the past, then it doesn't make sense to just change this one back. I have not seen this kind of difference in the past so it surprised me. Is this package benchmark policy documented somewhere, so that it is known that doing release to release benchmark comparisons is not always meaningful? |
|
The change to the benchmarks seems to be reasonably motivated to uncover issues that the previous benchmarks didn't. I don't believe that we need to have complete consistency for benchmarks and thus it's ok that other arches changed here. |
|
I was under the impression that these benchmarks are used to track performance regressions, but if the benchmarks could be changed at any time in a way that affects their performance then you can't count on using them for that purpose. I understand why you'd want to provide an improved test but if you do then they should be given a different test name IMO so it is clear that it shouldn't be compared with anything from an earlier commit. |
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version)?go 1.10beta1
Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (
go env)?ppc64le Ubuntu
What did you do?
Looking at benchmark results from some packages to find changes in performance.
What did you expect to see?
Consistent or small improvements as compared to results from previous runs, since no specific improvements were made for ppc64le in either of these packages.
What did you see instead?
Dramatic improvements for the benchmark results in crypto/ecdsa and crypto/elliptic starting with CL 80757 even though there were no changes that should affect ppc64le performance. This CL changed the benchmarks and the amd64 implementation but nothing specific to ppc64le.
These results are misleading because there was no change to provide an improvement for these crypto packages, but perfstat shows a big improvement.
I didn't look at the test changes in depth to understand why the results are different, possibly because the tests are now being run in parallel but weren't before.
If the plan is to add parallelized benchmarks then there should still be some that are run exactly as before to compare against.
Before and after this CL on ppc64le:
The text was updated successfully, but these errors were encountered: