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

Comments

@deepilla
Copy link

@deepilla deepilla commented Sep 7, 2018

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
Contributor

@cherrymui cherrymui commented Sep 7, 2018

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

@andybons andybons added this to the Unplanned milestone Sep 7, 2018
@agnivade
Copy link
Contributor

@agnivade agnivade commented Apr 22, 2019

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 mdempsky commented Apr 22, 2019

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

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

@gopherbot gopherbot commented Sep 25, 2019

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

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 25, 2019

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

@gopherbot gopherbot commented Apr 15, 2020

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

@gopherbot gopherbot closed this in 2a2423b Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.