testing: parallel benchmark results are poorly documented

[aclements]

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.

[rpickz]

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 testing.B.RunParallel and within the summary of the testing package as a note.

Would anyone recommend any additional areas to document this?

[gopherbot]

Change https://go.dev/cl/447136 mentions this issue: testing: Document RunParallel ns/op behavior

[felixge]

I just submitted https://go-review.googlesource.com/c/go/+/447136 for improving the documentation, PTAL.

I don't think we can change the reported ns/op at this point.

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?

We could perhaps introduce a new metric for parallel benchmarks.

I can send a patch for this. Maybe real-ns/op?

It also seems like it's possible to overwrite the reported "ns/op" with a better value like this:

start := time.Now()
b.RunParallel(func(p *testing.PB) {
	// ...
})

nsOp := float64(time.Since(start).Nanoseconds()) / (float64(b.N) / float64(runtime.GOMAXPROCS(-1)))
b.ReportMetric(nsOp, "ns/op")

Perhaps instead of reporting a new metric, we could add a convenience API, e.g. b.CorrectParallelMetrics().

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.

[puzpuzpuz]

I can send a patch for this. Maybe real-ns/op?

I'd suggest adding a ops/s metric (throughput) along with the existing one (ns/op). This should make the output much more intuitive.