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: unclear escape analysis #36504

Closed
FiloSottile opened this issue Jan 10, 2020 · 6 comments
Closed

cmd/compile: unclear escape analysis #36504

FiloSottile opened this issue Jan 10, 2020 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@FiloSottile
Copy link
Contributor

FiloSottile commented Jan 10, 2020

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

$ go version
go version devel +198f0452b0 Thu Nov 7 02:33:31 2019 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yep.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/filippo/Library/Caches/go-build"
GOENV="/Users/filippo/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/filippo"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org"
GOROOT="/Users/filippo/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/filippo/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/filippo/src/golang.org/x/crypto/go.mod"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/cm/zzcl1fjx27sc8sf3s6sd_0vw0000gn/T/go-build168572639=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have a function that (also thanks to inlining) I'd expect not to allocate anything to the heap.

https://go.googlesource.com/crypto/+/c8d9389ed30fa12bbe2123256b81a26dd5cbe7b8/chacha20poly1305/chacha20poly1305_generic.go (from CL 206977)

What did you see instead?

  Total:     6340801   13065057 (flat, cum) 49.99%
     26            .          .           	binary.LittleEndian.PutUint64(buf[:], uint64(n)) 
     27            .          .           	p.Write(buf[:]) 
     28            .          .           } 
     29            .          .            
     30            .          .           func (c *chacha20poly1305) sealGeneric(dst, nonce, plaintext, additionalData []byte) []byte { 
     31            .        740           	ret, out := sliceForAppend(dst, len(plaintext)+poly1305.TagSize) 
     32            .          .           	ciphertext, tag := out[:len(plaintext)], out[len(plaintext):] 
     33            .          .           	if subtle.InexactOverlap(out, plaintext) { 
     34            .          .           		panic("chacha20poly1305: invalid buffer overlap") 
     35            .          .           	} 
     36            .          .            
     37      6340801    6340801           	var polyKey [32]byte 
     38            .          .           	s, _ := chacha20.NewUnauthenticatedCipher(c.key[:], nonce) 
     39            .          .           	s.XORKeyStream(polyKey[:], polyKey[:]) 
     40            .          .           	s.Advance(1) // set the counter to 1, skipping 32 bytes 
     41            .          .           	s.XORKeyStream(ciphertext, plaintext) 
     42            .          .            
     43            .    6723516           	p := poly1305.New(&polyKey) 
     44            .          .           	writeWithPadding(p, additionalData) 
     45            .          .           	writeWithPadding(p, ciphertext) 
     46            .          .           	writeUint64(p, len(additionalData)) 
     47            .          .           	writeUint64(p, len(plaintext)) 
     48            .          .           	p.Sum(tag[:0]) 

-gcflags=-m has nothing about neither polyKey nor p.

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 10, 2020
@toothrot toothrot added this to the Backlog milestone Jan 10, 2020
@josharian
Copy link
Contributor

I patched in that CL and its parent and ran the package benchmarks. I see zero allocs, across the board, with Go 1.12 (old escape analysis) and with tip.

How did you get those profiling numbers?

@josharian
Copy link
Contributor

(Incidentally, inlining should be completely independent of escape analysis.)

@FiloSottile
Copy link
Contributor Author

Ah, sorry, I forgot to mention that this is the generic code which you can access on non-amd64 GOARCH or with the appengine tag.

(Incidentally, inlining should be completely independent of escape analysis.)

I meant that inlining prevents the values that can stay on the caller stack from escaping to the heap.

https://blog.filippo.io/efficient-go-apis-with-the-inliner/

@josharian
Copy link
Contributor

I just ran $ go build -tags=appengine -gcflags=-m, and I get, among many other lines:

./chacha20poly1305_generic.go:37:6: moved to heap: polyKey
./chacha20poly1305_generic.go:57:6: moved to heap: polyKey

Running with -m -m yields more detail:

./chacha20poly1305_generic.go:37:6: polyKey escapes to heap:
./chacha20poly1305_generic.go:37:6:   flow: {heap} = &polyKey:
./chacha20poly1305_generic.go:37:6:     from polyKey (address-of) at ./chacha20poly1305_generic.go:39:24
./chacha20poly1305_generic.go:37:6:     from polyKey[:] (slice) at ./chacha20poly1305_generic.go:39:24
./chacha20poly1305_generic.go:37:6:     from s.XORKeyStream(polyKey[:], polyKey[:]) (call parameter) at ./chacha20poly1305_generic.go:39:16
./chacha20poly1305_generic.go:37:6: polyKey escapes to heap:
./chacha20poly1305_generic.go:37:6:   flow: {heap} = &polyKey:
./chacha20poly1305_generic.go:37:6:     from polyKey (address-of) at ./chacha20poly1305_generic.go:39:36
./chacha20poly1305_generic.go:37:6:     from polyKey[:] (slice) at ./chacha20poly1305_generic.go:39:36
./chacha20poly1305_generic.go:37:6:     from s.XORKeyStream(polyKey[:], polyKey[:]) (call parameter) at ./chacha20poly1305_generic.go:39:16
./chacha20poly1305_generic.go:57:6: polyKey escapes to heap:
./chacha20poly1305_generic.go:57:6:   flow: {heap} = &polyKey:
./chacha20poly1305_generic.go:57:6:     from polyKey (address-of) at ./chacha20poly1305_generic.go:59:24
./chacha20poly1305_generic.go:57:6:     from polyKey[:] (slice) at ./chacha20poly1305_generic.go:59:24
./chacha20poly1305_generic.go:57:6:     from s.XORKeyStream(polyKey[:], polyKey[:]) (call parameter) at ./chacha20poly1305_generic.go:59:16
./chacha20poly1305_generic.go:57:6: polyKey escapes to heap:
./chacha20poly1305_generic.go:57:6:   flow: {heap} = &polyKey:
./chacha20poly1305_generic.go:57:6:     from polyKey (address-of) at ./chacha20poly1305_generic.go:59:36
./chacha20poly1305_generic.go:57:6:     from polyKey[:] (slice) at ./chacha20poly1305_generic.go:59:36
./chacha20poly1305_generic.go:57:6:     from s.XORKeyStream(polyKey[:], polyKey[:]) (call parameter) at ./chacha20poly1305_generic.go:59:16

Is it possible that you forgot the appengine tag when running the compiler with -m?

@FiloSottile
Copy link
Contributor Author

Yup. It was me being dumb and getting confused by the fact that the appengine tag actually makes subtle.InexactOverlap leak due to reflection, so is not a good representation of the pure Go code.

Sent https://golang.org/cl/215498 to replace it with purego, and sorry for wasting your time.

@josharian
Copy link
Contributor

Happy to be of help. :)

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

No branches or pull requests

4 participants