cmd/compile: for-loops cannot be inlined #14768
Labels
Milestone
Comments
|
For reference, the easy part (inlining |
|
Thanks for doing this; it was not exactly the results I was hoping for ;) Are you aware of which loops in HTTPClientServer were inlined? I'm curious about the 5% increase in time. |
|
grepping for "http" in the No idea which one of these is the culprit for the slowdown. |
mvdan
changed the title
wallyqs
added a commit
to wallyqs/nats-server
that referenced
this issue
Sep 8, 2017
Using a goto based loop makes it become a leaf function which can be inlined, making us get a slight performance increase in the fast path. See: golang/go#14768
wallyqs
added a commit
to wallyqs/nats-server
that referenced
this issue
Sep 8, 2017
Using a goto based loop makes it become a leaf function which can be inlined, making us get a slight performance increase in the fast path. See: golang/go#14768
This was referenced Nov 8, 2017
|
Any news on this? |
|
I will give it a look, I have a "adjust inlining parameters" CL up for review and I can try adding this one top see if it helps or hurts. |
|
Turns out that inlining for loops tickles (at least) two other bugs, don't start the party yet. |
|
@dr2chase Is it now ok to enable inlining of functions with for loops? |
|
In theory, yes. I am doing some benchmarking right now to see what the overall impact is likely to be, with a very naive cost model (i.e., a default cost model). It also requires change to some test output, or some noinline annotations. I don't know if this qualifies for 1.12; though it is technically "an open bug" and the change is trivial. |
|
Change https://golang.org/cl/148777 mentions this issue: |
|
Given that the fix is trivial and already somewhat-ready - any chance this could go in for 1.13? :) |
|
When I tried to revive it, there was some weird failure that it caused. |
|
Any chance for this being fixed sometime soon? I would love to undo my re-writes where I wrote my loops using goto instead of for, so they can be inlined. |
|
Not in 1.13, maybe 1.14. |
|
Go 1.14 is long gone, any chance this is getting fixed? Right now I'm manually inlining functions :/ |
|
@elichai |
|
Change https://golang.org/cl/256459 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
Oct 15, 2020
We already allow inlining "if" and "goto" statements, so we might as well allow "for" loops too. The majority of frontend support is already there too. The critical missing feature at the moment is that inlining doesn't properly reassociate OLABEL nodes with their control statement (e.g., OFOR) after inlining. This eventually causes SSA construction to fail. As a workaround, this CL only enables inlining for unlabeled "for" loops. It's left to a (yet unplanned) future CL to add support for labeled "for" loops. The increased opportunity for inlining leads to a small growth in binary size. For example: $ size go.old go.new text data bss dec hex filename 9740163 320064 230656 10290883 9d06c3 go.old 9793399 320064 230656 10344119 9dd6b7 go.new Updates #14768. Fixes #41474. Change-Id: I827db0b2b9d9fa2934db05caf6baa463f0cd032a Reviewed-on: https://go-review.googlesource.com/c/go/+/256459 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Go Bot <gobot@golang.org> Trust: Matthew Dempsky <mdempsky@google.com> Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
|
Change https://golang.org/cl/263037 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
Oct 16, 2020
In https://golang.org/cl/16528, a goto loop was chosen over a regular for loop since that would make the function inlinable. Thanks to the recent https://golang.org/cl/256459, for loops without a label can now be inlined. So we can undo the workaround and simplify the code. Also add the function to TestIntendedInlining, which passes both before and after the change, as expected. For #14768. Change-Id: Ie5df55a6bcb07c538ca331eef2f908807ff0b516 Reviewed-on: https://go-review.googlesource.com/c/go/+/263037 Trust: Daniel Martí <mvdan@mvdan.cc> Run-TryBot: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Go Bot <gobot@golang.org>
|
As of 30ba798, it seems that For example: func consumeWhitespace(b []byte) int {
for i, c := range b {
if !(c == ' ' || c == '\t' || c == '\r' || c == '\n') {
return i
}
}
return 0
}cannot be inlined, but: func consumeWhitespace(b []byte) int {
for i := 0; i < len(b); i++ {
if c := b[i]; !(c == ' ' || c == '\t' || c == '\r' || c == '\n') {
return i
}
}
return 0
}is inlineable. I'm not sure if this is deliberate or an oversight. |
A little of both. Some range loops desugar into non-trivial code (e.g., ranging over maps), so I punted on this because I expected it would be too complex... but in retrospect, all of that complexity happens after inlining anyway, so it would probably be pretty trivial actually. We should be able to easily address this for next release, along with labeled for loops. |
|
Change https://golang.org/cl/273270 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
Nov 25, 2020
…ontinue In CL 145200, I changed OBREAK, OCONTINUE, OGOTO, and OLABEL to just use Sym instead of Node. However, within the export data, I forgot to update the code for OBREAK and OCONTINUE. This isn't currently an issue because the inliner currently disallows these anyway, but it'll be an issue in the future once we add support for inlining them. Also, Russ independently ran into it in CL 273246. Updates #14768. Change-Id: I94575df59c08a750b0dce1d3ce612aba7bfeeb76 Reviewed-on: https://go-review.googlesource.com/c/go/+/273270 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Go Bot <gobot@golang.org> Trust: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Russ Cox <rsc@golang.org>
|
Change https://golang.org/cl/355497 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
Oct 18, 2021
After CL 349012 and CL 350911, we can fully handle these labeled statements, so we can allow them when inlining. Updates #14768 Change-Id: I0ab3fd3f8d7436b49b1aedd946516b33c63f5747 Reviewed-on: https://go-review.googlesource.com/c/go/+/355497 Run-TryBot: David Chase <drchase@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Dan Scales <danscales@google.com> Reviewed-by: David Chase <drchase@google.com> Trust: Dan Scales <danscales@google.com>
|
Change https://golang.org/cl/356869 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
Oct 28, 2021
Updates #14768 Change-Id: I33831f616eae5eeb099033e2b9cf90fa70d6ca86 Reviewed-on: https://go-review.googlesource.com/c/go/+/356869 Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Dan Scales <danscales@google.com> Trust: Dan Scales <danscales@google.com> Trust: Alberto Donizetti <alb.donizetti@gmail.com>
|
Is this done? It looks all kinds of loops are capable of being inlined now. |
|
Yes, thanks for pointing out. This is done now for 1.18. The missing cases (range loops and loops with labeled breaks/continues) are now inlineable, so all |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Using
go devel +ed4a27aCurrently, functions with for-loops are not inline-able, would be nice if they did. Not long ago, I used a non-idiomatic goto in order to cheat around the inliner.
@dr2chase
The text was updated successfully, but these errors were encountered: