Skip to content
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: simple functions in compress/flate, inlined in go1.10 no longer inlined #27621

Closed
laboger opened this issue Sep 11, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@laboger
Copy link
Contributor

commented Sep 11, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

tip

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

ppc64le Ubuntu 16.04

What did you do?

Inspecting code from compress/flate, noticed some simple functions that I would expect to be inlined that are not.

What did you expect to see?

Functions compress/flate.byLiteral.Len, compress/flate.byLiteral.Less, compress/flate.byLiteral.Swap, compress/flate.byFreq.Len, compress/flate.byFreq.Less, compress/flate.byFreq.Swap inlined as was done in go 1.10.

What did you see instead?

No inlining for these functions on the latest branch or in go 1.11.

I just built the test in compress/flate and looked at the code. Not sure if you need more information.
I did add the -m=2 option and the output said those functions should be inlined.

../../compress/flate/huffman_code.go:321:6: can inline byLiteral.Len as: method(byLiteral) func() int { return len(s) }
../../compress/flate/huffman_code.go:323:6: can inline byLiteral.Less as: method(byLiteral) func(int, int) bool { return s[i].literal < s[j].literal }
../../compress/flate/huffman_code.go:327:6: can inline byLiteral.Swap as: method(byLiteral) func(int, int) { s[i], s[j] = s[j], s[i] }
../../compress/flate/huffman_code.go:336:6: can inline byFreq.Len as: method(byFreq) func() int { return len(s) }
../../compress/flate/huffman_code.go:338:6: can inline byFreq.Less as: method(byFreq) func(int, int) bool { if s[i].freq == s[j].freq { return s[i].literal < s[j].literal }; return s[i].freq < s[j].freq }
../../compress/flate/huffman_code.go:345:6: can inline byFreq.Swap as: method(byFreq) func(int, int) { s[i], s[j] = s[j], s[i] }

@laboger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

Forgot to mention: they aren't being inlined in the corresponding functions like these:
compress/flate.(*byLiteral).Len
compress/flate.(*byLiteral).Less
compress/flate.(*byLiteral).Swap
compress.flate.(*byFreq).Len
compress.flate.(*byFreq).Less
compress.flate.(*byFreq).Swap

@TocarIP

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

Looks like #25338

@gopherbot

This comment has been minimized.

Copy link

commented Sep 17, 2018

Change https://golang.org/cl/135697 mentions this issue: cmd/compile/internal/gc: inline autogenerated (*T).M wrappers

@andybons

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

@laboger if you apply the above fix locally, is the issue resolved? If so, I'll close this as a dupe of #25338

@andybons andybons added this to the Unplanned milestone Sep 26, 2018

@laboger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

I tried the fix above and the functions are now inlined.

@andybons

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

@laboger thank you. Closing as a dupe of #25338.

@andybons andybons closed this Sep 26, 2018

gopherbot pushed a commit that referenced this issue Oct 16, 2018

cmd/compile/internal/gc: inline autogenerated (*T).M wrappers
Currently all inlining of autogenerated wrappers is disabled,
because it causes build failures, when indexed export format is enabled.
Turns out we can reenable it for common case of (*T).M wrappers.
This fixes most performance degradation of 1.11 vs 1.10.

encoding/binary:
name                    old time/op   new time/op   delta
ReadSlice1000Int32s-6    14.8µs ± 2%   11.5µs ± 2%  -22.01%  (p=0.000 n=10+10)
WriteSlice1000Int32s-6   14.8µs ± 2%   11.7µs ± 2%  -20.95%  (p=0.000 n=10+10)

bufio:
name           old time/op    new time/op    delta
WriterFlush-6    32.4ns ± 1%    28.8ns ± 0%  -11.17%  (p=0.000 n=9+10)

sort:
SearchWrappers-6       231ns ± 1%   231ns ± 0%     ~     (p=0.129 n=9+10)
SortString1K-6         365µs ± 1%   298µs ± 1%  -18.43%  (p=0.000 n=9+10)
SortString1K_Slice-6   274µs ± 2%   276µs ± 1%     ~     (p=0.105 n=10+10)
StableString1K-6       490µs ± 1%   373µs ± 1%  -23.73%  (p=0.000 n=10+10)
SortInt1K-6            210µs ± 1%   142µs ± 1%  -32.69%  (p=0.000 n=10+10)
StableInt1K-6          243µs ± 0%   151µs ± 1%  -37.75%  (p=0.000 n=10+10)
StableInt1K_Slice-6    130µs ± 1%   130µs ± 0%     ~     (p=0.237 n=10+8)
SortInt64K-6          19.9ms ± 1%  13.5ms ± 1%  -32.32%  (p=0.000 n=10+10)
SortInt64K_Slice-6    11.5ms ± 1%  11.5ms ± 1%     ~     (p=0.912 n=10+10)
StableInt64K-6        21.5ms ± 0%  13.5ms ± 1%  -37.30%  (p=0.000 n=9+10)
Sort1e2-6              108µs ± 2%    83µs ± 3%  -23.26%  (p=0.000 n=10+10)
Stable1e2-6            218µs ± 0%   161µs ± 1%  -25.99%  (p=0.000 n=8+9)
Sort1e4-6             22.6ms ± 1%  16.8ms ± 0%  -25.45%  (p=0.000 n=10+7)
Stable1e4-6           67.6ms ± 1%  49.7ms ± 0%  -26.48%  (p=0.000 n=10+10)
Sort1e6-6              3.44s ± 0%   2.55s ± 1%  -26.05%  (p=0.000 n=8+9)
Stable1e6-6            13.7s ± 0%    9.9s ± 1%  -27.68%  (p=0.000 n=8+10)

Fixes #27621
Updates #25338

Change-Id: I6fe633202f63fa829a6ab849c44d7e45f8835dff
Reviewed-on: https://go-review.googlesource.com/c/135697
Run-TryBot: Ilya Tocar <ilya.tocar@intel.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.