code review 42280045: cmd/gc: do not compute dead value maps if they will not...

cmd/gc: do not compute dead value maps if they will not be used

Reduces 6g big.go memory usage from 251 MB to 242 MB.
Reduces 6g slow.go memory usage from 529 MB to 453 MB.

Mostly a memory savings; 6g slow.go is only about 5% faster.

The test programs are at
https://rsc.googlecode.com/hg/testdata/big.go (36k lines, 276kB)
https://rsc.googlecode.com/hg/testdata/slow.go (7k lines, 352kB)

[rsc, 2013-12-20 20:55:05]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2013-12-20 21:49:15]

Another question about testing, just to be a broken record:

For changes like this, could we make tests that have a small input program,
compile and/or link it, and then do tests against the contents of the
pclntable or other parts of the toolchain output(s)?



On Fri, Dec 20, 2013 at 12:55 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:
> cmd/gc: do not compute dead value maps if they will not be used
>
> Reduces 6g big.go memory usage from 251 MB to 242 MB.
> Reduces 6g slow.go memory usage from 529 MB to 453 MB.
>
> Mostly a memory savings; 6g slow.go is only about 5% faster.
>
> The test programs are at
> https://rsc.googlecode.com/hg/testdata/big.go (36k lines, 276kB)
> https://rsc.googlecode.com/hg/testdata/slow.go (7k lines, 352kB)
>
> Please review this at https://codereview.appspot.com/42280045/
>
> Affected files (+29, -18 lines):
>   M src/cmd/gc/plive.c
>
>
> Index: src/cmd/gc/plive.c
> ===================================================================
> --- a/src/cmd/gc/plive.c
> +++ b/src/cmd/gc/plive.c
> @@ -690,7 +690,7 @@
>  // liveness computation.  The cfg argument is an array of BasicBlock*s
> and the
>  // vars argument is an array of Node*s.
>  static Liveness*
> -newliveness(Node *fn, Prog *ptxt, Array *cfg, Array *vars)
> +newliveness(Node *fn, Prog *ptxt, Array *cfg, Array *vars, int
> computedead)
>  {
>         Liveness *result;
>         int32 i;
> @@ -719,8 +719,13 @@
>
>         result->livepointers = arraynew(0, sizeof(Bvec*));
>         result->argslivepointers = arraynew(0, sizeof(Bvec*));
> -       result->deadvalues = arraynew(0, sizeof(Bvec*));
> -       result->argsdeadvalues = arraynew(0, sizeof(Bvec*));
> +       if(computedead) {
> +               result->deadvalues = arraynew(0, sizeof(Bvec*));
> +               result->argsdeadvalues = arraynew(0, sizeof(Bvec*));
> +       } else {
> +               result->deadvalues = nil;
> +               result->argsdeadvalues = nil;
> +       }
>         return result;
>  }
>
> @@ -741,13 +746,15 @@
>                 free(*(Bvec**)arrayget(lv->argslivepointers, i));
>         arrayfree(lv->argslivepointers);
>
> -       for(i = 0; i < arraylength(lv->deadvalues); i++)
> -               free(*(Bvec**)arrayget(lv->deadvalues, i));
> -       arrayfree(lv->deadvalues);
> -
> -       for(i = 0; i < arraylength(lv->argsdeadvalues); i++)
> -               free(*(Bvec**)arrayget(lv->argsdeadvalues, i));
> -       arrayfree(lv->argsdeadvalues);
> +       if(lv->deadvalues != nil) {
> +               for(i = 0; i < arraylength(lv->deadvalues); i++)
> +                       free(*(Bvec**)arrayget(lv->deadvalues, i));
> +               arrayfree(lv->deadvalues);
> +
> +               for(i = 0; i < arraylength(lv->argsdeadvalues); i++)
> +                       free(*(Bvec**)arrayget(lv->argsdeadvalues, i));
> +               arrayfree(lv->argsdeadvalues);
> +       }
>
>         for(i = 0; i < arraylength(lv->cfg); i++) {
>                 free(lv->uevar[i]);
> @@ -1333,10 +1340,12 @@
>                         arrayadd(lv->livepointers, &locals);
>
>                         // Dead stuff second.
> -                       args = bvalloc(argswords() * BitsPerPointer);
> -                       arrayadd(lv->argsdeadvalues, &args);
> -                       locals = bvalloc(localswords() * BitsPerPointer);
> -                       arrayadd(lv->deadvalues, &locals);
> +                       if(lv->deadvalues != nil) {
> +                               args = bvalloc(argswords() *
> BitsPerPointer);
> +                               arrayadd(lv->argsdeadvalues, &args);
> +                               locals = bvalloc(localswords() *
> BitsPerPointer);
> +                               arrayadd(lv->deadvalues, &locals);
> +                       }
>                 }
>
>                 // walk backward, emit pcdata and populate the maps
> @@ -1391,9 +1400,11 @@
>                                 twobitlivepointermap(lv, liveout,
> lv->vars, args, locals);
>
>                                 // Record dead values.
> -                               args = *(Bvec**)arrayget(lv->argsdeadvalues,
> pos);
> -                               locals = *(Bvec**)arrayget(lv->deadvalues,
> pos);
> -                               twobitdeadvaluemap(lv, liveout, lv->vars,
> args, locals);
> +                               if(lv->deadvalues != nil) {
> +                                       args =
*(Bvec**)arrayget(lv->argsdeadvalues,
> pos);
> +                                       locals =
*(Bvec**)arrayget(lv->deadvalues,
> pos);
> +                                       twobitdeadvaluemap(lv, liveout,
> lv->vars, args, locals);
> +                               }
>
>                                 pos--;
>                         }
> @@ -1487,7 +1498,7 @@
>         cfg = newcfg(firstp);
>         if(0) printcfg(cfg);
>         vars = getvariables(fn);
> -       lv = newliveness(fn, firstp, cfg, vars);
> +       lv = newliveness(fn, firstp, cfg, vars, deadsym != nil);
>
>         // Run the dataflow framework.
>         livenessprologue(lv);
>
>
> --
> 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/groups/opt_out.
>

