code review 85430046: runtime: cache gotraceback setting

runtime: cache gotraceback setting

On Plan 9 gotraceback calls getenv calls malloc, and we gotraceback
on every call to gentraceback, which happens during garbage collection.
Honestly I don't even know how this works on Plan 9.
I suspect it does not, and that we are getting by because
no one has tried to run with $GOTRACEBACK set at all.

This will speed up all the other systems by epsilon, since they
won't call getenv and atoi repeatedly.

[rsc, 2014-04-08 19:59:01]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-04-08 20:02:39]

LGTM
 On Apr 8, 2014 12:59 PM, <rsc@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: cache gotraceback setting
>
> On Plan 9 gotraceback calls getenv calls malloc, and we gotraceback
> on every call to gentraceback, which happens during garbage collection.
> Honestly I don't even know how this works on Plan 9.
> I suspect it does not, and that we are getting by because
> no one has tried to run with $GOTRACEBACK set at all.
>
> This will speed up all the other systems by epsilon, since they
> won't call getenv and atoi repeatedly.
>
> Please review this at https://codereview.appspot.com/85430046/
>
> Affected files (+28, -11 lines):
>   M src/pkg/runtime/proc.c
>   M src/pkg/runtime/runtime.c
>
>
> Index: src/pkg/runtime/proc.c
> ===================================================================
> --- a/src/pkg/runtime/proc.c
> +++ b/src/pkg/runtime/proc.c
> @@ -155,6 +155,10 @@
>         // in a fault during a garbage collection, it will not
>         // need to allocated memory.
>         runtime·newErrorCString(0, &i);
> +
> +       // Initialize the cached gotraceback value, since
> +       // gotraceback calls getenv, which mallocs on Plan 9.
> +       runtime·gotraceback(nil);
>
>         runtime·goargs();
>         runtime·goenvs();
> Index: src/pkg/runtime/runtime.c
> ===================================================================
> --- a/src/pkg/runtime/runtime.c
> +++ b/src/pkg/runtime/runtime.c
> @@ -20,22 +20,35 @@
>  int32
>  runtime·gotraceback(bool *crash)
>  {
> +       // Keep a cached value to make gotraceback fast,
> +       // since we call it on every call to gentraceback.
> +       // The cached value is a uint32 in which the low bit
> +       // is the "crash" setting and the top 31 bits are the
> +       // gotraceback value.
> +       static uint32 cache = ~(uint32)0;
>         byte *p;
> +       uint32 x;
>
>         if(crash != nil)
>                 *crash = false;
> -       p = runtime·getenv("GOTRACEBACK");
> -       if(p == nil || p[0] == '\0') {
> -               if(m->traceback != 0)
> -                       return m->traceback;
> -               return 1;       // default is on
> +       if(m->traceback != 0)
> +               return m->traceback;
> +       x = runtime·atomicload(&cache);
> +       if(x == ~(uint32)0) {
> +               p = runtime·getenv("GOTRACEBACK");
> +               if(p == nil)
> +                       p = (byte*)"";
> +               if(p[0] == '\0')
> +                       x = 1<<1;
> +               else if(runtime·strcmp(p, (byte*)"crash") == 0)
> +                       x = (2<<1) | 1;
> +               else
> +                       x = runtime·atoi(p)<<1;
> +               runtime·atomicstore(&cache, x);
>         }
> -       if(runtime·strcmp(p, (byte*)"crash") == 0) {
> -               if(crash != nil)
> -                       *crash = true;
> -               return 2;       // extra information
> -       }
> -       return runtime·atoi(p);
> +       if(crash != nil)
> +               *crash = x&1;
> +       return x>>1;
>  }
>
>  int32
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[0intro, 2014-04-08 20:17:55]

Thanks, this is much better.

GOTRACEBACK never worked on Plan 9. It fails on:

cpu% GOTRACEBACK=1
cpu% go version
go 227126: warning: process exceeds 100 file descriptors
go 227126: suicide: sys: trap: fault read addr=0x4 pc=0x0007185f

[rsc, 2014-04-08 20:41:38]

On Tue, Apr 8, 2014 at 4:17 PM, <0intro@gmail.com> wrote:

> Thanks, this is much better.
>
> GOTRACEBACK never worked on Plan 9. It fails on:
>

So it works after this CL?

Thanks.
Russ

[rsc, 2014-04-08 20:54:46]

On Tue, Apr 8, 2014 at 4:45 PM, David du Colombier <0intro@gmail.com> wrote:

> > So it works after this CL?
>
> No, same issue after your change.
>

It's because getenv calls malloc. Getenv really needs to not call malloc
here. Maybe it can call sbrk or something.

[rsc, 2014-04-08 20:55:13]

Or getenv could have a static buffer it uses to read $GOTRACEBACK only.

[rsc, 2014-04-09 02:35:46]

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

runtime: cache gotraceback setting

On Plan 9 gotraceback calls getenv calls malloc, and we gotraceback
on every call to gentraceback, which happens during garbage collection.
Honestly I don't even know how this works on Plan 9.
I suspect it does not, and that we are getting by because
no one has tried to run with $GOTRACEBACK set at all.

This will speed up all the other systems by epsilon, since they
won't call getenv and atoi repeatedly.

LGTM=bradfitz
R=golang-codereviews, bradfitz, 0intro
CC=golang-codereviews
https://codereview.appspot.com/85430046

[gobot, 2014-04-09 03:49:35]

This CL appears to have broken the linux-386 builder.
See http://build.golang.org/log/520f2901a28775df8d4627bdf7370a62213431dc