code review 7289048: cmd/gc: add way to specify 'noescape' for extern funcs

cmd/gc: add way to specify 'noescape' for extern funcs

A new comment directive //go:noescape instructs the compiler
that the following external (no body) func declaration should be
treated as if none of its arguments escape to the heap.

Fixes issue 4099.

[rsc, 2013-02-03 20:16:39]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[dave_cheney.net, 2013-02-03 20:19:25]

Neat. I'll try some benchmarks today!

On Mon, Feb 4, 2013 at 7:16 AM,  <rsc@golang.org> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/gc: add way to specify 'noescape' for extern funcs
>
> A new comment directive //go:noescape instructs the compiler
> that the following external (no body) func declaration should be
> treated as if none of its arguments escape to the heap.
>
> Fixes issue 4099.
>
> Please review this at https://codereview.appspot.com/7289048/
>
> Affected files:
>   M src/cmd/gc/esc.c
>   M src/cmd/gc/go.h
>   M src/cmd/gc/go.y
>   M src/cmd/gc/lex.c
>   M src/cmd/gc/y.tab.c
>   A test/fixedbugs/issue4099.go
>
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[minux1, 2013-02-03 20:23:48]

i'm a little worried about the trend of introducing annotations
like this (previously //go:fieldtrack is little known and it has to
be explicitly enabled).

people will ask for more.... (for example, //go:noboundcheck,
//go:expecttrue [mirror gcc's __builtin_expect]).

however, i don't have any alternative suggestions either.

[dave_cheney.net, 2013-02-03 20:42:24]

> people will ask for more.... (for example, //go:noboundcheck,
> //go:expecttrue [mirror gcc's __builtin_expect]).
>
> however, i don't have any alternative suggestions either.

As a suggestion, Dmorsing and I talked about exposing the existing
@noescape annotation that exists in the compiled source. The details
are a bit fuzzy and we discarded the idea because we thought we could
never get such a hack through code review. Maybe if this was applied
only to forward decl funcs it would limit the damage.

Dave

[minux1, 2013-02-03 20:54:49]

On Monday, February 4, 2013, Dave Cheney wrote:

> > people will ask for more.... (for example, //go:noboundcheck,
> > //go:expecttrue [mirror gcc's __builtin_expect]).
> >
> > however, i don't have any alternative suggestions either.
>
> As a suggestion, Dmorsing and I talked about exposing the existing
> @noescape annotation that exists in the compiled source. The details
> are a bit fuzzy and we discarded the idea because we thought we could
> never get such a hack through code review. Maybe if this was applied
> only to forward decl funcs it would limit the damage.
>
i'm wondering if we can put the annotation to assembler/C files
(as they are already ugly when compared to Go),
and make them also produce go declarations headers in the
object files, then let gc read those (i.e. first compile C/asm source
files, pack them, then provide the archive to gc, and finally append
the _go_.$GOCHAR to the archive.

then we don't even need forward declarations in Go.

[rsc, 2013-02-03 20:59:27]

This is just a suggestion. It's the best I could come up with to address
the issue. But we could choose not to address the issue.

My rationale is that escape analysis is an important optimization, and
there is nothing that the compiler can possibly do better with escape
analysis of extern funcs in the absence of being told what the answer is.

I do like the idea of putting the annotations in the C or assembly instead,
but I think that's significantly more complicated: it would require work in
cmd/go, cmd/6a, and cmd/6c. This approach is at least a bit more localized,
and it is possible for gccgo to adopt too. It is also not part of the
language spec, so we can change our mind later if we want.

It is not possible to remove the forward declarations from the language.
Backward compatibility aside, they serve an important purpose by letting
other tools (besides the compiler) know that those functions really do
exist. For example, there are exported extern funcs with docs that are
shown by godoc.

I feel confident that we can say no to people who ask for more things. In
contrast, for something like bounds checking the compiler can do quite a
bit better just by working harder at analyzing the code.

Russ

[dave_cheney.net, 2013-02-03 21:13:07]

SGTM. Lets try some benchmarks and see if this pays for itself. 

On 04/02/2013, at 7:59, Russ Cox <rsc@golang.org> wrote:

> This is just a suggestion. It's the best I could come up with to address the
issue. But we could choose not to address the issue.
> 
> My rationale is that escape analysis is an important optimization, and there
is nothing that the compiler can possibly do better with escape analysis of
extern funcs in the absence of being told what the answer is. 
> 
> I do like the idea of putting the annotations in the C or assembly instead,
but I think that's significantly more complicated: it would require work in
cmd/go, cmd/6a, and cmd/6c. This approach is at least a bit more localized, and
it is possible for gccgo to adopt too. It is also not part of the language spec,
so we can change our mind later if we want.
> 
> It is not possible to remove the forward declarations from the language.
Backward compatibility aside, they serve an important purpose by letting other
tools (besides the compiler) know that those functions really do exist. For
example, there are exported extern funcs with docs that are shown by godoc.
> 
> I feel confident that we can say no to people who ask for more things. In
contrast, for something like bounds checking the compiler can do quite a bit
better just by working harder at analyzing the code.
> 
> Russ

[DMorsing, 2013-02-03 21:22:57]

One of the ideas I had during my brainstorming session with Dave was the idea of
decorator functions. For example:

func extern(b []byte)

func Extern(b []byte) {
    // this marks b as noescape for extern, not an actual function call
    noescape(b) 
    extern(b)
}

It pollutes the namespace a bit, but it gets around adding annotations.

[remyoudompheng, 2013-02-03 21:25:35]

On 2013/02/03 21:22:57, DMorsing wrote:
> One of the ideas I had during my brainstorming session with Dave was the idea
of
> decorator functions. For example:
> 
> func extern(b []byte)
> 
> func Extern(b []byte) {
>     // this marks b as noescape for extern, not an actual function call
>     noescape(b) 
>     extern(b)
> }
> 
> It pollutes the namespace a bit, but it gets around adding annotations.

This looks dangerous exposed as is. It should be part of the unsafe package at
least.

[DMorsing, 2013-02-03 21:41:28]

On 2013/02/03 21:25:35, remyoudompheng wrote:
> This looks dangerous exposed as is. It should be part of the unsafe package at
> least.

I didn't think of that. Great suggestion.

Still, I worry about adding such a weird function that has very different
behavior compared to any other function. It's part of the reason I never went
through with the idea.

[adg, 2013-02-04 00:17:37]

I'm quite happy with the comments for this purpose because if a compiler
doesn't support them (or we choose to remove support for them), the code
still works. It just gets slower.

Andrew

[agl1, 2013-02-04 16:34:04]

Benchmarks generally have a for loop to call the same function in a loop, so
they underestimate the benefit of this as the arguments only need to copied to
the heap once.

None the less, the Salsa20 benchmark still gets a 5% boost.

[iant, 2013-02-04 17:17:57]

I think this is as reasonable an approach an any.

We need to know this property for better optimization.  The property can not be
discovered.  Therefore it requires an explicit annotation.  Although the
annotation logically belongs with the external code, that external code might be
written in any language.  Also we would like an annotation that can work with
any Go implementation, including gccgo.  Therefore, the best place for the
annotation is in the Go source code.

This annotation must be correct if it occurs, but it does not cause any trouble
if the annotation is not understood by the compiler; the only effect is code
that is not optimized as well.  We have precedent for using comments for
annotations of this sort: //line and //go:fieldtrack.

Therefore, I think this approach is reasonable.  We could consider adding a more
formal annotation support to the language.  That would be backward compatible,
but so far it does not seem to be necessary.  It may never be necessary for
annotations like the ones we have now, in which failing to understand the
annotation does not cause real trouble.

All that said, I think this CL requires documentation somewhere.  But where?  As
far as I can tell we've managed to fail to document //line.  That also needs to
be fixed.

[rsc, 2013-02-05 03:49:04]

Hello golang-dev@googlegroups.com, dave@cheney.net, minux.ma@gmail.com,
daniel.morsing@gmail.com, remyoudompheng@gmail.com, adg@golang.org,
agl@golang.org, iant@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-02-05 03:49:28]

I added some words to the docs for cmd/gc.

[iant, 2013-02-05 05:58:10]

LGTM

[dave_cheney.net, 2013-02-05 09:57:44]

Something is confusing me, hg tip, unmodified, already says

% go build -gcflags -m | grep decl
./bytes_decl.go:8: IndexByte s does not escape

Is this a bug with -m ?

On Tue, Feb 5, 2013 at 4:58 PM,  <iant@golang.org> wrote:
> LGTM
>
> https://codereview.appspot.com/7289048/

[rsc, 2013-02-05 11:41:33]

Yes, it looks like -m is printing the wrong thing for external functions,
but otherwise the analysis is working:

$ cat x.go
package p

func F(x []byte)

func g() {
var buf [100]byte
F(buf[:])
}
$ go tool 6g -m x.go
x.go:3: F x does not escape
x.go:6: moved to heap: buf
x.go:7: buf escapes to heap
$ 6h x.6
go object darwin amd64 devel +e2f9b9a58240 Mon Feb 04 00:21:44 2013 -0500
X:none
  exports automatically generated from
  x.go in package "p"

$$  // exports
    package p
import runtime "runtime"
func @"".F (@"".x·1 []byte)  <<< NOTE: no 'esc:0x0' tag

Russ

[dave_cheney.net, 2013-02-05 11:46:58]

Fair enough, should I raise an issue for -m ?

I'm also have problems determining any benchmark improvements after annotating
some external functions. This may be because most of our benchmarks are careful
not to allocate during the benchmark routine.

Dave

On 05/02/2013, at 22:41, Russ Cox <rsc@golang.org> wrote:

> Yes, it looks like -m is printing the wrong thing for external functions, but
otherwise the analysis is working:
> 
> $ cat x.go
> package p
> 
> func F(x []byte)
> 
> func g() {
> 	var buf [100]byte
> 	F(buf[:])
> }
> $ go tool 6g -m x.go
> x.go:3: F x does not escape
> x.go:6: moved to heap: buf
> x.go:7: buf escapes to heap
> $ 6h x.6
> go object darwin amd64 devel +e2f9b9a58240 Mon Feb 04 00:21:44 2013 -0500
X:none
>   exports automatically generated from
>   x.go in package "p"
> 
> $$  // exports
>     package p
> 	import runtime "runtime"
> 	func @"".F (@"".x·1 []byte)  <<< NOTE: no 'esc:0x0' tag
> 
> Russ

[rsc, 2013-02-05 11:52:24]

On Tue, Feb 5, 2013 at 6:46 AM, Dave Cheney <dave@cheney.net> wrote:

> Fair enough, should I raise an issue for -m ?
>
> I'm also have problems determining any benchmark improvements after
> annotating some external functions. This may be because most of our
> benchmarks are careful not to allocate during the benchmark routine.
>

That's certainly what it is. We've been careful not to put ourselves in
situations where we needed this so far.

Russ

[rsc, 2013-02-05 11:57:03]

Don't bother adding an issue for -m. I'll fix it before submitting this CL.

[rsc, 2013-02-05 12:00:48]

*** Submitted as https://code.google.com/p/go/source/detail?r=d42bb094c316 ***

cmd/gc: add way to specify 'noescape' for extern funcs

A new comment directive //go:noescape instructs the compiler
that the following external (no body) func declaration should be
treated as if none of its arguments escape to the heap.

Fixes issue 4099.

R=golang-dev, dave, minux.ma, daniel.morsing, remyoudompheng, adg, agl, iant
CC=golang-dev
https://codereview.appspot.com/7289048