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: Allocations leak into outer scope #39514

Open
holiman opened this issue Jun 10, 2020 · 3 comments
Open

cmd/compile: Allocations leak into outer scope #39514

holiman opened this issue Jun 10, 2020 · 3 comments
Milestone

Comments

@holiman
Copy link

@holiman holiman commented Jun 10, 2020

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
Copy link
Contributor

@bradfitz bradfitz commented Jun 10, 2020

/cc @mdempsky

@holiman holiman changed the title Allocation leaks into outer scope Allocations leak into outer scope Jun 10, 2020
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 10, 2020

(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
Copy link
Author

@holiman holiman commented Jun 11, 2020

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

@toothrot toothrot changed the title Allocations leak into outer scope cmd/compile: Allocations leak into outer scope Jun 12, 2020
@toothrot toothrot added this to the Backlog milestone Jun 12, 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
4 participants
You can’t perform that action at this time.