-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/compile: "redo growslice calling convention" caused a 14.7% regression in AppendMsgReplicateDecision #56440
Comments
Definitely not a great benchmark. With a sub-1ns running time, lots of noise. I think we can probably get rid of it. (Anyone know how to do that? Where's the config of what is run?) @dr2chase That said, it does look like there is something happening here. The inner loop looks like it has an extra bounds check in it. That may be fixable, I'd need to extract a repro from this giant repository first. |
I can reproduce with:
|
The default bent benchmarks are here: https://cs.opensource.google/go/x/benchmarks/+/3255f073:cmd/bent/configs/benchmarks-50.toml It needs a decoder ring to tell you what benchmark+package that is, but it's in minio. A nanosecond's not great, but this was a regression, right? I try to have a diversity of benchmarks, but also, I am willing to ignore a few regressions if the big picture is okay. |
Simple repro:
The generated code is a bit worse after my CL because the compiler doesn't realize that |
Change https://go.dev/cl/445875 mentions this issue: |
FWIW, this did not seem to fix the benchmark: https://perf.golang.org/dashboard/?benchmark=AppendMsgReplicateDecision-8&days=90&end=2022-10-28T15%3A45 The most recent run is for https://go.dev/cl/444715, which was submitted after https://go.dev/cl/445875. |
You're right, it fixes my simple repro but not the original repro. |
Change https://go.dev/cl/446277 mentions this issue: |
Fixes a performance regression due to CL 418554. Fixes golang#56440 Change-Id: I6ff152e9b83084756363f49ee6b0844a7a284880 Reviewed-on: https://go-review.googlesource.com/c/go/+/445875 Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Keith Randall <khr@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com>
The recently added rule only works before decomposing slices. Add a rule that works after decomposing slices. The reason we need the latter is because although the length may be a constant, it can be hidden inside a slice that is not constant (its pointer or capacity might be changing). By applying this optimization after decomposing slices, we can find more cases where it applies. Fixes golang#56440 Change-Id: I0094e59eee3065ab4d210defdda8227a6e897420 Reviewed-on: https://go-review.googlesource.com/c/go/+/446277 Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Keith Randall <khr@google.com>
We should determine if this is worth fixing, if the benchmark is too noisy (or not really useful), or if there's a real bug here.
The text was updated successfully, but these errors were encountered: