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

Proposal: cmd/compile: add a go:inline directive #21536

Closed
OneOfOne opened this Issue Aug 19, 2017 · 9 comments

Comments

Projects
None yet
7 participants
@OneOfOne
Contributor

OneOfOne commented Aug 19, 2017

The compiler (gc) has come a long way, however the inliner is sometimes lacking, for example any function with a for-loop no matter how simple it is, won't get inlined so we either have to use a goto loop instead (which is used few times in the runtime) or just have a one big ugly function.

My proposal is to add a go:inline directive that will force inlining, even if it's just for non-exported functions.

This will help make a lot of code much cleaner and allows separation of big functions that can't be currently inlined.

If the proposal is accepted, I'd like to work on it.

Another semi-common example:

func (xx *XXHash64) WriteString(s string) (int, error) {
	if len(s) == 0 {
		return 0, nil
	}
	return xx.Write([]byte(s))
}
// cannot inline (*XXHash64).WriteString: non-leaf method

Related: #17566 #14768

@dr2chase @josharian @randall77 @mvdan

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian Aug 19, 2017

Contributor

This proposal has basically no chance of being accepted.

Go eschews knobs.

Compiler directives are widely disliked.

I don't think it can be implemented reasonably, if at all. Functions are not eligible for inlining for many reasons. These include conceptual implementation difficulty: I have no idea how to inline a function containing defer. And practical implementation difficulty: It took a dedicated person a summer to get non-leaf inlining working correctly, even with part of the implementation already written. And it's still not released because of the binary size and compilation time aspects. Supporting such a directive would mean solving a host of such problems.

I personally have long wanted a much smaller directive that causes compilation to fail if a function is deemed ineligible for inlining. This would allow me to refactor without fear that future inlining changes will cause my no-op inlining to become a silent performance regression. Even this much more limited proposal has been rejected forcefully.

All that said, if you'd like to work on improving inlining, that'd be fabulous. There are many opportunities to improve inlining decisions, to improve how we do inlining (e.g. partial inlining), and to expand the set of functions that can be unlined.

Contributor

josharian commented Aug 19, 2017

This proposal has basically no chance of being accepted.

Go eschews knobs.

Compiler directives are widely disliked.

I don't think it can be implemented reasonably, if at all. Functions are not eligible for inlining for many reasons. These include conceptual implementation difficulty: I have no idea how to inline a function containing defer. And practical implementation difficulty: It took a dedicated person a summer to get non-leaf inlining working correctly, even with part of the implementation already written. And it's still not released because of the binary size and compilation time aspects. Supporting such a directive would mean solving a host of such problems.

I personally have long wanted a much smaller directive that causes compilation to fail if a function is deemed ineligible for inlining. This would allow me to refactor without fear that future inlining changes will cause my no-op inlining to become a silent performance regression. Even this much more limited proposal has been rejected forcefully.

All that said, if you'd like to work on improving inlining, that'd be fabulous. There are many opportunities to improve inlining decisions, to improve how we do inlining (e.g. partial inlining), and to expand the set of functions that can be unlined.

@neelance

This comment has been minimized.

Show comment
Hide comment
@neelance

neelance Aug 19, 2017

Member

@josharian The flag -gcflags "-m" makes the go compiler print decisions on inlining. Maybe you can build a tool that parses this output and verifies it against some "must inline" list. Most people won't need it, but if you really do, then you could add that tool to your build pipeline.

Member

neelance commented Aug 19, 2017

@josharian The flag -gcflags "-m" makes the go compiler print decisions on inlining. Maybe you can build a tool that parses this output and verifies it against some "must inline" list. Most people won't need it, but if you really do, then you could add that tool to your build pipeline.

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian Aug 19, 2017

Contributor

@neelance yes, or alternatively parse the output of go tool nm and make sure the inlined function has been dead-code-eliminated. (Note that I want it most of all for package runtime, so "my build pipeline" is probably go test runtime.)

Contributor

josharian commented Aug 19, 2017

@neelance yes, or alternatively parse the output of go tool nm and make sure the inlined function has been dead-code-eliminated. (Note that I want it most of all for package runtime, so "my build pipeline" is probably go test runtime.)

@neelance

This comment has been minimized.

Show comment
Hide comment
@neelance

neelance Aug 19, 2017

Member

