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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: 4% kubelet compile time regression from CL 482356 "cmd/compile: allow more inlining of functions that construct closures" #59547

Closed
prattmic opened this issue Apr 11, 2023 · 7 comments
Closed

cmd/compile: 4% kubelet compile time regression from CL 482356 "cmd/compile: allow more inlining of functions that construct closures" #59547

prattmic opened this issue Apr 11, 2023 · 7 comments
Assignees
@thanm
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Go1.21

Comments

@prattmic
Copy link
Member

https://go.dev/cl/482356 seems to have caused a regression of ~4% for the kubernetes kubelet:

https://perf.golang.org/dashboard/?benchmark=GoBuildKubelet&unit=sec/op#commit39986d28e435d23e1d1dc41b8f16c0cf30181208

The immediate parent of this commit was also tested (f1caf1a), so it does seem to be caused by https://go.dev/cl/482356.

Strangely, our other two build benchmarks (Istio and pkgsite) regressed <1%:

https://perf.golang.org/dashboard/?benchmark=GoBuildIstioctl&unit=sec/op#commit39986d28e435d23e1d1dc41b8f16c0cf30181208
https://perf.golang.org/dashboard/?benchmark=GoBuildFrontend&unit=sec/op#commit39986d28e435d23e1d1dc41b8f16c0cf30181208

cc @thanm to see if this is expected
cc @mdempsky
@prattmic prattmic added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 11, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 11, 2023
@prattmic
Copy link
Member Author

cc @aclements author of original version of this CL

@thanm
Copy link
Contributor

thanm commented Apr 11, 2023

CL 482356 definitely increases the amount of inlining-- a 4% increase is certainly not out of the realm of possibility. I can try running compilebench on just the commit to see what the numbers are there.

@prattmic
Copy link
Member Author

Sweet has a compile benchmark for kubernetes, but no runtime benchmarks. Perhaps all that extra inlining gives a boost to kubernetes performance, which would be great.

@mknyszek mknyszek added this to the Go1.21 milestone Apr 12, 2023
@thanm
Copy link
Contributor

thanm commented Apr 14, 2023

I've reworked the code that marks unreferenced hidden closures as dead (to fix another bug). The new patch marks a lot more stuff dead, so I am hoping that it will help speed up the compiler a bit as well. Will send CL shortly.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/484859 mentions this issue: cmd/compile: rework marking of dead hidden closure functions

gopherbot pushed a commit that referenced this issue Apr 17, 2023
@thanm
cmd/compile: rework marking of dead hidden closure functions
d240226 
This patch generalizes the code in the inliner that marks unreferenced
hidden closure functions as dead. Rather than doing the marking on the
fly (previous approach), this new approach does a single pass at the
end of inlining, which catches more dead functions.

Fixes #59638.
Updates #59404.
Updates #59547.

Change-Id: I54fd63e9e37c9123b08a3e7def7d1989919bba91
Reviewed-on: https://go-review.googlesource.com/c/go/+/484859
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/486377 mentions this issue: cmd/compile: rework marking of dead hidden closure functions

@thanm
Copy link
Contributor

thanm commented May 19, 2023

Kubelet build is now back on track with https://go.dev/cl/492016. Marking this closed.

@thanm thanm closed this as completed May 19, 2023
@golang golang locked and limited conversation to collaborators May 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@thanm thanm

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
4 participants
@mknyszek @prattmic @gopherbot @thanm