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: add a go:noinline directive #12312

Closed
randall77 opened this Issue Aug 25, 2015 · 23 comments

Comments

Projects
None yet
@randall77
Contributor

randall77 commented Aug 25, 2015

We're thinking about adding a <go:noinline> directive to help with testing. Currently, if you don't want a function inlined, you add a <switch{}> or a defer or other not-obviously-a-nop construct to the function. We'd like to introduce a new directive called <go:noinline> to mark such functions, as the old way depends on a consistent definition of nop-obviousness (which we probably don't want to commit to).

We particularly need this feature on the SSA branch because if a function is inlined, the code contained in that function might switch from being SSA-compiled to old-compiler-compiled. Without some sort of noinline mark the SSA-specific tests might not be testing the SSA backend at all.

It is possible that once everything is running under SSA this directive will be less useful, but I think it will be generically useful going forward. Thus I'd like to discuss this now in anticipation of it being merged into mainline sometime soon (if it is accepted).

Implementation is pretty easy, https://go-review.googlesource.com/13911 if you're interested.

@randall77 randall77 added the Proposal label Aug 25, 2015

@randall77 randall77 self-assigned this Aug 25, 2015

@randall77 randall77 added this to the Go1.6Early milestone Aug 25, 2015

@randall77 randall77 changed the title from add a go:noinline directive to proposal: add a go:noinline directive Aug 25, 2015

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 25, 2015

Contributor

Usually the implementation comes later :)

Seems reasonable but makes me nervous.

I have a general issue about the proliferation of such things, as I fear the compiler guys will, as always, infect the language with annotations.

Contributor

robpike commented Aug 25, 2015

Usually the implementation comes later :)

Seems reasonable but makes me nervous.

I have a general issue about the proliferation of such things, as I fear the compiler guys will, as always, infect the language with annotations.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Aug 25, 2015

Contributor

I am worried about proliferation here too but I certainly see the need for it repeatedly. It's okay with me. It's also okay for compilers to completely ignore it, like any other comment. :-)

Contributor

rsc commented Aug 25, 2015

I am worried about proliferation here too but I certainly see the need for it repeatedly. It's okay with me. It's also okay for compilers to completely ignore it, like any other comment. :-)

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Aug 25, 2015

Member
Member

minux commented Aug 25, 2015

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Aug 25, 2015

Contributor

Restating the same: worried about proliferation.
That said, I'm ok with the mechanism for testing. I think it should not be advertised widely. And ideally, it should not be available (meaning: have no effect) unless in some form of test scenario. I don't want people to start relying on it.

Contributor

griesemer commented Aug 25, 2015

Restating the same: worried about proliferation.
That said, I'm ok with the mechanism for testing. I think it should not be advertised widely. And ideally, it should not be available (meaning: have no effect) unless in some form of test scenario. I don't want people to start relying on it.

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian Aug 26, 2015

Contributor

@minux said on golang-dev (moving here to keep the discussion in one place):

Ok, how about add a runtime/debug func DoNotInline() that technically is a no-op, but if called, will inhibit inlining of the enclosing function.
For now, we just need to implement that in assembly. Future improved compiler might need to detect it.
It is as clear as //go:noinline, but doesn't require changes to the compiler.

There are two reasons this is not as good as an annotation:

  • It moves it from gc-only, which is fairly hidden and to which the Go 1 compatibility promise doesn't apply, to the stdlib, where it is visible and plausibly creates expectations of cross-compiler support.
  • This annotation is mainly helpful for testing the runtime and compiler. Having to insert extra code into a function in order to achieve this is not ideal, because it means that the part of the runtime or compiler that we are interested has to be unimpacted by the insertion of such code. That's why dev.ssa currently uses the obviously-inlinable-but-not-yet-inlined switch {}: It compiles away to nothing, and the compiler doesn't yet know how to compile panics, defers, etc.
Contributor

josharian commented Aug 26, 2015

@minux said on golang-dev (moving here to keep the discussion in one place):

Ok, how about add a runtime/debug func DoNotInline() that technically is a no-op, but if called, will inhibit inlining of the enclosing function.
For now, we just need to implement that in assembly. Future improved compiler might need to detect it.
It is as clear as //go:noinline, but doesn't require changes to the compiler.