@josharian I already suspected that you want it for the stdlib. May I ask why you didn't build it yet? I get the point of not adding such a feature to the Go compiler itself, but a standalone tool doesn't do any harm if only used by individuals voluntarily. I just cringe when I hear "fear of refactoring". ;-)

Member

neelance commented Aug 19, 2017

@josharian I already suspected that you want it for the stdlib. May I ask why you didn't build it yet? I get the point of not adding such a feature to the Go compiler itself, but a standalone tool doesn't do any harm if only used by individuals voluntarily. I just cringe when I hear "fear of refactoring". ;-)

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian Aug 19, 2017

Contributor

@neelance just haven't gotten to it yet; I have little contribution time at the moment and lots I want to do. I'd be delighted if someone else wanted to jump in and do this. The recently introduce runtime function tophash would be an excellent and relevant test case. :)

Contributor

josharian commented Aug 19, 2017

@neelance just haven't gotten to it yet; I have little contribution time at the moment and lots I want to do. I'd be delighted if someone else wanted to jump in and do this. The recently introduce runtime function tophash would be an excellent and relevant test case. :)

@mvdan mvdan changed the title from [proposal] cmd/compile: add a go:inline directive to Proposal cmd/compile: add a go:inline directive Aug 19, 2017

@mvdan mvdan changed the title from Proposal cmd/compile: add a go:inline directive to Proposal: cmd/compile: add a go:inline directive Aug 19, 2017

@gopherbot gopherbot added this to the Proposal milestone Aug 19, 2017

@gopherbot gopherbot added the Proposal label Aug 19, 2017

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Aug 20, 2017

Change https://golang.org/cl/57410 mentions this issue: runtime: add TestIntendedInlining

gopherbot commented Aug 20, 2017

Change https://golang.org/cl/57410 mentions this issue: runtime: add TestIntendedInlining

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Aug 21, 2017

Contributor

For the reasons Josh outlined above, we're not going to do this. Note also that even in C/C++, the "inline" directive does not really mean "this must be inlined". It is more a guideline than a rule.

Contributor

rsc commented Aug 21, 2017

For the reasons Josh outlined above, we're not going to do this. Note also that even in C/C++, the "inline" directive does not really mean "this must be inlined". It is more a guideline than a rule.

@rsc rsc closed this Aug 21, 2017

gopherbot pushed a commit that referenced this issue Aug 22, 2017

runtime: add TestIntendedInlining
The intent is to allow more aggressive refactoring
in the runtime without silent performance changes.

The test would be useful for many functions.
I've seeded it with the runtime functions tophash and add;
it will grow organically (or wither!) from here.

Updates #21536 and #17566

Change-Id: Ib26d9cfd395e7a8844150224da0856add7bedc42
Reviewed-on: https://go-review.googlesource.com/57410
Reviewed-by: Martin Möhrmann <moehrmann@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@mandarjog

This comment has been minimized.

Show comment
Hide comment
@mandarjog

mandarjog May 9, 2018

Any reason why inlining is more difficult in go than other languages like java or c++.

I was trying to find the rationale behind no branches / 40 expressions or less restrictions for inlined functions.
Any pointers are appreciated.

While trying to write performant code (dynamic proto3 marshaling), hand inlining a function brought down marshal time from 5ms to 4ms. That can be significant depending on your performance goals.

mandarjog commented May 9, 2018

Any reason why inlining is more difficult in go than other languages like java or c++.

I was trying to find the rationale behind no branches / 40 expressions or less restrictions for inlined functions.
Any pointers are appreciated.

While trying to write performant code (dynamic proto3 marshaling), hand inlining a function brought down marshal time from 5ms to 4ms. That can be significant depending on your performance goals.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor May 9, 2018

Contributor

@mandarjog A closed issue is not the place for this discussion. General discussions about inlining in Go should use a forum; see https://golang.org/wiki/Questions . Specific discussions about the gc toolchain should take place on the golang-dev mailing list.

Contributor

ianlancetaylor commented May 9, 2018

@mandarjog A closed issue is not the place for this discussion. General discussions about inlining in Go should use a forum; see https://golang.org/wiki/Questions . Specific discussions about the gc toolchain should take place on the golang-dev mailing list.

@ValarDragon ValarDragon referenced this issue Aug 1, 2018

Open

Master Issue of Go1 Issues to consider #1

0 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment