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: optimise away deferred calls to empty functions #26534

Open
ainar-g opened this Issue Jul 22, 2018 · 17 comments

Comments

Projects
None yet
8 participants
@ainar-g
Contributor

ainar-g commented Jul 22, 2018

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

This comment has been minimized.

Contributor

ainar-g commented Jul 22, 2018

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

This comment has been minimized.

Contributor

josharian commented Jul 22, 2018

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?

@josharian josharian added this to the Unplanned milestone Jul 22, 2018

@josharian josharian changed the title from cmd/compile: optimise deferred NOOPs out to cmd/compile: optimise away deferred calls to empty functions Jul 22, 2018

@ainar-g

This comment has been minimized.

Contributor

ainar-g commented Jul 22, 2018

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

This comment has been minimized.

Contributor

josharian commented Jul 23, 2018

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

This comment has been minimized.

Contributor

cznic commented Jul 23, 2018

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.

@ALTree ALTree added help wanted and removed help-wanted labels Jul 23, 2018

@iamoryanmoshe

This comment has been minimized.

Contributor

iamoryanmoshe commented Jul 31, 2018

@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

This comment has been minimized.

rkuska commented Aug 2, 2018

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

This comment has been minimized.

Contributor

iamoryanmoshe commented Aug 2, 2018

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

This comment has been minimized.

Contributor

josharian commented Aug 3, 2018

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

This comment has been minimized.

Contributor

iamoryanmoshe commented Aug 3, 2018

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

This comment has been minimized.

Contributor

iamoryanmoshe commented Aug 3, 2018

@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

This comment has been minimized.

Contributor

iamoryanmoshe commented Aug 5, 2018

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

@agnivade

This comment has been minimized.

Member

agnivade commented Aug 5, 2018

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

@iamoryanmoshe

This comment has been minimized.

Contributor

iamoryanmoshe commented Aug 5, 2018

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

@josharian

This comment has been minimized.

Contributor

josharian commented Aug 5, 2018

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

This comment has been minimized.

Contributor

iamoryanmoshe commented Aug 6, 2018

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

This comment has been minimized.

gopherbot commented Aug 6, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment