code review 9786044: runtime: flag static variables as no-pointers

runtime: flag static variables as no-pointers

Variables in data sections of 32-bit executables interfere with
garbage collector's ability to free objects and/or unnecessarily
slow down the garbage collector.

This changeset moves some static variables to .noptr sections.
'files' in symtab.c is now allocated dynamically.

[atom, 2013-05-26 16:05:34]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-05-26 16:11:06]

Does not linker figure out that an array of int8 is NoPointers?

[minux1, 2013-05-26 16:12:34]

On Mon, May 27, 2013 at 12:11 AM, <dvyukov@google.com> wrote:

> Does not linker figure out that an array of int8 is NoPointers?
>
The C compilers don't generate typeinfo.

[minux1, 2013-05-26 16:14:22]

https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (left):

https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c#old...
src/pkg/runtime/symtab.c:322: } files[200];
why use malloc for files?

[dvyukov, 2013-05-26 16:16:06]

https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:316: const int32 maxfiles = 200;
we do not use const, please use enum.

[dvyukov, 2013-05-26 16:17:14]

On 2013/05/26 16:14:22, minux wrote:
> https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c
> File src/pkg/runtime/symtab.c (left):
> 
>
https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c#old...
> src/pkg/runtime/symtab.c:322: } files[200];
> why use malloc for files?

I guess to not scan it during every GC for the whole duration of the program.

[dvyukov, 2013-05-26 16:18:17]

On 2013/05/26 16:12:34, minux wrote:
> On Mon, May 27, 2013 at 12:11 AM, <mailto:dvyukov@google.com> wrote:
> 
> > Does not linker figure out that an array of int8 is NoPointers?
> >
> The C compilers don't generate typeinfo.

The linker conservatively marks all C objects as pointers, right?

[atom, 2013-05-26 16:20:45]

On 2013/05/26 16:17:14, dvyukov wrote:
> On 2013/05/26 16:14:22, minux wrote:
> > https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c
> > File src/pkg/runtime/symtab.c (left):
> > 
> >
>
https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c#old...
> > src/pkg/runtime/symtab.c:322: } files[200];
> > why use malloc for files?
> 
> I guess to not scan it during every GC for the whole duration of the program.

Yes.

[minux1, 2013-05-26 16:22:45]

On Mon, May 27, 2013 at 12:17 AM, <dvyukov@google.com> wrote:

> On 2013/05/26 16:14:22, minux wrote:
>
> https://codereview.appspot.**com/9786044/diff/5001/src/pkg/**
>
runtime/symtab.c<https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c>
>
>> File src/pkg/runtime/symtab.c (left):
>>
>
>
> https://codereview.appspot.**com/9786044/diff/5001/src/pkg/**
>
runtime/symtab.c#oldcode322<https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c#oldcode322>
>
>> src/pkg/runtime/symtab.c:322: } files[200];
>> why use malloc for files?
>>
>
> I guess to not scan it during every GC for the whole duration of the
> program.
>
why not just put them into .noptrbss?
because they are temporary data?

[atom, 2013-05-26 16:24:01]

https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:316: const int32 maxfiles = 200;
On 2013/05/26 16:16:06, dvyukov wrote:
> we do not use const, please use enum.

It appears the dist tool fails to handle enums:

go tool dist: FAILED: ... runtime/symtab.c

[dvyukov, 2013-05-26 16:28:33]

https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:316: const int32 maxfiles = 200;
On 2013/05/26 16:24:01, atom wrote:
> On 2013/05/26 16:16:06, dvyukov wrote:
> > we do not use const, please use enum.
> 
> It appears the dist tool fails to handle enums:
> 
> go tool dist: FAILED: ... runtime/symtab.c

... but... how does it handle all other enums?

[atom, 2013-05-26 16:33:42]

On 2013/05/26 16:22:45, minux wrote:
> On Mon, May 27, 2013 at 12:17 AM, <mailto:dvyukov@google.com> wrote:
> 
> > On 2013/05/26 16:14:22, minux wrote:
> >
> > https://codereview.appspot.**com/9786044/diff/5001/src/pkg/**
> >
>
runtime/symtab.c<https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c>
> >
> >> File src/pkg/runtime/symtab.c (left):
> >>
> >
> >
> > https://codereview.appspot.**com/9786044/diff/5001/src/pkg/**
> >
>
runtime/symtab.c#oldcode322<https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c#oldcode322>
> >
> >> src/pkg/runtime/symtab.c:322: } files[200];
> >> why use malloc for files?
> >>
> >
> > I guess to not scan it during every GC for the whole duration of the
> > program.
> >
> why not just put them into .noptrbss?
> because they are temporary data?

The field files.srcstring contains a pointer. It isn't impossible for garbage
collection to start while running the dosrcline() function.

[atom, 2013-05-26 16:37:16]

https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9786044/diff/5001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:316: const int32 maxfiles = 200;
On 2013/05/26 16:28:33, dvyukov wrote:
> On 2013/05/26 16:24:01, atom wrote:
> > On 2013/05/26 16:16:06, dvyukov wrote:
> > > we do not use const, please use enum.
> > 
> > It appears the dist tool fails to handle enums:
> > 
> > go tool dist: FAILED: ... runtime/symtab.c
> 
> ... but... how does it handle all other enums?

... the error was caused a missing ;

[dvyukov, 2013-05-26 16:41:26]

LGTM with nitpicks

https://codereview.appspot.com/9786044/diff/15001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9786044/diff/15001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:310: static struct {
{ on next line for structs

https://codereview.appspot.com/9786044/diff/15001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:316: enum {
{ on next line
or everything on the same line
we use both variants

[atom, 2013-05-26 16:47:37]

https://codereview.appspot.com/9786044/diff/15001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9786044/diff/15001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:310: static struct {
On 2013/05/26 16:41:26, dvyukov wrote:
> { on next line for structs

Done.

https://codereview.appspot.com/9786044/diff/15001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:316: enum {
On 2013/05/26 16:41:26, dvyukov wrote:
> { on next line
> or everything on the same line
> we use both variants

Done.

[atom, 2013-05-26 18:06:23]

On 2013/05/26 16:41:26, dvyukov wrote:
> LGTM with nitpicks
> 
> https://codereview.appspot.com/9786044/diff/15001/src/pkg/runtime/symtab.c
> File src/pkg/runtime/symtab.c (right):
> 
>
https://codereview.appspot.com/9786044/diff/15001/src/pkg/runtime/symtab.c#ne...
> src/pkg/runtime/symtab.c:310: static struct {
> { on next line for structs
> 
>
https://codereview.appspot.com/9786044/diff/15001/src/pkg/runtime/symtab.c#ne...
> src/pkg/runtime/symtab.c:316: enum {
> { on next line
> or everything on the same line
> we use both variants

I suppose I can run hg submit.

[atom, 2013-05-27 06:12:26]

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

runtime: flag static variables as no-pointers

Variables in data sections of 32-bit executables interfere with
garbage collector's ability to free objects and/or unnecessarily
slow down the garbage collector.

This changeset moves some static variables to .noptr sections.
'files' in symtab.c is now allocated dynamically.

R=golang-dev, dvyukov, minux.ma
CC=golang-dev
https://codereview.appspot.com/9786044