cmd/compile: Allocations leak into outer scope

[holiman]

What version of Go are you using (go version)?

go version go1.14.4 linux/amd64

Does this issue reproduce with the latest release?

I believe I'm on the latest release

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build872077689=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I hit a strange issue while doing some benchmarking in go-ethereum . It turned out that the allocations for a particular operation went through the roof, for no good reason as far as I could tell.

I narrowed down the problem as best I could. this gist contains a tiny VM + benchmarking code which shows two cases: one where allocations are made for a branch that is never taken, and another where the unused branch is commented out, and allocations are "sane".

It's quite a difference:

BenchmarkAllocs/bad-6         	     100	  10296649 ns/op	 6400030 B/op	  200000 allocs/op
BenchmarkAllocs/good-6        	   34618	     29632 ns/op	       0 B/op	       0 allocs/op

What did you expect to see?

That allocations inside a scope which was never entered are not made.

What did you see instead?

That allocations seems to be done outside the scope, directly after the entry into the method.

[bradfitz]

/cc @mdempsky

[mdempsky]

(I'm assuming that in your actual application the ok := true is something that's actually not statically predictable. That is, that your issue is more than you think we should apply better dead code elimination here.)

In BadOperation, the call to caller.Address() means that caller might escape to the heap (because we're invoking an interface method, and we don't know what that method does with the receiver). So the Account{} in AccountRef(Account{}) has to be heap allocated because of this. I don't think there's anything we can do about this.

The contract.CodeAddr = &addr part is a bit subtler, and there's arguably room for improvement. There's two related issues here:

1. Here contract is a pointer, so contract.CodeAddr = ... is an assignment through a pointer, so we conservatively treat it as a store to the heap. You can avoid this by changing contract := &Contract{...} to contract := Contract{...}.

   Theoretically escape analysis could recognize that the pointer is actually always pointing to a composite literal. This would be easier to do on SSA, but there's basic patterns like this we could recognize even in the Node AST.

2. The other aspect is that if the address of a local variable leaks anywhere (even in a path we don't take), we conservatively allocate the object on the heap. We could theoretically move the object to the heap only once we know it escapes, but that has its own complexities.

[holiman]

That is, that your issue is more than you think we should apply better dead code elimination here.

Yes, exactly!

Thanks for the detailed explanation! It helped me to figure out how to work around this -- in this case, I took a copy of the addr so that the pointer reference does not leak. ethereum/go-ethereum@e02eb02#diff-1ad751e400b74d9c7234ae3134edccf0R359 .

Three pretty small changes (three commits ending with that one) reduced the allocations dramatically.

As far as I'm concerned, this ticket can be closed (unless you want it to remain open since " there's arguably room for improvement")

[gopherbot]

Change https://golang.org/cl/262937 mentions this issue: cmd/compile: handle escape analysis assignment through pointer more precisely

[mdempsky]

@cuonglm found a fix for this in CL 262937, but it appears to not help real world Go code much. E.g., k8s.io/kubernetes/cmd/kubelet is only able to stack allocate one new variable.

Right now, the staticValue optimization it relies on is less efficient than it could be (it already showed up as a measurable compiler performance hit in fixing #41474). I'm thinking we should optimize that first. Once the fix doesn't impact compile-time, I'm okay with revisiting the optimization.