There are two reasons this is not as good as an annotation:

  • It moves it from gc-only, which is fairly hidden and to which the Go 1 compatibility promise doesn't apply, to the stdlib, where it is visible and plausibly creates expectations of cross-compiler support.
  • This annotation is mainly helpful for testing the runtime and compiler. Having to insert extra code into a function in order to achieve this is not ideal, because it means that the part of the runtime or compiler that we are interested has to be unimpacted by the insertion of such code. That's why dev.ssa currently uses the obviously-inlinable-but-not-yet-inlined switch {}: It compiles away to nothing, and the compiler doesn't yet know how to compile panics, defers, etc.
@randall77

This comment has been minimized.

Show comment
Hide comment
@randall77

randall77 Sep 1, 2015

Contributor

I think having this feature be test-only would be nice. We don't see any clean way to do that, however. The compiler doesn't know what is test code and what isn't. We'd have to add a flag to the compiler and have the go tool set that flag for test builds. That seems like a lot of ugly to keep people from using this feature where it doesn't belong.

Contributor

randall77 commented Sep 1, 2015

I think having this feature be test-only would be nice. We don't see any clean way to do that, however. The compiler doesn't know what is test code and what isn't. We'd have to add a flag to the compiler and have the go tool set that flag for test builds. That seems like a lot of ugly to keep people from using this feature where it doesn't belong.

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Sep 1, 2015

Member
Member

minux commented Sep 1, 2015

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian Sep 2, 2015

Contributor

@minux hmmm. How would work with using go:noinline in the runtime or test directory tests, which are run as part of all.bash? Would we set GOEXPERIMENT for the duration of all.bash? Would it cause confusion when someone manually ran 'go test runtime' and the tests failed? To echo Keith, it seems like that mechanism would cause more trouble than it is worth.

Contributor

josharian commented Sep 2, 2015

@minux hmmm. How would work with using go:noinline in the runtime or test directory tests, which are run as part of all.bash? Would we set GOEXPERIMENT for the duration of all.bash? Would it cause confusion when someone manually ran 'go test runtime' and the tests failed? To echo Keith, it seems like that mechanism would cause more trouble than it is worth.

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Sep 2, 2015

Member
Member

minux commented Sep 2, 2015

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian Sep 2, 2015

Contributor

I think it'll be useful outside of the SSA branch.

A very cursory glance at the master branch suggests it'd be useful in pprof test's allocateTransient2M, runtime test's testIfaceEqual, the runtime's atomic* functions, fixedbugs/issue10441.go, fixedbugs/issue12133.go, and fixedbugs/issue8036.go. There are probably more. And it'd be a shame to lose the SSA tests when it gets merged into master and GOEXPERIMENT can no longer be set.

Contributor

josharian commented Sep 2, 2015

I think it'll be useful outside of the SSA branch.

A very cursory glance at the master branch suggests it'd be useful in pprof test's allocateTransient2M, runtime test's testIfaceEqual, the runtime's atomic* functions, fixedbugs/issue10441.go, fixedbugs/issue12133.go, and fixedbugs/issue8036.go. There are probably more. And it'd be a shame to lose the SSA tests when it gets merged into master and GOEXPERIMENT can no longer be set.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Sep 2, 2015

Contributor

"Useful" is always true for a feature request. The question is, does the usefulness justify the cost? The cost here is continued proliferation of magic comments, which are becoming too numerous already.

Contributor

robpike commented Sep 2, 2015

"Useful" is always true for a feature request. The question is, does the usefulness justify the cost? The cost here is continued proliferation of magic comments, which are becoming too numerous already.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Sep 3, 2015

Contributor

My concern here is that a name like go:noinline sounds like something a developer could depend on. Maybe all that's needed is a different name or a qualification that tells the average developer to stay away or at least that the directive cannot be relied on, something like one of these:

//go:test noinline
//go:internal noinline

Contributor

robpike commented Sep 3, 2015

My concern here is that a name like go:noinline sounds like something a developer could depend on. Maybe all that's needed is a different name or a qualification that tells the average developer to stay away or at least that the directive cannot be relied on, something like one of these:

//go:test noinline
//go:internal noinline

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Sep 3, 2015

Member

