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: support inlining of functions containing function literals #28727

Open
tdewolff opened this issue Nov 11, 2018 · 4 comments
Open

cmd/compile: support inlining of functions containing function literals #28727

tdewolff opened this issue Nov 11, 2018 · 4 comments
Assignees
Milestone

Comments

@tdewolff
Copy link

@tdewolff tdewolff commented Nov 11, 2018

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

$ go version
go version go1.11.1 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/tmp/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/people/tdew803/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go-1.11"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.11/pkg/tool/linux_amd64"
GCCGO="gccgo"
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-build626602379=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Looking at the following benchmarks, returning a func literal always gets allocated on the heap while a similar approach using a struct can be allocated on the stack. I propose that func literals are also assessed whether they can be put on the stack or on the heap.

package main

import "testing"

func FuncA(x *int) func() {
    return func() { (*x)++ }
}

var A int

func BenchmarkTestA(b *testing.B) {
    for n := 0; n < b.N; n++ {
        f := FuncA(&A)
        f()
    }
}

////////////////

type Increaser struct {
    x *int
}

func (inc Increaser) Increase() {
    (*inc.x)++
}

func FuncB(x *int) Increaser {
    return Increaser{x}
}

var B int

func BenchmarkTestB(b *testing.B) {
    for n := 0; n < b.N; n++ {
        inc := FuncB(&B)
        inc.Increase()
    }
}

giving

$ go test -bench=. -benchmem -gcflags '-m -l'
# test [test]
./run_test.go:6:12: func literal escapes to heap
./run_test.go:6:12: func literal escapes to heap
./run_test.go:5:12: leaking param: x to result ~r1 level=-1
./run_test.go:13:7: Increaser.Increase inc does not escape
./run_test.go:17:12: leaking param: x to result ~r1 level=0
./run_test.go:25:20: &A escapes to heap
./run_test.go:23:21: BenchmarkTestA b does not escape
./run_test.go:30:21: BenchmarkTestB b does not escape
./run_test.go:32:22: BenchmarkTestB &B does not escape
<autogenerated>:1: (*Increaser).Increase .this does not escape
# test
/tmp/go-build496566533/b001/_testmain.go:42:42: testdeps.TestDeps literal escapes to heap
goos: linux
goarch: amd64
pkg: test
BenchmarkTestA-8   	50000000	        26.9 ns/op	      16 B/op	       1 allocs/op
BenchmarkTestB-8   	1000000000	         2.41 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	test	4.041s

When this func literal would be allocated on the stack, we would see a ~10x performance increase.

@randall77
Copy link
Contributor

@randall77 randall77 commented Nov 11, 2018

We do allocate function literals on the stack, if possible.
We can't currently return the address of stack-allocated objects, function literals or otherwise.
That's what is happening in benchmark A, the stack frame that the closure would be allocated on disappears on the return.
Your benchmark B is misleading I think because everything inlines.

@ianlancetaylor ianlancetaylor changed the title cmd/gc: allocate func literal on stack if possible cmd/compile: allocate func literal on stack if possible Nov 12, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Nov 12, 2018
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Apr 16, 2019

The crux of this issue is that functions that contain closures aren't currently inlineable. If FuncA was inlined into BenchmarkTestA, then I expect escape analysis would detect that it can be stack allocated.

@mdempsky mdempsky changed the title cmd/compile: allocate func literal on stack if possible cmd/compile: support inlining of functions containing function literals Apr 16, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@danscales danscales self-assigned this Nov 5, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 5, 2020

Change https://golang.org/cl/267880 mentions this issue: Exporting, importing, and inlining functions with OCLOSURE

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 11, 2021

Change https://golang.org/cl/283112 mentions this issue: [dev.regabi] cmd/compile: Exporting, importing, and inlining functions with OCLOSURE

gopherbot pushed a commit that referenced this issue Jan 20, 2021
…s with OCLOSURE

I have exporting, importing, and inlining of functions with closures
working in all cases (issue #28727). all.bash runs successfully without
errors.

Approach:
  - Write out the Func type, Dcls, ClosureVars, and Body when exporting
    an OCLOSURE.

  - When importing an OCLOSURE, read in the type, dcls, closure vars,
    and body, and then do roughly equivalent code to (*noder).funcLit

  - During inlining of a closure within inlined function, create new
    nodes for all params and local variables (including closure
    variables), so they can have a new Curfn and some other field
    values. Must substitute not only on the Nbody of the closure, but
    also the Type, Cvars, and Dcl fields.

Fixes #28727

Change-Id: I4da1e2567c3fa31a5121afbe82dc4e5ee32b3170
Reviewed-on: https://go-review.googlesource.com/c/go/+/283112
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Trust: Dan Scales <danscales@google.com>
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
8 participants