code review 15930045: cmd/gc: use 100x less memory for []byte("string")

cmd/gc: use 100x less memory for []byte("string")

[]byte("string") was simplifying to
[]byte{0: 0x73, 1: 0x74, 2: 0x72, 3: 0x69, 4: 0x6e, 5: 0x67},
but that latter form takes up much more memory in the compiler.
Preserve the string form and recognize it to turn global variables
initialized this way into linker-initialized data.

Reduces the compiler memory footprint for a large []byte initialized
this way from approximately 10 kB/B to under 100 B/B.

See also issue 6643.

[rsc, 2013-10-23 03:41:52]

Hello golang-dev@googlegroups.com,

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

[r, 2013-10-23 04:53:49]

Can we do this after 1.2?

[iant, 2013-10-23 14:08:55]

LGTM

[rsc, 2013-10-23 14:16:13]

I'm on the fence about Go 1.2. If you'd rather wait, that's fine.

[r, 2013-10-23 14:28:15]

I would prefer to wait.

-rob

[oleku, 2013-10-23 15:51:35]

Is there any particular reason why you would prefer it to wait ? Just 
curious 

Thanks 


On Wednesday, October 23, 2013 3:27:53 PM UTC+1, Rob Pike wrote:
>
> I would prefer to wait.
>
> -rob
>
>

[rsc, 2013-10-23 15:54:43]

On Wed, Oct 23, 2013 at 11:51 AM, Oleku Konko <oleku.konko@gmail.com> wrote:

> Is there any particular reason why you would prefer it to wait ? Just
> curious
>

It's a subtle change to a subtle piece of code. If it were only a 2x or 10x
improvement I would definitely wait. 100x is a bit more borderline, but I'm
still happy to wait.

Russ

[r, 2013-10-23 16:14:08]

There's no point in having a release freeze if we don't freeze the compiler.

-rob

[rsc, 2013-10-23 16:34:49]

To be clear, the only reason we'd even consider putting this in Go 1.2 is
if it was a critical bug fix for some reason, like it was causing
compilations to fail due to being out of memory on a real-world program.
Since no one has complained about it to date, and since the current
behavior has existed since before Go 1, it can probably wait for Go 1.3.

Russ

[oleku, 2013-10-23 17:19:03]

I see your point and appreciate keeping to the rules .... Is there a clear 
calender for release timelines ? 
Thanks 

On Wednesday, October 23, 2013 5:13:46 PM UTC+1, Rob Pike wrote:
>
> There's no point in having a release freeze if we don't freeze the 
> compiler.
>
> -rob
>
>

[oleku, 2013-10-23 17:20:24]

Does it have to be 1.3 ???  What about 1.2.1 etc ??? Would the compiler 
freeze still be in effect ? 

On Wednesday, October 23, 2013 5:34:47 PM UTC+1, rsc wrote:
>
> To be clear, the only reason we'd even consider putting this in Go 1.2 is 
> if it was a critical bug fix for some reason, like it was causing 
> compilations to fail due to being out of memory on a real-world program. 
> Since no one has complained about it to date, and since the current 
> behavior has existed since before Go 1, it can probably wait for Go 1.3.
>
> Russ
>

[rsc, 2013-10-23 17:20:36]

On Wed, Oct 23, 2013 at 1:19 PM, Oleku Konko <oleku.konko@gmail.com> wrote:

> I see your point and appreciate keeping to the rules .... Is there a clear
> calender for release timelines ?
> Thanks
>

https://docs.google.com/document/d/106hMEZj58L9nq9N9p7Zll_WKfo-oyZHFyI6MttuZm...

Russ

[oleku, 2013-10-24 00:02:22]

Thank you very much ... 

On Wednesday, October 23, 2013 6:20:34 PM UTC+1, rsc wrote:
>
> On Wed, Oct 23, 2013 at 1:19 PM, Oleku Konko <oleku...@gmail.com<javascript:>
> > wrote:
>
>> I see your point and appreciate keeping to the rules .... Is there a 
>> clear calender for release timelines ? 
>> Thanks 
>>
>
>
>
https://docs.google.com/document/d/106hMEZj58L9nq9N9p7Zll_WKfo-oyZHFyI6MttuZm...
>
> Russ
>
>

[dave_cheney.net, 2013-11-14 08:14:26]

Just a note to say I have been using this on my linux/amd64 system without
incident since the CL was mailed.

[gobot, 2013-12-20 01:52:55]

R=iant@golang.org (assigned by rsc@golang.org)

[gobot, 2013-12-20 02:44:39]

Replacing golang-dev with golang-codereviews.

[iant, 2013-12-20 16:49:03]

LGTM

[dave_cheney.net, 2013-12-28 04:43:41]

@rsc, would you be able to merge this soon ?

On Sat, Dec 21, 2013 at 3:49 AM,  <iant@golang.org> wrote:
> LGTM
>
> https://codereview.appspot.com/15930045/

[rsc, 2014-01-06 20:22:21]

On Fri, Dec 27, 2013 at 11:43 PM, Dave Cheney <dave@cheney.net> wrote:

> @rsc, would you be able to merge this soon ?
>

With the new linker changes it should have less effect than it used to.
Still some probably, but less. I haven't had a chance to measure whether it
is still worthwhile. Would you like to?

Thanks.
Russ

[dave_cheney.net, 2014-01-06 20:24:16]

Yes please! Can you freshen the patch so it will clpatch cleanly again.


On Tue, Jan 7, 2014 at 7:22 AM, Russ Cox <rsc@golang.org> wrote:

> On Fri, Dec 27, 2013 at 11:43 PM, Dave Cheney <dave@cheney.net> wrote:
>
>> @rsc, would you be able to merge this soon ?
>>
>
> With the new linker changes it should have less effect than it used to.
> Still some probably, but less. I haven't had a chance to measure whether it
> is still worthwhile. Would you like to?
>
> Thanks.
> Russ
>
>

[bradfitz, 2014-01-06 20:25:47]

You can just:

$ curl https://codereview.appspot.com/download/issue15930045_40001.diff |
patch -p1

Only one hunk fails and it's trivial to add yourself:

$ cat src/cmd/gc/go.h.rej
--- src/cmd/gc/go.h
+++ src/cmd/gc/go.h
@@ -1250,6 +1250,7 @@
 void dumpobj(void);
 void ieeedtod(uint64 *ieee, double native);
 Sym* stringsym(char*, int);
+void slicebytes(Node*, char*, int);

 /*



On Mon, Jan 6, 2014 at 12:24 PM, Dave Cheney <dave@cheney.net> wrote:

> Yes please! Can you freshen the patch so it will clpatch cleanly again.
>
>
> On Tue, Jan 7, 2014 at 7:22 AM, Russ Cox <rsc@golang.org> wrote:
>
>> On Fri, Dec 27, 2013 at 11:43 PM, Dave Cheney <dave@cheney.net> wrote:
>>
>>> @rsc, would you be able to merge this soon ?
>>>
>>
>> With the new linker changes it should have less effect than it used to.
>> Still some probably, but less. I haven't had a chance to measure whether it
>> is still worthwhile. Would you like to?
>>
>> Thanks.
>> Russ
>>
>>
>  --
> 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.
>

[dave_cheney.net, 2014-01-07 01:11:35]

Hi Russ and Brad,

I tried to measure this change today but I think the linker changes have
neutered it. Placing a breakpoint on slicebytes does not yield any
invocations when compiling large packages like
code.google.com/p/go.text/collate.

Do you have any suggestions how I can reactivate the feature ?

Cheers

Dave


On Tue, Jan 7, 2014 at 7:25 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> You can just:
>
> $ curl https://codereview.appspot.com/download/issue15930045_40001.diff |
> patch -p1
>
> Only one hunk fails and it's trivial to add yourself:
>
> $ cat src/cmd/gc/go.h.rej
> --- src/cmd/gc/go.h
> +++ src/cmd/gc/go.h
> @@ -1250,6 +1250,7 @@
>  void dumpobj(void);
>  void ieeedtod(uint64 *ieee, double native);
>  Sym* stringsym(char*, int);
> +void slicebytes(Node*, char*, int);
>
>  /*
>
>
>
> On Mon, Jan 6, 2014 at 12:24 PM, Dave Cheney <dave@cheney.net> wrote:
>
>> Yes please! Can you freshen the patch so it will clpatch cleanly again.
>>
>>
>> On Tue, Jan 7, 2014 at 7:22 AM, Russ Cox <rsc@golang.org> wrote:
>>
>>> On Fri, Dec 27, 2013 at 11:43 PM, Dave Cheney <dave@cheney.net> wrote:
>>>
>>>> @rsc, would you be able to merge this soon ?
>>>>
>>>
>>> With the new linker changes it should have less effect than it used to.
>>> Still some probably, but less. I haven't had a chance to measure whether it
>>> is still worthwhile. Would you like to?
>>>
>>> Thanks.
>>> Russ
>>>
>>>
>>  --
>> 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.
>>
>
>

[dave_cheney.net, 2014-01-07 01:29:33]

I've done some measurements of this CL and I believe it is still useful to
apply.

In general there is no measurable improvement in `go install -a std` or in any
of the tests like rotateN.go or 64bit.go with this CL applied.

On the other hand there is regression either.

Generating a specific test case shows the benefit of this CL.

Before:
lucky(~/src) % /usr/bin/time -v /home/dfc/go/pkg/tool/linux_amd64/6g -o
$WORK/command-line-arguments.a -p command-line-arguments -complete -D
_/home/dfc/src -I $WORK -pack ./moby.go
        Command being timed: "/home/dfc/go/pkg/tool/linux_amd64/6g -o
/tmp/go-build656226913/command-line-arguments.a -p command-line-arguments
-complete -D _/home/dfc/src -I /tmp/go-build656226913 -pack ./moby.go"
        User time (seconds): 6.46
        System time (seconds): 1.21
        Percent of CPU this job got: 76%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:09.98
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 3661332
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 915600
        Voluntary context switches: 73
        Involuntary context switches: 678
        Swaps: 0
        File system inputs: 0
        File system outputs: 0
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

After:
lucky(~/src) % /usr/bin/time -v /home/dfc/go/pkg/tool/linux_amd64/6g -o
$WORK/command-line-arguments.a -p command-line-arguments -complete -D
_/home/dfc/src -I $WORK -pack ./moby.go
        Command being timed: "/home/dfc/go/pkg/tool/linux_amd64/6g -o
/tmp/go-build656226913/command-line-arguments.a -p command-line-arguments
-complete -D _/home/dfc/src -I /tmp/go-build656226913 -pack ./moby.go"
        User time (seconds): 0.07
        System time (seconds): 0.02
        Percent of CPU this job got: 98%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.09
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 57608
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 14621
        Voluntary context switches: 1
        Involuntary context switches: 10
        Swaps: 0
        File system inputs: 0
        File system outputs: 0
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

So 3.6Gb RSS -> 57Mb RSS to compile Herman Melville's classic sounds like a win.

[rsc, 2014-01-07 01:43:44]

Thanks!

[rsc, 2014-01-07 01:43:52]

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

cmd/gc: use 100x less memory for []byte("string")

[]byte("string") was simplifying to
[]byte{0: 0x73, 1: 0x74, 2: 0x72, 3: 0x69, 4: 0x6e, 5: 0x67},
but that latter form takes up much more memory in the compiler.
Preserve the string form and recognize it to turn global variables
initialized this way into linker-initialized data.

Reduces the compiler memory footprint for a large []byte initialized
this way from approximately 10 kB/B to under 100 B/B.

See also issue 6643.

R=golang-codereviews, r, iant, oleku.konko, dave, gobot, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/15930045