code review 9778049: runtime: introduce helper persistentalloc() function

runtime: introduce helper persistentalloc() function
It is a caching wrapper around SysAlloc() that can allocate small chunks.
Use it for symtab allocations. Reduces number of symtab walks from 4 to 3
(reduces buildfuncs time from 10ms to 7.5ms on a large binary,
reduces initial heap size by 680K on the same binary).
Also can be used for type info allocation, itab allocation.
There are also several places in GC where we do the same thing,
they can be changed to use persistentalloc().
Also can be used in FixAlloc, because each instance of FixAlloc allocates
in 128K regions, which is too eager.
Reincarnation of committed and rolled back https://codereview.appspot.com/9805043
The latent bugs that it revealed are fixed:
https://codereview.appspot.com/9837049
https://codereview.appspot.com/9778048

[dvyukov, 2013-05-30 17:32:04]

Hello golang-dev@googlegroups.com,

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

[khr1, 2013-05-30 19:48:16]

LGTM

Do you have bug #s / CL #s for the latent bugs that were fixed?


On Thu, May 30, 2013 at 10:32 AM, <dvyukov@google.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
>
https://dvyukov%40google.com@**code.google.com/p/go/<http://40google.com@code...
>
>
> Description:
> runtime: introduce helper persistentalloc() function
> It is a caching wrapper around SysAlloc() that can allocate small
> chunks.
> Use it for symtab allocations. Reduces number of symtab walks from 4 to
> 3
> (reduces buildfuncs time from 10ms to 7.5ms on a large binary,
> reduces initial heap size by 680K on the same binary).
> Also can be used for type info allocation, itab allocation.
> There are also several places in GC where we do the same thing,
> they can be changed to use persistentalloc().
> Also can be used in FixAlloc, because each instance of FixAlloc
> allocates
> in 128K regions, which is too eager.
> Reincarnation of committed and rolled back
>
https://codereview.appspot.**com/9805043<https://codereview.appspot.com/9805043>
> The latent bugs that it revealed are fixed.
>
> Please review this at
https://codereview.appspot.**com/9778049/<https://codereview.appspot.com/9778...
>
> Affected files:
>   M src/pkg/runtime/malloc.goc
>   M src/pkg/runtime/malloc.h
>   M src/pkg/runtime/symtab.c
>
>
> Index: src/pkg/runtime/malloc.goc
> ==============================**==============================**=======
> --- a/src/pkg/runtime/malloc.goc
> +++ b/src/pkg/runtime/malloc.goc
> @@ -502,6 +502,53 @@
>         return p;
>  }
>
> +static struct
> +{
> +       Lock;
> +       byte*   pos;
> +       byte*   end;
> +} persistent;
> +
> +enum
> +{
> +       PersistentAllocChunk    = 256<<10,
> +       PersistentAllocMaxBlock = 64<<10,  // VM reservation granularity
> is 64K on windows
> +};
> +
> +// Wrapper around SysAlloc that can allocate small chunks.
> +// There is no associated free operation.
> +// Intended for things like function/type/debug-related persistent data.
> +// If align is 0, uses default align (currently 8).
> +void*
> +runtime·persistentalloc(**uintptr size, uintptr align)
> +{
> +       byte *p;
> +
> +       if(align) {
> +               if(align&(align-1))
> +                       runtime·throw("**persistentalloc: align is now a
> power of 2");
> +               if(align > PageSize)
> +                       runtime·throw("**persistentalloc: align is too
> large");
> +       } else
> +               align = 8;
> +       if(size >= PersistentAllocMaxBlock)
> +               return runtime·SysAlloc(size);
> +       runtime·lock(&persistent);
> +       persistent.pos = (byte*)ROUND((uintptr)**persistent.pos, align);
> +       if(persistent.pos + size > persistent.end) {
> +               persistent.pos = runtime·SysAlloc(**PersistentAllocChunk);
> +               if(persistent.pos == nil) {
> +                       runtime·unlock(&persistent);
> +                       runtime·throw("runtime: cannot allocate memory");
> +               }
> +               persistent.end = persistent.pos + PersistentAllocChunk;
> +       }
> +       p = persistent.pos;
> +       persistent.pos += size;
> +       runtime·unlock(&persistent);
> +       return p;
> +}
> +
>  static Lock settype_lock;
>
>  void
> Index: src/pkg/runtime/malloc.h
> ==============================**==============================**=======
> --- a/src/pkg/runtime/malloc.h
> +++ b/src/pkg/runtime/malloc.h
> @@ -447,6 +447,7 @@
>  void   runtime·MHeap_Scavenger(void);
>
>  void*  runtime·mallocgc(uintptr size, uint32 flag, int32 dogc, int32
> zeroed);
> +void*  runtime·persistentalloc(**uintptr size, uintptr align);
>  int32  runtime·mlookup(void *v, byte **base, uintptr *size, MSpan **s);
>  void   runtime·gc(int32 force);
>  void   runtime·markallocated(void *v, uintptr n, bool noptr);
> Index: src/pkg/runtime/symtab.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/symtab.c
> +++ b/src/pkg/runtime/symtab.c
> @@ -69,11 +69,6 @@
>
>  static uintptr mainoffset;
>
> -// A dynamically allocated string containing multiple substrings.
> -// Individual strings are slices of hugestring.
> -static String hugestring;
> -static int32 hugestring_len;
> -
>  extern void main·main(void);
>
>  static uintptr
> @@ -303,7 +298,6 @@
>         return p - buf;
>  }
>
> -// appends p to hugestring
>  static String
>  gostringn(byte *p, int32 l)
>  {
> @@ -311,13 +305,8 @@
>
>         if(l == 0)
>                 return runtime·emptystring;
> -       if(hugestring.str == nil) {
> -               hugestring_len += l;
> -               return runtime·emptystring;
> -       }
> -       s.str = hugestring.str + hugestring.len;
>         s.len = l;
> -       hugestring.len += s.len;
> +       s.str = runtime·persistentalloc(l, 1);
>         runtime·memmove(s.str, p, l);
>         return s;
>  }
> @@ -348,8 +337,6 @@
>         switch(sym->symtype) {
>         case 't':
>         case 'T':
> -               if(hugestring.str == nil)
> -                       break;
>                 if(runtime·strcmp(sym->name, (byte*)"etext") == 0)
>                         break;
>                 f = &func[nfunc++];
> @@ -574,11 +561,12 @@
>         walksymtab(dofunc);
>
>         // Initialize tables.
> -       // Can use FlagNoPointers - all pointers either point into
> sections of the executable
> -       // or point into hugestring.
> -       func = runtime·mallocgc((nfunc+1)***sizeof func[0],
> FlagNoPointers, 0, 1);
> +       // Memory obtained from runtime·persistentalloc() is not scanned
> by GC,
> +       // this is fine because all pointers either point into sections of
> the executable
> +       // or also obtained from persistentmalloc().
> +       func = runtime·persistentalloc((**nfunc+1)*sizeof func[0], 0);
>         func[nfunc].entry = (uint64)etext;
> -       fname = runtime·mallocgc(nfname*sizeof fname[0], FlagNoPointers,
> 0, 1);
> +       fname = runtime·persistentalloc(**nfname*sizeof fname[0], 0);
>         nfunc = 0;
>         lastvalue = 0;
>         walksymtab(dofunc);
> @@ -588,15 +576,9 @@
>
>         // record src file and line info for each func
>         files = runtime·malloc(maxfiles * sizeof(files[0]));
> -       walksymtab(dosrcline);  // pass 1: determine hugestring_len
> -       hugestring.str = runtime·mallocgc(hugestring_**len,
> FlagNoPointers, 0, 0);
> -       hugestring.len = 0;
> -       walksymtab(dosrcline);  // pass 2: fill and use hugestring
> +       walksymtab(dosrcline);
>         files = nil;
>
> -       if(hugestring.len != hugestring_len)
> -               runtime·throw("buildfunc: problem in initialization
> procedure");
> -
>         m->nomemprof--;
>  }
>
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[dvyukov, 2013-05-31 06:38:24]

On 2013/05/30 19:48:16, khr1 wrote:
> LGTM
> 
> Do you have bug #s / CL #s for the latent bugs that were fixed?


Updated CL description with CL numbers.

[dvyukov, 2013-05-31 06:43:04]

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

runtime: introduce helper persistentalloc() function
It is a caching wrapper around SysAlloc() that can allocate small chunks.
Use it for symtab allocations. Reduces number of symtab walks from 4 to 3
(reduces buildfuncs time from 10ms to 7.5ms on a large binary,
reduces initial heap size by 680K on the same binary).
Also can be used for type info allocation, itab allocation.
There are also several places in GC where we do the same thing,
they can be changed to use persistentalloc().
Also can be used in FixAlloc, because each instance of FixAlloc allocates
in 128K regions, which is too eager.
Reincarnation of committed and rolled back
https://codereview.appspot.com/9805043
The latent bugs that it revealed are fixed:
https://codereview.appspot.com/9837049
https://codereview.appspot.com/9778048

R=golang-dev, khr
CC=golang-dev
https://codereview.appspot.com/9778049

[iant, 2013-05-31 13:33:58]

https://codereview.appspot.com/9778049/diff/10001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/9778049/diff/10001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:523: if(align) {
Write if(align != 0) { or if(align > 0) {

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

https://codereview.appspot.com/9778049/diff/10001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:566: // or also obtained from persistentmalloc().
s/persistentmalloc/persistentalloc/

[dvyukov, 2013-06-01 21:35:30]

Sent https://codereview.appspot.com/9693044

On Fri, May 31, 2013 at 5:33 PM,  <iant@golang.org> wrote:
>
> https://codereview.appspot.com/9778049/diff/10001/src/pkg/runtime/malloc.goc
> File src/pkg/runtime/malloc.goc (right):
>
>
https://codereview.appspot.com/9778049/diff/10001/src/pkg/runtime/malloc.goc#...
> src/pkg/runtime/malloc.goc:523: if(align) {
> Write if(align != 0) { or if(align > 0) {
>
> https://codereview.appspot.com/9778049/diff/10001/src/pkg/runtime/symtab.c
> File src/pkg/runtime/symtab.c (right):
>
>
https://codereview.appspot.com/9778049/diff/10001/src/pkg/runtime/symtab.c#ne...
> src/pkg/runtime/symtab.c:566: // or also obtained from
> persistentmalloc().
> s/persistentmalloc/persistentalloc/
>
> https://codereview.appspot.com/9778049/