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: 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
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@prattmic
Copy link
Member

prattmic commented Jul 25, 2023

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

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

@gopherbot
Copy link

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
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
…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
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
…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

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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
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
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
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

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

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

4 participants