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: method values cause receiver to escape to the heap #27557

Closed
deepilla opened this issue Sep 7, 2018 · 6 comments
Closed

cmd/compile: method values cause receiver to escape to the heap #27557

deepilla opened this issue Sep 7, 2018 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@deepilla
Copy link

deepilla commented Sep 7, 2018 •
edited

What did you do?

In this minimal code example, functions f1 and f2 both take an argument t and call its foo method. t1 calls the method directly; t2 uses a method value.

package escape

type T struct{}

func (pt *T) foo() {}

func f1(t T) {
    t.foo()
}

func f2(t T) {
    f := t.foo
    f()
}

See this golang-nuts thread for background.

What did you expect to see?

The value t allocated on the stack in both functions.

What did you see instead?

In function f2, t escapes to the heap.

$ go build -gcflags="-l -m=2"

./escape.go:5:7: (*T).foo pt does not escape
./escape.go:8:3: f1 t does not escape
./escape.go:12:8: t escapes to heap
./escape.go:12:8:       from t.foo (call part) at ./escape.go:12:8
./escape.go:11:9: moved to heap: t
./escape.go:12:8: f2 t.foo does not escape

I understand that method values involve making a copy of the receiver (a pointer in this case). But if the compiler can figure out that t doesn't escape from f1, why can't it do the same for f2?

Does this issue reproduce with the latest release (go1.11)?

Yes.

System details

go version go1.11 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/deepilla/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/deepilla/Projects/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.11 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.11
uname -sr: Linux 4.18.7-arch1-1-ARCH
LSB Version:	1.4
Distributor ID:	Arch
Description:	Arch Linux
Release:	rolling
Codename:	n/a
/usr/lib/libc.so.6: GNU C Library (GNU libc) stable release version 2.28.
@cherrymui
Copy link
Member

We probably can treat the method expression the same way as closures, which will probably solve this.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 7, 2018
@andybons andybons added this to the Unplanned milestone Sep 7, 2018
@agnivade
Copy link
Contributor

This seems fixed with the latest master (go version devel +9dce58d30d Wed Apr 17 19:09:15 2019 +0000 linux/amd64). But I still see a moved to heap: t. Not sure, what that means if t.foo does not escape.

gotip tool compile -m bound_checker.go 
bound_checker.go:5:6: can inline (*T).foo
bound_checker.go:7:6: can inline f1
bound_checker.go:8:7: inlining call to (*T).foo
bound_checker.go:5:6: can inline (*T).foo-fm
bound_checker.go:5:6: inlining call to (*T).foo
bound_checker.go:5:7: (*T).foo pt does not escape
bound_checker.go:11:9: moved to heap: t
bound_checker.go:12:8: f2 t.foo does not escape

@mdempsky ?

@mdempsky
Copy link
Member

mdempsky commented Apr 22, 2019 •
edited

@agnivade Thanks for checking. The moved to heap: t message here is the critical one that this issue is about eliminating.

To explain the diagnostics, moved to heap: t means exactly that: that the t parameter will be heap allocated. Meanwhile, the t.foo does not escape message is referring to the implicit closure created for the method value.

This issue is about how OCALLPART is analyzed:

go/src/cmd/compile/internal/gc/escape.go

Lines 519 to 526 in 744fcfe

case OCALLPART:
e.spill(k, n)
// esc.go says "Contents make it to memory, lose
// track." I think we can just flow n.Left to our
// spilled location though.
// TODO(mdempsky): Try that.
e.assignHeap(n.Left, "call part", n)

Currently we always flow the receiver argument (i.e., t in t.foo) directly to the heap. What we actually need to do is flow it to the closure and to the receiver parameter. (My comment there is actually wrong, because I didn't consider the latter.)

For example, we need to make sure t still moves to heap here:

package p

var sink interface{}

type T struct{}

func (t *T) escape() { sink = t }
func (t *T) returns() *T { return t }

func _() {
    var t T // must heap allocate
    f := t.escape
    f()
}

func _() {
    var t T // must heap allocate
    f := t.returns
    sink = f()
}

@gopherbot
Copy link

Change https://golang.org/cl/197137 mentions this issue: test: add regress test for #27557

@mdempsky
Copy link
Member

What we actually need to do is flow it to the closure and to the receiver parameter.

One additional caveat I realized looking at this again is that scc.go needs to be updated to recognize OCALLPART.

gopherbot pushed a commit that referenced this issue Sep 25, 2019
@mdempsky
test: add regress test for #27557
f346a4c 
This commit just adds a regress test for a few of the important corner
cases that I identified in #27557, which turn out to not be tested
anywhere.

While here, annotate a few of the existing test cases where we could
improve escape analysis.

Updates #27557.

Change-Id: Ie57792a538f7899bb17915485fabc86100f469a3
Reviewed-on: https://go-review.googlesource.com/c/go/+/197137
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/228263 mentions this issue: cmd/compile: better escape analysis of method values

@golang golang locked and limited conversation to collaborators Apr 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@mdempsky @andybons @deepilla @agnivade @gopherbot @cherrymui