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: generic method values at package scope pin their receiver value #54343

Open
mdempsky opened this issue Aug 8, 2022 · 5 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky commented Aug 8, 2022

https://go.dev/play/p/EO98OQn3OIr

If the generic method value x.M appears at package scope, then x is spilled into a global variable and never gets garbage collected, even if the method value itself is garbage collected.

This happens because for generic method values, stencil.go desugar the method value into a function literal that captures the receiver value after evaluating it. However, the compiler backend (particularly escape analysis) doesn't currently handle package-scope function literals with closure variables. This causes a problem for generic method values that appear in package-scope initialization expressions.

The solution currently taken by stencil.go (and that I've duplicated in unified IR's shaped-based stenciling) is to spill the receiver value to a global variable instead. However, this is suboptimal because it prevents garbage collection.

Strictly speaking, this isn't a correctness issue, because runtime.SetFinalizer warns that variables allocated during package initialization may never be finalized. But it's something that we can do better about.

/cc @golang/compiler

@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 8, 2022
@mdempsky mdempsky added this to the Go1.20 milestone Aug 8, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 8, 2022
@cherrymui
Copy link
Member

cherrymui commented Aug 8, 2022

Maybe it could rewrite to something like

var m func()
func init() { m = New[int]().M }

So it will not be a "package-scoped closure"?

@mdempsky
Copy link
Member Author

mdempsky commented Aug 8, 2022

So the natural time to do the rewrite would be during IR construction, which currently happens before initialization ordering. That rewrite would unfortunately affect when m is initialized.

But along those lines, I think rewriting to this would work without affecting initialization ordering:

var m = func(r *T[int]) func() { return r.M }(New[int]())

And then that could get further rewritten into:

var m = func(r *T[int]) func() { return func() { T[int].M(r, &.dict.T[int]) } }(New[int]())

Thanks for the suggestion.

@gopherbot
Copy link

gopherbot commented Aug 9, 2022

Change https://go.dev/cl/422198 mentions this issue: test: add test for package-scope method value GC

gopherbot pushed a commit that referenced this issue Aug 9, 2022
The Go 1.18 frontend handles package-scope generic method values by
spilling the receiver value to a global temporary variable, which pins
it into memory. This issue isn't present in unified IR, which uses
OMETHVALUE when the receiver type is statically known.

Updates #54343.

Change-Id: I2c4ffeb125a3cf338f949a93b0baac75fff6cd31
Reviewed-on: https://go-review.googlesource.com/c/go/+/422198
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
The Go 1.18 frontend handles package-scope generic method values by
spilling the receiver value to a global temporary variable, which pins
it into memory. This issue isn't present in unified IR, which uses
OMETHVALUE when the receiver type is statically known.

Updates golang#54343.

Change-Id: I2c4ffeb125a3cf338f949a93b0baac75fff6cd31
Reviewed-on: https://go-review.googlesource.com/c/go/+/422198
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
The Go 1.18 frontend handles package-scope generic method values by
spilling the receiver value to a global temporary variable, which pins
it into memory. This issue isn't present in unified IR, which uses
OMETHVALUE when the receiver type is statically known.

Updates golang#54343.

Change-Id: I2c4ffeb125a3cf338f949a93b0baac75fff6cd31
Reviewed-on: https://go-review.googlesource.com/c/go/+/422198
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
@mdempsky
Copy link
Member Author

mdempsky commented Aug 11, 2022

https://go-review.googlesource.com/c/go/+/423074 tested a non-generic version of the test, and it still flaked on arm with "never GC'd": https://storage.googleapis.com/go-build-log/019cddda/linux-arm-aws_8b1e8c2b.log

@gopherbot
Copy link

gopherbot commented Aug 11, 2022

Change https://go.dev/cl/423115 mentions this issue: test: make issue54343.go robust against the tiny allocator

gopherbot pushed a commit that referenced this issue Aug 11, 2022
I structured the test for issue54343.go after issue46725.go, where I
was careful to use `[4]int`, which is a type large enough to avoid the
tiny object allocator (which interferes with finalizer semantics). But
in that test, I didn't note the importance of that type, so I
mistakenly used just `int` in issue54343.go.

This CL switches issue54343.go to use `[4]int` too, and then adds
comments to both pointing out the significance of this type.

Updates #54343.

Change-Id: I699b3e64b844ff6d8438bbcb4d1935615a6d8cc4
Reviewed-on: https://go-review.googlesource.com/c/go/+/423115
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: David Chase <drchase@google.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. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: In Progress
Development

No branches or pull requests

3 participants