We already have:

//go:noescape
//go:nosplit
//go:nowritebarrier
//go:systemstack

Does the same argument not apply to all those? Especially to e.g. "noescape"? We going to rename those to //go:internal_no_seriously_i_mean_it nowritebarrier?

It's a comment. Anybody is free to ignore it. And it's an inlining option... it should not change semantics of the program. If it did, we'd have a bigger problem. So nobody should ever want to use it anyway, as it can only make things slower.

I think there's way too much discussion here (including this comment) for something trivial and internal-only which exists only to clean up some tests.

Member

bradfitz commented Sep 3, 2015

We already have:

//go:noescape
//go:nosplit
//go:nowritebarrier
//go:systemstack

Does the same argument not apply to all those? Especially to e.g. "noescape"? We going to rename those to //go:internal_no_seriously_i_mean_it nowritebarrier?

It's a comment. Anybody is free to ignore it. And it's an inlining option... it should not change semantics of the program. If it did, we'd have a bigger problem. So nobody should ever want to use it anyway, as it can only make things slower.

I think there's way too much discussion here (including this comment) for something trivial and internal-only which exists only to clean up some tests.

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Sep 3, 2015

Member
Member

minux commented Sep 3, 2015

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Sep 3, 2015

Member

Let's figure out why do we need to mark a function not
inlinable first. (I'm not talking about the dev.ssa branch, the
problem there is too specific to that branch.)

