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

Jump to bottom

cmd/compile: deduplicate wrappers for promoted methods #57916

Open
mdempsky opened this issue Jan 19, 2023 · 3 comments
Open

cmd/compile: deduplicate wrappers for promoted methods #57916

mdempsky opened this issue Jan 19, 2023 · 3 comments
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.
Milestone
Backlog

Comments

@mdempsky
Copy link
Member

mdempsky commented Jan 19, 2023 •
edited

Given:

type X int
func (*X) m() {}

type A struct { X }
type B struct { b int32; X }
type C struct { c uint32; X }

the compiler currently synthesizes method wrappers for the promoted methods:

func (a *A) m() { a.X.m() }
func (b *B) m() { b.X.m() }
func (c *C) m() { c.X.m() }

But we only really need 1 wrapper:

  • A can just use X's method directly, because the X field is at offset 0 anyway.
  • B and C could use a single shared wrapper, because they need the same pointer adjustment before tail calling to (*X).m.

This would save some executable size, but I also think it could help slightly with CPU I-cache and branch predictions. For example, within the compiler itself, ir.Node's Op and Pos methods are always actually implemented by ir.miniNode (which we always embed at offset 0). But we create a wrapper method for each ir.Node implementation, which means (caveat: hypothesizing) a bunch of identical methods all end up taking up I-cache space and the branch predictor has to guess which one is going to be called.
@mdempsky mdempsky added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 19, 2023
@mdempsky mdempsky added this to the Backlog milestone Jan 19, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 19, 2023
@cherrymui
Copy link
Member

For direct call of A.m, we should be able to inline it and the caller would call A.X.m directly (same for B and C). (The bottom-up inlining strategy might mess this up in some cases?) So I guess this is mostly for methods used as function pointers (interface methods, method expressions)? Sounds like a reasonable thing to do.

Currently the wrappers are generated in the AST form. If we deduplicate the wrappers the AST would not be well typed. Maybe this is fine?

Another possibility: maybe we could allow link-time deduplication of compiler-generated functions. I think this might work if we mark them content-addressable symbols. So functions with identical bodies could be deduplicated. We'd want to limit it to compiler-generated functions, as user-defined functions generally have different line numbers, or we could add the line numbers to the content hash. Also compiler-generated functions are generally hidden from traceback.

@mdempsky
Copy link
Member Author

Yeah, direct calls are already always dispatched directly to the original, promoted method. (Caveat: this is specific to cmd/compile, gccgo seems to behave differently in some cases; e.g., #52025.)

As for generating a well-typed wrapper, I think something like this is fine:

func `(*X)+4.m`(p unsafe.Pointer) {
    x := (*X)(unsafe.Add(p, 4))
    x.m()
}

The function would only end up referenced in the rtype and itabs anyway, so it just needs to be ABI compatible with the current wrappers.

maybe we could allow link-time deduplication of compiler-generated functions.

Yeah, that seems viable too.

@cherrymui
Copy link
Member

Okay, if the wrapper is never called directly even in AST, then the type is not an issue. I was worrying something like calling a (*B).m(*B) with a *C arg.

I can try and see if content-addressable symbol works for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

No branches or pull requests
3 participants
@mdempsky @gopherbot @cherrymui