code review 9791044: runtime: allocate page table lazily

runtime: allocate page table lazily
This removes the 256MB memory allocation at startup,
which conflicts with ulimit.
Also will allow to eliminate an unnecessary memory dereference in GC,
because the page table is usually mapped at known address.
Update issue 5049.
Update issue 5236.

[dvyukov, 2013-05-27 06:51:19]

Hello golang-dev@googlegroups.com,

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

[khr, 2013-05-28 06:01:57]

The code looks good, but I don't like the naming.  Things like "MapMap" make me
cringe.  The array of pointers to MSpans should really be called "spans",
although I'm willing to wait for a separate CL for that.

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

https://codereview.appspot.com/9791044/diff/8001/src/pkg/runtime/malloc.goc#n...
src/pkg/runtime/malloc.goc:314: uintptr arena_size, bitmap_size, map_size;
span_size?

https://codereview.appspot.com/9791044/diff/8001/src/pkg/runtime/malloc.h
File src/pkg/runtime/malloc.h (right):

https://codereview.appspot.com/9791044/diff/8001/src/pkg/runtime/malloc.h#new...
src/pkg/runtime/malloc.h:415: uintptr	map_mapped;
I don't like the name map_mapped.  Maybe "spans_mapped" (in preparation to
rename "map" to "spans", which doesn't need to be in this change)?

https://codereview.appspot.com/9791044/diff/8001/src/pkg/runtime/malloc.h#new...
src/pkg/runtime/malloc.h:446: void	runtime·MHeap_MapMap(MHeap *h);
Same here, "MapMap" -> "EnsureSpansAreMapped"?

[dvyukov, 2013-05-28 06:52:16]

On 2013/05/28 06:01:57, khr wrote:
> The code looks good, but I don't like the naming.  Things like "MapMap" make
me
> cringe.  The array of pointers to MSpans should really be called "spans",
> although I'm willing to wait for a separate CL for that.
> 
> https://codereview.appspot.com/9791044/diff/8001/src/pkg/runtime/malloc.goc
> File src/pkg/runtime/malloc.goc (right):
> 
>
https://codereview.appspot.com/9791044/diff/8001/src/pkg/runtime/malloc.goc#n...
> src/pkg/runtime/malloc.goc:314: uintptr arena_size, bitmap_size, map_size;
> span_size?
> 
> https://codereview.appspot.com/9791044/diff/8001/src/pkg/runtime/malloc.h
> File src/pkg/runtime/malloc.h (right):
> 
>
https://codereview.appspot.com/9791044/diff/8001/src/pkg/runtime/malloc.h#new...
> src/pkg/runtime/malloc.h:415: uintptr	map_mapped;
> I don't like the name map_mapped.  Maybe "spans_mapped" (in preparation to
> rename "map" to "spans", which doesn't need to be in this change)?
> 
>
https://codereview.appspot.com/9791044/diff/8001/src/pkg/runtime/malloc.h#new...
> src/pkg/runtime/malloc.h:446: void	runtime·MHeap_MapMap(MHeap *h);
> Same here, "MapMap" -> "EnsureSpansAreMapped"?



As per offline discussion, we decided to rename it to "spanarray", because it's
not actually "spans" nor "map".
The bulk of the renaming will be done in a separate CL.

[dvyukov, 2013-05-28 07:00:54]

PTAL
renamed it to spanarray

[r, 2013-05-28 12:54:33]

Names than state their type are weaker than names that explain their purpose. We
call a variable i or index, not countinteger, for clarity and to avoid
redundancy, and to me "spanarray" sounds like "countinteger". It's type is
array, its name should say something we don't already know.

[dvyukov, 2013-05-28 15:00:53]

On 2013/05/28 12:54:33, r wrote:
> Names than state their type are weaker than names that explain their purpose.
We
> call a variable i or index, not countinteger, for clarity and to avoid
> redundancy, and to me "spanarray" sounds like "countinteger". It's type is
> array, its name should say something we don't already know.

I think that the "span" part is good, we have spans only in a single place, so
it states both "type" and "purpose".
What about "spantab"?

[r, 2013-05-28 15:28:01]

'tab' is an alias for 'array'. how about 'spans'?

[khr1, 2013-05-28 15:51:03]

Or "spanlookup", as that's what it is for.  "spans" was my original
suggestion...


On Tue, May 28, 2013 at 8:28 AM, <r@golang.org> wrote:

> 'tab' is an alias for 'array'. how about 'spans'?
>
>
>
https://codereview.appspot.**com/9791044/<https://codereview.appspot.com/9791...
>

[dvyukov, 2013-05-28 16:41:22]

OK, let it be "spans".
PTAL.

[khr, 2013-05-28 16:44:59]

LGTM

[rsc, 2013-05-28 16:47:36]

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

> Also will allow to eliminate an unnecessary memory dereference in GC,
> because the page table is usually mapped at known address.
>

How does "usually" make a memory dereference unnecessary?

Russ

[dvyukov, 2013-05-28 17:01:46]

On Tue, May 28, 2013 at 8:47 PM, Russ Cox <rsc@golang.org> wrote:

> On Mon, May 27, 2013 at 2:51 AM, <dvyukov@google.com> wrote:
>
>> Also will allow to eliminate an unnecessary memory dereference in GC,
>> because the page table is usually mapped at known address.
>>
>
> How does "usually" make a memory dereference unnecessary?
>
>
Currently it's unfeasible, but if/when the GC inner loop is simpler (like
what it was before precise GC), we can have 2 inner loops: one that
reads runtime·mheap->spans
and the second with the hardcoded constant.

[dvyukov, 2013-05-28 18:04:40]

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

runtime: allocate page table lazily
This removes the 256MB memory allocation at startup,
which conflicts with ulimit.
Also will allow to eliminate an unnecessary memory dereference in GC,
because the page table is usually mapped at known address.
Update issue 5049.
Update issue 5236.

R=golang-dev, khr, r, khr, rsc
CC=golang-dev
https://codereview.appspot.com/9791044