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

Closed
mdempsky opened this issue Aug 8, 2022 · 6 comments
Closed

cmd/compile: generic method values at package scope pin their receiver value #54343

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

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

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

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

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
@mdempsky
test: add test for package-scope method value GC
f93b668 
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
@mdempsky @jproberts
test: add test for package-scope method value GC
d9889f9 
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
@mdempsky @jproberts
test: add test for package-scope method value GC
9a7f600 
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

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

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
@mdempsky
test: make issue54343.go robust against the tiny allocator
8743198 
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>
@AndersonQ AndersonQ mentioned this issue Aug 19, 2022
Closed
@mdempsky
Copy link
Member Author

mdempsky commented Nov 8, 2022

This will be fixed in 1.20 with GOEXPERIMENT=unified, and there's a regress test.

@mdempsky mdempsky closed this as completed Nov 8, 2022
@golang golang locked and limited conversation to collaborators Nov 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mdempsky mdempsky

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.20
Development

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