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: PGO devirtualization follow-ups #61577

Closed
2 of 5 tasks
prattmic opened this issue Jul 25, 2023 · 9 comments
Closed
2 of 5 tasks

cmd/compile: PGO devirtualization follow-ups #61577

prattmic opened this issue Jul 25, 2023 · 9 comments
Assignees
@prattmic
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.22

Comments

@prattmic
Copy link
Member

prattmic commented Jul 25, 2023 •
edited
Loading

https://go.dev/cl/492436 implements the bare minimum of a PGO devirtualization implementation. There are still numerous limitations that we would like address shortly:

cc @cherrymui @aclements
@prattmic prattmic added this to the Go1.22 milestone Jul 25, 2023
@prattmic prattmic self-assigned this Jul 25, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 25, 2023
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 25, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/497175 mentions this issue: cmd/compile: lookup indirect callees from export data for devirtualization

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/529557 mentions this issue: cmd/internal/objabi: add inverse of PathToPrefix

gopherbot pushed a commit that referenced this issue Oct 13, 2023
@prattmic
cmd/internal/objabi: add inverse of PathToPrefix
696fb5e 
Add PrefixToPath, which can be used to convert a package path in a
symbol name back to the original package path.

For #61577.

Change-Id: Ifbe8c852a7f41ff9b81ad48b92a26a0e1b046777
Reviewed-on: https://go-review.googlesource.com/c/go/+/529557
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Oct 13, 2023
@prattmic
cmd/compile: lookup indirect callees from export data for devirtualiz…
bfb8924 
…ation

Today, the PGO IR graph only contains entries for ir.Func loaded into
the package. This can include functions from transitive dependencies,
but only if they happen to be referenced by something in the current
package. If they are not referenced, noder never bothers to load them.

This leads to a deficiency in PGO devirtualization: some callee methods
are available in transitive dependencies but do not devirtualize because
they happen to not get loaded from export data.

Resolve this by adding an explicit lookup from export data of callees
mentioned in the profile.

I have chosen to do this during loading of the profile for simplicity:
the PGO IR graph always contains all of the functions we might need.
That said, it isn't strictly necessary. PGO devirtualization could do
the lookup lazily if it decides it actually needs a method. This saves
work at the expense of a bit more complexity, but I've chosen the
simpler approach for now as I measured the cost of this as significantly
less than the rest of PGO loading.

For #61577.

Change-Id: Ieafb2a549510587027270ee6b4c3aefd149a901f
Reviewed-on: https://go-review.googlesource.com/c/go/+/497175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
yunginnanet pushed a commit to yunginnanet/go that referenced this issue Oct 20, 2023
@prattmic @yunginnanet
cmd/internal/objabi: add inverse of PathToPrefix
41871c1 
Add PrefixToPath, which can be used to convert a package path in a
symbol name back to the original package path.

For golang#61577.

Change-Id: Ifbe8c852a7f41ff9b81ad48b92a26a0e1b046777
Reviewed-on: https://go-review.googlesource.com/c/go/+/529557
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
yunginnanet pushed a commit to yunginnanet/go that referenced this issue Oct 20, 2023
@prattmic @yunginnanet
cmd/compile: lookup indirect callees from export data for devirtualiz…
117517d 
…ation

Today, the PGO IR graph only contains entries for ir.Func loaded into
the package. This can include functions from transitive dependencies,
but only if they happen to be referenced by something in the current
package. If they are not referenced, noder never bothers to load them.

This leads to a deficiency in PGO devirtualization: some callee methods
are available in transitive dependencies but do not devirtualize because
they happen to not get loaded from export data.

Resolve this by adding an explicit lookup from export data of callees
mentioned in the profile.

I have chosen to do this during loading of the profile for simplicity:
the PGO IR graph always contains all of the functions we might need.
That said, it isn't strictly necessary. PGO devirtualization could do
the lookup lazily if it decides it actually needs a method. This saves
work at the expense of a bit more complexity, but I've chosen the
simpler approach for now as I measured the cost of this as significantly
less than the rest of PGO loading.