Right now, I think the only reason is because inlined function
won't appear in stack traces, and that is a bug of our back
tracer, and if we want to be able to inline more, we have to
solve the problem. (Unless we starts to convert recursive
functions to iterative ones or do tailcall optimization, this
stack trace issue is indeed solvable. Showing the value of
arguments to inlined functions is another problem entirely,
and it's probably not worth the effort to solve.)

Is there any other reason I missed?

If my assumption that //go:noinline is only used to solve
the stack trace problem is correct, I think introducing
//go:noinline and make it generally available is just papering
over the real issue.

Member

minux commented Sep 3, 2015

Let's figure out why do we need to mark a function not
inlinable first. (I'm not talking about the dev.ssa branch, the
problem there is too specific to that branch.)

Right now, I think the only reason is because inlined function
won't appear in stack traces, and that is a bug of our back
tracer, and if we want to be able to inline more, we have to
solve the problem. (Unless we starts to convert recursive
functions to iterative ones or do tailcall optimization, this
stack trace issue is indeed solvable. Showing the value of
arguments to inlined functions is another problem entirely,
and it's probably not worth the effort to solve.)

Is there any other reason I missed?

If my assumption that //go:noinline is only used to solve
the stack trace problem is correct, I think introducing
//go:noinline and make it generally available is just papering
over the real issue.

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian Sep 4, 2015

Contributor

@minux another reason is because you need code to take a particular form to trigger a compiler bug, and preventing inlining allows a more minimal/reasonable form. See e.g. this from fixedbugs/issue10441.go:

func bar() {
    f := func() {}
    foo(&f)
}

func foo(f *func()) func() {
    defer func() {}() // prevent inlining of foo
    return *f
}

And with that, I'm giving up arguing for this. I thought it was a simple, unobtrusive, low visibility, low impact fix to an annoyance felt by people working on the compiler. It has ballooned to the point that it no longer feels worth the words.

Contributor

josharian commented Sep 4, 2015

@minux another reason is because you need code to take a particular form to trigger a compiler bug, and preventing inlining allows a more minimal/reasonable form. See e.g. this from fixedbugs/issue10441.go:

func bar() {
    f := func() {}
    foo(&f)
}

func foo(f *func()) func() {
    defer func() {}() // prevent inlining of foo
    return *f
}

And with that, I'm giving up arguing for this. I thought it was a simple, unobtrusive, low visibility, low impact fix to an annoyance felt by people working on the compiler. It has ballooned to the point that it no longer feels worth the words.

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Sep 4, 2015

Member
Member

minux commented Sep 4, 2015

@dr2chase

This comment has been minimized.

Show comment
Hide comment
@dr2chase

dr2chase Sep 4, 2015

Contributor

I agree with Josh, and I am astonished at the amount of discussion generated for a flag that I actually expect most users to avoid except when it really is necessary for real live workarounds. Imagine instead if it were //go:inline , and trying to stop people from using that inappropriately.

As a general rule, I will always favor an explicit comment-expressed directive over attempting to control the compiler by subtext. We have "noinline" now. We spell it "switch {}". Is that documented? Is that guaranteed to work in the future? Is a random Go programmer going to know what that means, and leave it alone?

Contributor

dr2chase commented Sep 4, 2015

I agree with Josh, and I am astonished at the amount of discussion generated for a flag that I actually expect most users to avoid except when it really is necessary for real live workarounds. Imagine instead if it were //go:inline , and trying to stop people from using that inappropriately.

As a general rule, I will always favor an explicit comment-expressed directive over attempting to control the compiler by subtext. We have "noinline" now. We spell it "switch {}". Is that documented? Is that guaranteed to work in the future? Is a random Go programmer going to know what that means, and leave it alone?

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Sep 4, 2015

Member

Introducing a new directive for testing the compiler is one thing,
introducing a new directive for general use is another.

I don't think we should add more generally available directives
lightly. (I just removed one, //go:systemstack)

People might not use //go:noinline very often, but outside of
compiler testing, I see all its use as workaround of a particular
issue which we should solve instead.

Right now there is no convincing argument that explains why
exposing //go:noinline without limitation is necessary.

Member

minux commented Sep 4, 2015

Introducing a new directive for testing the compiler is one thing,
introducing a new directive for general use is another.

I don't think we should add more generally available directives
lightly. (I just removed one, //go:systemstack)

People might not use //go:noinline very often, but outside of
compiler testing, I see all its use as workaround of a particular
issue which we should solve instead.

Right now there is no convincing argument that explains why
exposing //go:noinline without limitation is necessary.

@tzneal

This comment has been minimized.

Show comment
Hide comment
@tzneal

tzneal Sep 4, 2015

Member

As an example where compiler improvements have changed the intent of a test with respect to inlining, see race/testdata/regression_test.go:

func NewImage() Image {
          var pleaseDoNotInlineMe stack
          pleaseDoNotInlineMe.push(1)
          _ = pleaseDoNotInlineMe.pop()
          return Image{}
  }

I tested with go1.2 and the stack push/pop prevented inline. However, I just built that code and it no longer does. The test is no longer testing what it was intended to test due to the implicit prevention of inlining.

Member

tzneal commented Sep 4, 2015

As an example where compiler improvements have changed the intent of a test with respect to inlining, see race/testdata/regression_test.go:

func NewImage() Image {
          var pleaseDoNotInlineMe stack
          pleaseDoNotInlineMe.push(1)
          _ = pleaseDoNotInlineMe.pop()
          return Image{}
  }

I tested with go1.2 and the stack push/pop prevented inline. However, I just built that code and it no longer does. The test is no longer testing what it was intended to test due to the implicit prevention of inlining.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Sep 4, 2015

Contributor

Minux, I understand that you disagree, and for good ideal reasons, but pragmatically I think we should move forward with //go:noinline. Again, compilers in other toolchains will always be free to ignore it, just as they are free to ignore any comments.

As for the specific name, "noinline" matches the other directives. I don't think we should introduce a second naming convention at this point.

Contributor

rsc commented Sep 4, 2015

Minux, I understand that you disagree, and for good ideal reasons, but pragmatically I think we should move forward with //go:noinline. Again, compilers in other toolchains will always be free to ignore it, just as they are free to ignore any comments.

As for the specific name, "noinline" matches the other directives. I don't think we should introduce a second naming convention at this point.

@adg adg added Proposal and removed Proposal labels Sep 25, 2015

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Oct 21, 2015

Contributor

I think there's sufficient support in favor of this proposal that it should be accepted.

Contributor

adg commented Oct 21, 2015

I think there's sufficient support in favor of this proposal that it should be accepted.

@rsc rsc changed the title from proposal: add a go:noinline directive to cmd/compile: add a go:noinline directive Oct 24, 2015

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Oct 29, 2015

CL https://golang.org/cl/13911 mentions this issue.

@gopherbot gopherbot closed this in a92543e Oct 29, 2015

@golang golang locked and limited conversation to collaborators Nov 4, 2016

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