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: combining two inlined functions with interfaces produces non-inlineable code #61036

Closed
Jacalz opened this issue Jun 27, 2023 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@Jacalz
Copy link
Contributor

Jacalz commented Jun 27, 2023

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
@Jacalz
Copy link
Contributor Author

Jacalz commented Jun 27, 2023

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.

@bcmills 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 gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jun 28, 2023
@cagedmantis 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 cagedmantis added this to the Backlog milestone Jun 28, 2023
@cagedmantis
Copy link
Contributor

/cc @golang/compiler

@randall77
Copy link
Contributor

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

@thanm
Copy link
Contributor

thanm commented Jul 5, 2023

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
Copy link
Contributor Author

Jacalz commented Jul 5, 2023

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.

@mknyszek 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 mknyszek modified the milestones: Backlog, Unplanned Jul 12, 2023
@mdempsky
Copy link
Contributor

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
Copy link
Contributor Author

Jacalz commented Jul 12, 2023

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.

@golang golang locked and limited conversation to collaborators Jul 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

8 participants