testing: parallel benchmark results are poorly documented #31884
Comments
andybons
added
the
NeedsFix
When many UUIDs are being generated concurrently, contention on the atomic counter can slow things down. There might be ways to speed this up, but for now, just add a parallel benchmark so we can measure the baseline. Initial results with `go test -bench . -cpu 1,2` on my machine (two physical cores): BenchmarkContended 56413502 18.3 ns/op BenchmarkContended-2 82533951 33.5 ns/op Note that the 33.5ns/op measure is worse than it appears, because that's wall ns/op, not cpu-ns/op (see golang/go#31884), so the time is actually 67.0ns/op when contended.
|
Looking at improving this documentation now. Considering where best to document - thinking this detail would be best added in a little of the documentation for Would anyone recommend any additional areas to document this? |
|
Change https://go.dev/cl/447136 mentions this issue: |
|
I just submitted https://go-review.googlesource.com/c/go/+/447136 for improving the documentation, PTAL.
That's unfortunate. I think fixing the value would be the best solution here. This doesn't really fall under the Go 1 compatibility promise, does it?
I can send a patch for this. Maybe It also seems like it's possible to overwrite the reported "ns/op" with a better value like this: Perhaps instead of reporting a new metric, we could add a convenience API, e.g. But again, that's also ugly. I'd prefer if the current behavior could be fixed. Regardless of further action, I hope the documentation patch makes sense and can be applied in the meantime. |
I'd suggest adding a |
Updates #31884 Change-Id: Ibad3d31038a8426c0bce61c1726392880f934865 Reviewed-on: https://go-review.googlesource.com/c/go/+/447136 Reviewed-by: Bryan Mills <bcmills@google.com> Run-TryBot: Bryan Mills <bcmills@google.com> Reviewed-by: Benny Siegert <bsiegert@gmail.com> Auto-Submit: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
The way parallel benchmark results are reported is easy to misinterpret, and the correct way to interpret them is not documented. Typically this makes perfectly-scalable benchmarks appear to perform much worse at low parallelism, and makes not-at-all-scalable benchmarks appear to be doing fine. For example, this came up in #31820 (comment)
Specifically, the "ns/op" reported is not CPU-ns/op, it's wall-ns/op. For example, suppose each op takes exactly 100 ns, regardless of parallelism. If the single-threaded benchmark runs for 1 sec, it will execute 10,000,000 ops, so ns/op = 1s/10,000,000 ops = 100 ns/op. But if the same benchmark runs 4-way parallel for 1 sec, it will execute 40,000,000 ops, so ns/op = 1s/40,000,000 = 25 ns/op. (I really wish it didn't work this way...)
Interpreting the results of CPU-bound parallel benchmarks is further complicated by hyper-threading (though this isn't the fault of the testing package).
I don't think we can change the reported ns/op at this point. We could perhaps introduce a new metric for parallel benchmarks. At the very least, we should document this.
The text was updated successfully, but these errors were encountered: