cmd/compile: combining two inlined functions with interfaces produces non-inlineable code #61036

Jacalz · 2023-06-27T17:32:46Z

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

$ go version 
go version go1.20.5 linux/amd64

Does this issue reproduce with the latest release?

With latest stable release (Go 1.20.5), yes. I have not tested Go 1.21 release candidate.

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jacob/.cache/go-build"
GOENV="/home/jacob/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/jacob/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jacob/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/lib/golang"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.5"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/jacob/git/fyne/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build38465015=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Link to code: https://go.dev/play/p/78gWoQEaUui

We have the AddXY method that is inlineable with a cost of 12 and Components that is inlineable with cost of 5 as methods on the size type:

type size struct {
	x, y int
}

func (s size) AddXY(x, y int) size {
	return size{s.x + x, s.y + y}
}

func (s size) Components() (int, int) {
	return s.x, s.y
}

We then create two functions and an interface. Combining the two functions for simplified code we get AddSlowVector() which gets a cost of 85 and thus isn't inlineable. However, manually inlining AddXY() to produce AddFastVector() gets us a cost of 79 and is inlineable and 15x faster with zero allocations.

type vector interface {
	Components() (int, int)
}

func (s size) AddFastVector(v vector) size {
	x, y := v.Components()
	return size{s.x + x, s.y + y}
}

func (s size) AddSlowVector(v vector) size {
	return s.AddXY(v.Components())
}

What did you expect to see?

I would expect these two to produce the exact same code because both Components() and AddXY() are inlineable with very small costs (the sum is less than the 80 cost limit). Simplifying AddFastVector() to not duplicate the code from AddXY should be inlineable and not be much slower. It would seem logical for the compiler to basically produce the fast function by inlining AddXY in the slow function.

What did you see instead?

Simplifying AddFastVector() into AddSlowVector() produces a 15x slower function that allocates once. Not using an interface for the input into the function decreases the inline cost a lot and does not exhibit the same slowdown. I do understand that interfaces need to be devirtualized and that produces some overhead but there should not be such a huge difference between the two functions (that are both using interfaces) in my opinion.

BenchmarkPosition_Add/AddFastVector()-8         	775631107	         1.492 ns/op	       0 B/op	       0 allocs/op
BenchmarkPosition_Add/AddSlowVector()-8         	43955944	        22.84 ns/op	      16 B/op	       1 allocs/op
BenchmarkPosition_Add/AddFastSize()-8           	1000000000	         0.4322 ns/op	       0 B/op	       0 allocs/op
BenchmarkPosition_Add/AddSlowSize()-8           	1000000000	         0.4953 ns/op	       0 B/op	       0 allocs/op

The text was updated successfully, but these errors were encountered:

Jacalz · 2023-06-27T17:45:44Z

We have had to introduce comments (https://github.com/fyne-io/fyne/blob/03384a7a3aa88f328724e399301ad38b321dcd84/geometry.go#L78, https://github.com/fyne-io/fyne/blob/03384a7a3aa88f328724e399301ad38b321dcd84/geometry.go#L109 and two more further up) in the codebase of Fyne in order to make sure that someone doesn't try to simplify the code in the future. A 15 to 20x slowdown + one allocation for such a small and simple change is not something we want to see in very commonly used types inside the toolkit.

cagedmantis · 2023-06-28T13:59:21Z

/cc @golang/compiler

randall77 · 2023-07-03T21:42:53Z

This looks like it falls under the general umbrella of improved inlining heuristics. @mpratt @mdempsky

thanm · 2023-07-05T14:35:02Z

I spent a little time looking at this, but I am not sure this is really an actionable bug per se.

There is always going to be a certain amount of inaccuracy when it comes to the cost that estimates the "cost" of inlining a given function; this is one of those cases where the numbers come out slightly differently with inline code vs a function call. Enforcing exact quality of inlining "cost" calculation for hand-inlined code vs call to an out of line function is not a goal that we're trying to achieve.

Jacalz · 2023-07-05T16:00:43Z

I can see the point about not directly comparing inlining costs in that way, it was just a tool for me to try and get some sense into why it behaves like it does. It seems to me like this is something that the compiler should be able to figure out and the fact that it manages to do it when there is a concrete type but not with an interface which seems strange to me.

mdempsky · 2023-07-12T19:15:12Z

The current inliner is known to be very naive, and we're currently working to overhaul it. I hope that should improve your particular use case too, but we need to consider the entire Go ecosystem when tuning optimization policies, so I don't think we can commit to always inlining when you'd like us to.

That said, we have //go:noinline to prevent inlining of functions. I've long thought we should have a //go:inline directive that would hint the compiler towards wanting to inline the function. Maybe there's a proposal for that. If not, I think it's reasonable to file a new one.

Either way, I don't think this issue is particularly actionable, so I'm closing it.

Jacalz · 2023-07-12T19:28:23Z

Cool. Seems sensible to me. I'm looking forward for more improvements to the inliner. Are the improvements scheduled for any particular Go release in the future?

I only found #17566 that mentioned go:inline outside of your comment. Adding it seems like a great idea but I don't think I personally have time at the moment to create a proposal.

bcmills changed the title ~~Combining two inlined functions with interfaces produces non inlineable code~~ cmd/compile: Combining two inlined functions with interfaces produces non inlineable code Jun 28, 2023

gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jun 28, 2023

bcmills added the Performance label Jun 28, 2023

cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 28, 2023

cagedmantis added this to the Backlog milestone Jun 28, 2023

mknyszek changed the title ~~cmd/compile: Combining two inlined functions with interfaces produces non inlineable code~~ cmd/compile: combining two inlined functions with interfaces produces non-inlineable code Jul 12, 2023

mknyszek modified the milestones: Backlog, Unplanned Jul 12, 2023

mdempsky closed this as completed Jul 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: combining two inlined functions with interfaces produces non-inlineable code #61036

cmd/compile: combining two inlined functions with interfaces produces non-inlineable code #61036

Jacalz commented Jun 27, 2023 •

edited

Jacalz commented Jun 27, 2023 •

edited

cagedmantis commented Jun 28, 2023

randall77 commented Jul 3, 2023

thanm commented Jul 5, 2023

Jacalz commented Jul 5, 2023 •

edited

mdempsky commented Jul 12, 2023

Jacalz commented Jul 12, 2023

cmd/compile: combining two inlined functions with interfaces produces non-inlineable code #61036

cmd/compile: combining two inlined functions with interfaces produces non-inlineable code #61036

Comments

Jacalz commented Jun 27, 2023 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

Jacalz commented Jun 27, 2023 • edited

cagedmantis commented Jun 28, 2023

randall77 commented Jul 3, 2023

thanm commented Jul 5, 2023

Jacalz commented Jul 5, 2023 • edited

mdempsky commented Jul 12, 2023

Jacalz commented Jul 12, 2023

Jacalz commented Jun 27, 2023 •

edited

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

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

Jacalz commented Jun 27, 2023 •

edited

Jacalz commented Jul 5, 2023 •

edited