cmd/compile: optimise away deferred calls to empty functions

[ainar-g]

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

Does this issue reproduce with the latest release?

go version devel +48c7973 Fri Jul 20 20:08:15 2018 +0000 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ainar/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ainar/go"
GOPROXY=""
GORACE=""
GOROOT="/home/ainar/go/gotip"
GOTMPDIR=""
GOTOOLDIR="/home/ainar/go/gotip/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-build664294769=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/TbjFF3fe8Wu

$ go run -gcflags '-S' assertopt.go

What did you expect to see?

Lines 12, 13, and 14 skipped by the compiler.

What did you see instead?

Line 12 is skipped, but defers on 13 and 14 are still there. The Go compiler leaves those in, despite the fact that they are NOOPs.

This makes it hard to create assertions (in this case, postconditions) that don't hurt the performance of production builds using build tags.

[ainar-g]

FWIW, I went further and tested it with and without closures, and also with the go statement. The results are:

    assert(x == 0)                    // Optimised.
    func() {}()                       // Optimised.
    func() { assert(x == 1) }()       // Optimised.
    defer assert(x == 0)              // Not optimised.
    defer func() {}()                 // Not optimised.
    defer func() { assert(x == 1) }() // Not optimised.
    go assert(x == 0)                 // Not optimised.
    go func() {}()                    // Not optimised.
    go func() { assert(x == 1) }()    // Not optimised.

[josharian]

The optimized cases you see are only optimized because of inlining. If you change it to:

//go:noinline
func assert(cond bool) { /* NOOP in this build tag. */ }

then you'll see all present.

Recognizing and eliminating go and defer of functions with completely empty bodies should be straightforward enough. Care must be taken to ensure that evaluation of the arguments (and receiver) for side-effects still occurs. It might be a bit harder with functions from other packages, if the function doesn't happen to be inlineable, but that should be rare (and if it is a real problem, see #25999).

Is it safe to infer that this happens to you with real code, where a function is empty due to build tags, and that the defer calls are a performance problem?

[ainar-g]

Good catch with the inlining, and thanks for the input!

There is no real code at the moment, I'm just tinkering with the concept of a package for contact programming in Go. The interface is still in flux, but the main idea is that all methods are NOOPs, unless a build tag is specified. Preconditions are checked immediately, and postconditions are checked in a deferred function. Ideally, I would like to be able to write something like this in the package's README:

<...> Calling these methods has no runtime overhead, unless the build tag contract is used. If the build tag is not used, those methods are NOOPs, and thus are optimised away by the compiler (at least since Go 1.XX). <...>

But depending on the interface I will end up with, this might be a hard promise to keep.

[josharian]

I've also been pondering contract-based programming in Go, so I hope that you will write up the results of your experiment and share it.

The fact that this issue isn't motivated by existing code with performance issues makes it a bit lower priority, realistically, but it is (I believe) a simple enough fix that someone will decide to tackle it anyway.

[cznic]

I would like to be able to write something like this in the package's README:

Please note that the language specification does not guarantee anything of that. It may be true for a particular version of a particular Go compiler, at most.

[iamoryanmoshe]

@josharian Which package checks for these and optimizes?

Seems like something I could do using ast, but I would like direction as to which file to look for the optimization functions in.

If it is not taken I'd like to tackle it with a bit of guidance (:

[rkuska]

I am also interested in working on this issue so we can join the efforts, I was looking at the compile documentation and I think that the optimization should happen either in Generic SSA or Type-checking and AST transformations phase. I am more inclined to do this in SSA phase as there is stated following:

... Some of these [optimizations] currently happen before the conversion to SSA due to historical reasons, but the long-term plan is to move all of them here [to Generic SSA phase].

It seems like the best place to put this optimizations is cmd/compile/internal/ssa/deadcode.go as I consider these functions as a deadcode.

I will investigate more once I catch up on with SSA and receive some sort of ack or anything :)

[iamoryanmoshe]

I also read a bit about SSA today, tomorrow I'll dive into the code and understand exactly how we should do it (:

@rkuska I sent you an email with further discussion

[josharian]

If you see a clean way to do this in SSA world, that'd be great. I had had in mind catching this case in cmd/compile/internal/gc/walk.go, func walkstmt, case ODEFER / OPROC. (OPROC should be renamed to OGO; no one has done that yet.)

If you get a CL together, please be sure to add fairly extensive tests, including inlineable and non-inlineable (via //go:noinline), same-package and different-package, and cases in which the function argument evaluation has side-effects (e.g. f(g()), where g increments a global variable).

Sorry for the slow response, I have very little computer time at the moment.

[iamoryanmoshe]

I found a good place to do it in SSA deadcode.go, when we check for live code in functions. That way we eliminate all calls to the function.

Should I push the OPROC -> OGO change with this CL or create a new issue?

@josharian

[iamoryanmoshe]

@josharian Do we prefer to remove the function if it's a no-op function and let the SSA do it's own thing by removing the calls, or should we remove the calls to no-op functions but leave the functions as is?

[iamoryanmoshe]

Anyone familiar with the compiler has some time to answer a few questions?

[agnivade]

Please feel free to reach out to golang-dev mailing list.

[iamoryanmoshe]

@agnivade I asked my question there but no one answered, so I thought I better try here

[josharian]

I'll do my best to answer questions, but I have very limited computer time, so expect significant delays. Hopefully others will also find time to chip in. Ideally we can try to keep the conversation in one place, though, to make it easier to reply. For now I'll reply in each place to the questions asked there.

Should I push the OPROC -> OGO change with this CL or create a new issue?

This is very low priority. The name has been bad for years now. No need to create an issue for it. Feel free to just send a CL, but if you do so, (a) please prepare the CL using gorename, (b) please don't mix it with any other changes, to make it easier to review.

Do we prefer to remove the function if it's a no-op function and let the SSA do it's own thing by removing the calls, or should we remove the calls to no-op functions but leave the functions as is?

I don't really understand this question, I'm afraid.

Another thing to test, by the way:

type T int

func (t T) f() {}

func main() {
  var t *T
  t.f()
}

This will panic--t gets dereferenced in order to call f. The compiler implements this with an autogenerated wrapper function "*T.f. You'll want to make sure that this panic still occurs appropriately even with T.f empty.

[iamoryanmoshe]

I have something working, I'll push it later today, it doesn't interfere with methods so no problem with the case of nil object.

[gopherbot]

Change https://golang.org/cl/128035 mentions this issue: cmd/compile: optimize away deferred/go call to empty functions