[rsc, 2013-12-20 22:10:37]

R=r (so bradfitz doesn't get stuck with it)

On Fri, Dec 20, 2013 at 4:49 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Another question about testing, just to be a broken record:
>
> For changes like this, could we make tests that have a small input
> program, compile and/or link it, and then do tests against the contents of
> the pclntable or other parts of the toolchain output(s)?
>

Eventually, sure. As part of moving to Go that absolutely has to happen.
I'd rather wait until then, though.

For what it's worth, I'm working on the first CL for the new linker right
now, the part that actually writes an executable file with the right
headers, and it has tests from the start: given an in-memory description of
a tiny program to link, check that the resulting output file can be read
back by debug/macho and has the expected values and content.

Back to this CL, this CL has literally effect on the output file. It just
uses less memory during compilation. That is, the compiler was computing
some data and then not writing it to the output file. This CL makes it not
compute that unused data.

Russ

[bradfitz, 2013-12-20 22:12:11]

SGTM

Thanks.



On Fri, Dec 20, 2013 at 2:10 PM, Russ Cox <rsc@golang.org> wrote:

> R=r (so bradfitz doesn't get stuck with it)
>
> On Fri, Dec 20, 2013 at 4:49 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
>> Another question about testing, just to be a broken record:
>>
>> For changes like this, could we make tests that have a small input
>> program, compile and/or link it, and then do tests against the contents of
>> the pclntable or other parts of the toolchain output(s)?
>>
>
> Eventually, sure. As part of moving to Go that absolutely has to happen.
> I'd rather wait until then, though.
>
> For what it's worth, I'm working on the first CL for the new linker right
> now, the part that actually writes an executable file with the right
> headers, and it has tests from the start: given an in-memory description of
> a tiny program to link, check that the resulting output file can be read
> back by debug/macho and has the expected values and content.
>
> Back to this CL, this CL has literally effect on the output file. It just
> uses less memory during compilation. That is, the compiler was computing
> some data and then not writing it to the output file. This CL makes it not
> compute that unused data.
>
> Russ
>

[iant, 2013-12-20 22:52:01]

LGTM

Right now, of course, we never generate the dead value map.  I don't fully grasp
Carl's comment about when we will want to generate it--is there going to be a
time in the future when we always generate it?

[rsc, 2013-12-20 22:55:18]

On Fri, Dec 20, 2013 at 5:52 PM, <iant@golang.org> wrote:

> LGTM
>
> Right now, of course, we never generate the dead value map.  I don't
> fully grasp Carl's comment about when we will want to generate it--is
> there going to be a time in the future when we always generate it?
>

My understanding is that they are for debugging: the debugging mode (which
may involve code not checked in) memsets dead values to a recognizable bit
pattern so that any uses can be detected easily. So I do not expect there
will be a time when they are always generated. Of course, if that's wrong,
this change does no harm.

Russ

[rsc, 2013-12-21 00:14:46]

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

cmd/gc: do not compute dead value maps if they will not be used

Reduces 6g big.go memory usage from 251 MB to 242 MB.
Reduces 6g slow.go memory usage from 529 MB to 453 MB.

Mostly a memory savings; 6g slow.go is only about 5% faster.

The test programs are at
https://rsc.googlecode.com/hg/testdata/big.go (36k lines, 276kB)
https://rsc.googlecode.com/hg/testdata/slow.go (7k lines, 352kB)

R=golang-codereviews, bradfitz, iant
CC=golang-codereviews
https://codereview.appspot.com/42280045