cmd/compile: runtime.KeepAlive causes useless extra run-time overhead, even though it is an intrinsic? #33442
Labels
compiler/runtime
Issues related to the Go compiler and/or runtime.
NeedsDecision
Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Does this issue reproduce with the latest release?
Did not check.
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
I converted the benchmarks of the math package from the standard library to use runtime.KeepAlive instead of package level exported sink variables [1]. The rationale was to get more accurate and more robust benchmarks. See [0] for background (the more broad motivation for this change to package math was to check if runtime.KeepAlive is a possible substitute for the proposed formallyRead builtin func). For accuracy runtime.KeepAlive should not cause a run-time overhead, which I expected to be the case. For this issue I considered and compared only the effect on BenchmarkSignbit:
What did you expect to see?
I expected that if the change caused any significant performance differences, they should be speedups. This is because I expected runtime.KeepAlive not to cause a run time overhead.
What did you see instead?
In a lot of the benchmarks for the math package considerable slowdowns happened. In the following data for the changed code (with KeepAlive) one can see a weird sequence of four instructions MOVZX, LEAQ, ADDQ, MOVQ. The MOVQ writes to the stack. In essence, I think these four instruction spill to the stack the address of either the byte 0x00 or 0x01 from the runtime.staticbytes array, which seems like a useless thing to do.
Here is the disassembly and performance data for BenchmarkSignbit (outputs of objdump, pprof, and radare2) before and after the change:
This issue is maybe a little related: [2]
An unrelated possible issue visible in the disassembly is the MOVSD_XMM followed by the MOVQ. Could those two instructions not be replaced by a single MOVQ?
[0] #33325
[1] https://go-review.googlesource.com/c/go/+/188437
[2] #32115
The text was updated successfully, but these errors were encountered: