code review 13677045: build: disable precise collection of stack frames

build: disable precise collection of stack frames

The code for call site-specific pointer bitmaps was not ready in time,
but the zeroing required without it is too expensive to use by default.
We will have to wait for precise collection of stack frames until Go 1.3.

The precise collection can be re-enabled by

        GOEXPERIMENT=precisestack ./all.bash

but that will not be the default for a Go 1.2 build.

Fixes issue 6087.

[rsc, 2013-09-16 18:28:17]

Hello golang-dev@googlegroups.com,

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

[jnj, 2013-09-16 19:40:54]

https://codereview.appspot.com/13677045/diff/5001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/13677045/diff/5001/src/cmd/gc/pgen.c#newcode347
src/cmd/gc/pgen.c:347: 
describe

[rsc, 2013-09-16 20:15:23]

On Mon, Sep 16, 2013 at 3:40 PM, <jeremyjackins@gmail.com> wrote:

>
>
https://codereview.appspot.**com/13677045/diff/5001/src/**cmd/gc/pgen.c<https...
> File src/cmd/gc/pgen.c (right):
>
> https://codereview.appspot.**com/13677045/diff/5001/src/**
>
cmd/gc/pgen.c#newcode347<https://codereview.appspot.com/13677045/diff/5001/src/cmd/gc/pgen.c#newcode347>
> src/cmd/gc/pgen.c:347:
> describe
>

I don't understand.

Russ

[kortschak, 2013-09-16 20:55:36]

It should be 'to describe' or 'that describes'.

On 17/09/2013, at 5:45 AM, "Russ Cox" <rsc@golang.org> wrote:

> I don't understand.

[rsc, 2013-09-16 20:59:13]

aha, thanks, done.
was looking at wrong line.

[jnj, 2013-09-16 21:10:46]

On 2013/09/16 20:15:23, rsc wrote:
> On Mon, Sep 16, 2013 at 3:40 PM, <mailto:jeremyjackins@gmail.com> wrote:
> 
> >
> >
>
https://codereview.appspot.**com/13677045/diff/5001/src/**cmd/gc/pgen.c%3Chtt...>
> > File src/cmd/gc/pgen.c (right):
> >
> > https://codereview.appspot.**com/13677045/diff/5001/src/**
> >
>
cmd/gc/pgen.c#newcode347<https://codereview.appspot.com/13677045/diff/5001/src/cmd/gc/pgen.c#newcode347>
> > src/cmd/gc/pgen.c:347:
> > describe
> >
> 
> I don't understand.
> 
> Russ

The grammar of this comment doesn't make sense to me. I was suggesting that the
"s" be removed from "describes". But looking again, I think the "s" should be
dropped from "dumps" as well. The comment for the next function up also has
strange grammar. Perhaps my complaints are not relevant to this CL, though.

[jnj, 2013-09-16 21:12:51]

On 2013/09/16 21:10:46, jnj wrote:
> On 2013/09/16 20:15:23, rsc wrote:
> > On Mon, Sep 16, 2013 at 3:40 PM, <mailto:jeremyjackins@gmail.com> wrote:
> > 
> > >
> > >
> >
>
https://codereview.appspot.**com/13677045/diff/5001/src/**cmd/gc/pgen.c%253Ch...>
> > > File src/cmd/gc/pgen.c (right):
> > >
> > > https://codereview.appspot.**com/13677045/diff/5001/src/**
> > >
> >
>
cmd/gc/pgen.c#newcode347<https://codereview.appspot.com/13677045/diff/5001/src/cmd/gc/pgen.c#newcode347>
> > > src/cmd/gc/pgen.c:347:
> > > describe
> > >
> > 
> > I don't understand.
> > 
> > Russ
> 
> The grammar of this comment doesn't make sense to me. I was suggesting that
the
> "s" be removed from "describes". But looking again, I think the "s" should be
> dropped from "dumps" as well. The comment for the next function up also has
> strange grammar. Perhaps my complaints are not relevant to this CL, though.

Too slow, sorry for the noise

[kortschak, 2013-09-16 21:17:41]

Yes s/dumps/dump.

On 17/09/2013, at 6:41 AM, "jeremyjackins@gmail.com" <jeremyjackins@gmail.com>
wrote:

> But looking again, I think the
> "s" should be dropped from "dumps" as well.

[r, 2013-09-16 22:35:30]

LGTM

https://codereview.appspot.com/13677045/diff/21001/src/cmd/dist/buildruntime.c
File src/cmd/dist/buildruntime.c (right):

https://codereview.appspot.com/13677045/diff/21001/src/cmd/dist/buildruntime....
src/cmd/dist/buildruntime.c:41: // mkzexperiment writes zaexperiment.h [sic]:
use round brackets for sic.  [x] means a substitution; (x) means a comment.

https://codereview.appspot.com/13677045/diff/21001/src/cmd/dist/buildruntime....
src/cmd/dist/buildruntime.c:58: "// auto generated by go tool dist\n"
s/auto //

https://codereview.appspot.com/13677045/diff/21001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/13677045/diff/21001/src/pkg/runtime/proc.c#new...
src/pkg/runtime/proc.c:2950: return 1;
a couple of years ago, you would have written these loops using pointers.

[rsc, 2013-09-17 00:21:54]

https://codereview.appspot.com/13677045/diff/21001/src/cmd/dist/buildruntime.c
File src/cmd/dist/buildruntime.c (right):

https://codereview.appspot.com/13677045/diff/21001/src/cmd/dist/buildruntime....
src/cmd/dist/buildruntime.c:58: "// auto generated by go tool dist\n"
On 2013/09/16 22:35:30, r wrote:
> s/auto //

this matches a half dozen other such comments, so i'm going to leave them all as
is.
in general i do try to distinguish between 'generated' (but you have to run the
generator) and 'auto-generated' (you don't).

https://codereview.appspot.com/13677045/diff/21001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/13677045/diff/21001/src/pkg/runtime/proc.c#new...
src/pkg/runtime/proc.c:2950: return 1;
On 2013/09/16 22:35:30, r wrote:
> a couple of years ago, you would have written these loops using pointers.

indeed

[rsc, 2013-09-17 00:26:15]

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

build: disable precise collection of stack frames

The code for call site-specific pointer bitmaps was not ready in time,
but the zeroing required without it is too expensive to use by default.
We will have to wait for precise collection of stack frames until Go 1.3.

The precise collection can be re-enabled by

        GOEXPERIMENT=precisestack ./all.bash

but that will not be the default for a Go 1.2 build.

Fixes issue 6087.

R=golang-dev, jeremyjackins, dan.kortschak, r
CC=golang-dev
https://codereview.appspot.com/13677045

[iant, 2013-09-17 00:26:52]

LGTM