For golang#61577.

Change-Id: Ieafb2a549510587027270ee6b4c3aefd149a901f
Reviewed-on: https://go-review.googlesource.com/c/go/+/497175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/539698 mentions this issue: cmd/compile: move FuncPC intrinsic handling to common helper

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/539699 mentions this issue: cmd/compile: initial function value devirtualization

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/540258 mentions this issue: cmd/compile: support lookup of functions from export data

gopherbot pushed a commit that referenced this issue Nov 10, 2023
@prattmic @gopherbot
cmd/compile: move FuncPC intrinsic handling to common helper
e323e7d 
CL 539699 will need to do the equivalent of
internal/abi.FuncPCABIInternal to get the PC of a function value for the
runtime devirtualization check.

Move the FuncPC expression creation from the depths of walk to a
typecheck helper so it can be reused in both places.

For #61577.

Change-Id: I76f333157cf0e5fd867b41bfffcdaf6f45254707
Reviewed-on: https://go-review.googlesource.com/c/go/+/539698
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Nov 13, 2023
@prattmic
cmd/compile: initial function value devirtualization
fb6ff1e 
Today, PGO-based devirtualization only applies to interface calls. This
CL extends initial support to function values (i.e., function/closure
pointers passed as arguments or stored in a struct).

This CL is a minimal implementation with several limitations.

* Export data lookup of function value callees not implemented
  (equivalent of CL 497175; done in CL 540258).
* Callees must be standard static functions. Callees that are closures
  (requiring closure context) are not supported.

For #61577.

Change-Id: I7d328859035249e176294cd0d9885b2d08c853f6
Reviewed-on: https://go-review.googlesource.com/c/go/+/539699
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Nov 13, 2023
@prattmic
cmd/compile: support lookup of functions from export data
42bd21b 
As of CL 539699, PGO-based devirtualization supports devirtualization of
function values in addition to interface method calls. As with CL
497175, we need to explicitly look up functions from export data that
may not be imported already.

Symbol naming is ambiguous (`foo.Bar.func1` could be a closure or a
method), so we simply attempt to do both types of lookup. That said,
closures are defined in export data only as OCLOSURE nodes in the
enclosing function, which this CL does not yet attempt to expand.

For #61577.

Change-Id: Ic7205b046218a4dfb8c4162ece3620ed1c3cb40a
Reviewed-on: https://go-review.googlesource.com/c/go/+/540258
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/546935 mentions this issue: doc: add release notes for PGO improvements

gopherbot pushed a commit that referenced this issue Dec 4, 2023
@prattmic
doc: add release notes for PGO improvements
818de27 
For #61577.
For #61422.

Change-Id: I575bf657fb36bd7103c73620bb2371d0f490af20
Reviewed-on: https://go-review.googlesource.com/c/go/+/546935
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@And-ZJ
Copy link
Contributor

And-ZJ commented Dec 12, 2023

Hello, I have a question, I found that devirtualization may not be done if the type is not exported.
Will this approach be considered: if a hot method is found, the type that is not exported will be exported? Or some other way? Is it worth it?
Is it discussed in other issues? Sorry, I may not have noticed. Thank you!

@prattmic
Copy link
Member Author

Functions/types can end up in export data even if they are not explicitly exported for a variety of reasons, which would would enable devirtualization, however they are not guaranteed to be included. We have discussed this problem a bit, but not filed an issue, so I've filed #64676.

@prattmic
Copy link
Member Author

Remaining known issues filed as separate issues:

@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Dec 12, 2023
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@prattmic @ezz-no
doc: add release notes for PGO improvements
d07dfc8 
For golang#61577.
For golang#61422.

Change-Id: I575bf657fb36bd7103c73620bb2371d0f490af20
Reviewed-on: https://go-review.googlesource.com/c/go/+/546935
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@golang golang locked and limited conversation to collaborators Dec 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@prattmic prattmic

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.22
Development

No branches or pull requests
4 participants
@mknyszek @prattmic @gopherbot @And-ZJ