cmd/compile: notice when function variables are never reassigned, avoid indirect calls

[bradfitz]

It's somewhat common for people to have package-level variables like:

var u64 = binary.BigEndian.Uint64

So later in their code they can just write u64(buf[8:]) many times without stuttering a loud binary.BigEndian.Uint64 etc all over the page.

But calling via such a func variable is ~7.5x slower than calling binary.BigEndian.Uint64 directly: https://play.golang.org/p/eccPzwCSvfi

It'd be nice if the compiler noticed that the u64 variable was only initialized once and never reassigned to and never had its address taken (so it can't be mutated elsewhere), and did the fast thing automatically, not doing the indirect call.

/cc @randall77 @mdempsky @ianlancetaylor @cherrymui @josharian @danderson

[mdempsky]

We do this at function scope, but not currently for method values like binary.BigEndian.Uint64. For example, this benchmark already runs comparably to BenchmarkU64FuncWrap:

func BenchmarkU64LocalVar(b *testing.B) {
	var recv = binary.BigEndian
	var u64var = func(b []byte) uint64 { return recv.Uint64(b) }

	var buf [8]byte
	var x uint64
	for i := 0; i < b.N; i++ {
		x += u64var(buf[:])
	}
	sink += x
}

It shouldn't be hard to teach the inliner that var u64var = binary.BigEndian.Uint64 is equivalent to those first two lines.

Doing this with package-block variables is trickier. It wouldn't be hard to check for variables that are never reassigned within the package's Go source, but things like //go:linkname mean we can't guarantee no one else is going to modify it. If we're willing to lock-down //go:linkname somehow to prevent this from happening, I think this would be a reasonable optimization.

[CAFxX]

If we're willing to lock-down //go:linkname somehow to prevent this from happening, I think this would be a reasonable optimization.

As an alternative to restricting //go:linkname we could do it speculatively as is done today where it matters:

func _foo() {}

var foo = _foo

func bar() {
	foo()
}

could be rewritten as

func _foo() {}

var foo = _foo

func bar() {
	if foo == _foo {
		_foo()
	} else {
		foo()
	}
}

Obviously that specific syntax can't be used today as function addresses are not directly comparable, but this is allowed and basically equivalent:

func _foo() {}

var foo func() // foo == nil -> _foo

func bar() {
	if foo == nil {
		_foo()
	} else {
		foo()
	}
}

[josharian]

we're willing to lock-down //go:linkname somehow to prevent this from happening

We could track “is modified” state for both top level vars and linknamed vars and have the linker fail if there’s a mismatch.

[bcmills]

@CAFxX, wouldn't that approach waste branch-predictor resources?

[mdempsky]

We could track “is modified” state for both top level vars and linknamed vars and have the linker fail if there’s a mismatch.

Yeah, I think we definitely have options for emitting metadata that the linker can then use to complain if things look suspicious. E.g., maybe we could use the new linker ABI stuff to emit symbols under a new ABI if the compiler is making new assumptions about how they can be used.

But I think that still depends on us carving out when exactly the Go compiler is allowed to assume it sees all direct uses of a variable. There are certainly other package-wide optimizations we could do if we were allowed to make assumptions like this.

[bcmills]

But I think that still depends on us carving out when exactly the Go compiler is allowed to assume it sees all direct uses of a variable. There are certainly other package-wide optimizations we could do if we were allowed to make assumptions like this.

The language spec is pretty explicit about that, is it not?

An identifier may be exported to permit access to it from another package.

I will admit that I don't know what all go:linkname is used for at the moment, but the vast majority of cases I've seen so far would be better served by factoring out shared functionality into internal packages.

[mdempsky]

The language spec is pretty explicit about that, is it not?

Yes, "Standard Go" specifies that. But "gc Go" specifically provides //go:linkname as an extension to subvert that; see the bottom of https://golang.org/cmd/compile/. (To parallel GCC's "ISO C" vs "GNU C" terminology.)

IIRC, cmd/link's -X flag can also be used to initialize unexported variables, also in contradiction to the spec wording.

I will admit that I don't know what all go:linkname is used for at the moment, but the vast majority of cases I've seen so far would be better served by factoring out shared functionality into internal packages.

I agree that I think most, if not all, use of //go:linkname would be better addressed through internal packages. But I think we should make a conscious, informed decision that we're willing to break existing //go:linkname (mis)users and give them advance notice of our intentions to do that.

[CAFxX]

@CAFxX, wouldn't that approach waste branch-predictor resources?

Yes, at least in the non-constant case. And I also agree it would be ideal to implement a smarter solution. Just wanted to add that technique for completeness since it is not uncommon.

Also consider that while it's true we'd be using more branch predictor resources (due to the extra if), at the same time - and at least under the assumption that the function pointer is actually constant, over which this whole issue is predicated - we would also use less indirect branch predictor resources (due to always executing the direct call instead of the indirect one).