-
Notifications
You must be signed in to change notification settings - Fork 18k
unicode/utf8: make DecodeRune and DecodeLastRune inlineable #48195
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Comments
FWIW, I gave it a shot before: https://go-review.googlesource.com/c/go/+/332770 Unfortunately yes, I really can not see a way to make it work without messing with the inliner (either by changing the heuristics, or by adding special cases) or otherwise adding some kind of annotation. |
Seem to remember commenting on a similar issue, that couldn't do the same with binary.Uvarint(). But now with for loop inlining since added to go, Uvarint() can be made to (replace range use) inline as whole without needing a separate func. |
The inline cost of the following implementation is 83: func DecodeRune(p []byte) (r rune, size int) {
if len(p) > 0 && p[0] < RuneSelf {
r, size = rune(p[0]), 1
} else {
r, size = decodeRune(p)
}
return
} |
And this one is 81: func DecodeRune(p []byte) (r rune, size int) {
if len(p) > 0 && p[0] < RuneSelf {
return rune(p[0]), 1
}
r, size = decodeRune(p)
return
} |
Maybe the inline cost calculation should consider whether or not bound checks are eliminated. BTW, why the inline costs of function calls are so large? |
The ExtraCall budget is 57 plus two arguments we had at least 64 budget, maybe we can force inline by asm code. https://perf.golang.org/search?q=upload:20210922.6
|
Isn't that expected? With inlining, we generally want to get the common case to be inlined (i.e., ASCII), and perform the function call for the less common case (i.e., multibyte Unicode). |
Change https://golang.org/cl/354769 mentions this issue: |
Change https://golang.org/cl/354770 mentions this issue: |
Change https://golang.org/cl/356769 mentions this issue: |
Code like the following is unfortunately very common:
and we have several examples in the standard library itself:
All of these cases ultimate come down to the caller manually performing inlined logic for the ASCII case. We should make it such that
DecodeRune
andDecodeLastRune
are inlineable for the ASCII case. Unfortunately my attempts to do so exceed the inline budget by ever so much:We should find a way to make these inlinable whether by special casing them, clever tricks to make the cost lower, by increasing the inline budget, etc.
\cc @CAFxX @josharian @mdempsky @martisch
The text was updated successfully, but these errors were encountered: