Extend the maximum size map space...

Extend the maximum size map space

On 32-bit the maps are now aligned on a 32-byte boundary in order to encode more maps during compacting GC. The actual size of a map on 32-bit is 28 bytes making this change waste 4 bytes per map.

On 64-bit the encoding for compacting GC is now using more than 32-bits and the maps here are still pointer size aligned. The actual size of a map on 64-bit is 48 bytes and this change does not intruduce any waste.

My choice of 16 bits for kMapPageIndexBits for 64-bit should give the same maximum number of pages (8K) for map space. As maps on 64-bit are larger than on 32-bit the total number of maps on 64-bit will be smaller than on 32-bit. We could consider raising this to 17 or 18.

I moved the kPageSizeBits to globals.h as the calculation of the encoding really depended on this.

There are still an #ifdef/#endif in objects.h and this constant could be moved to globaks.h as well, but I kept it together with the related constants.

All the tests run in debug mode with additional options --gc-global --always-compact as well (except for a few tests on which also fails before this change when run with --gc-global --always-compact).

BUG=http://code.google.com/p/v8/issues/detail?id=524
BUG=http://crbug.com/29428
TEST=test/mjsunit/regress/regress-524.js


Committed: http://code.google.com/p/v8/source/detail?r=3481

[antonm, 2009-12-16 14:08:43]

http://codereview.chromium.org/504026/diff/3001/3002
File src/heap.cc (right):

http://codereview.chromium.org/504026/diff/3001/3002#newcode1146
src/heap.cc:1146: // If the map bject is aligned fill the padding area with Smi
0 objects.
nit: <o>bject

http://codereview.chromium.org/504026/diff/3001/3005
File src/objects.h (right):

http://codereview.chromium.org/504026/diff/3001/3005#newcode902
src/objects.h:902: //          |                 |          kMapPageIndexBits
may you also update (or just remove and add a reference) text in mark-compact.cc
(around ln. 834) and in spaces.h (ln. 109)?

[Mads Ager (chromium), 2009-12-16 14:19:16]

LGTM

http://codereview.chromium.org/504026/diff/3001/3010
File src/globals.h (right):

http://codereview.chromium.org/504026/diff/3001/3010#newcode186
src/globals.h:186: static const int kPageSizeBits = 13;
Get rid of static?  Add a bit more information to the comment?

http://codereview.chromium.org/504026/diff/3001/3010#newcode465
src/globals.h:465: // POINTER_SIZE_ALIGN returns the value aligned as a pointer.
POINTER -> MAP
pointer -> map

http://codereview.chromium.org/504026/diff/3001/3002
File src/heap.cc (right):

http://codereview.chromium.org/504026/diff/3001/3002#newcode1146
src/heap.cc:1146: // If the map bject is aligned fill the padding area with Smi
0 objects.
bject -> object

http://codereview.chromium.org/504026/diff/3001/3008
File src/objects-inl.h (right):

http://codereview.chromium.org/504026/diff/3001/3008#newcode979
src/objects-inl.h:979: << kMapAlignmentBits);
I would move the << to the previous line.

http://codereview.chromium.org/504026/diff/3001/3011
File test/mjsunit/regress/regress-524.js (right):

http://codereview.chromium.org/504026/diff/3001/3011#newcode30
test/mjsunit/regress/regress-524.js:30: var i = 500000
Does this run fast enough in debug mode with extra checking that it will not
cause timeouts?

[Søren Thygesen Gjesse, 2009-12-16 15:10:30]

http://codereview.chromium.org/504026/diff/3001/3010
File src/globals.h (right):

http://codereview.chromium.org/504026/diff/3001/3010#newcode186
src/globals.h:186: static const int kPageSizeBits = 13;
On 2009/12/16 14:19:16, Mads Ager wrote:
> Get rid of static?  Add a bit more information to the comment?

Done.

http://codereview.chromium.org/504026/diff/3001/3010#newcode465
src/globals.h:465: // POINTER_SIZE_ALIGN returns the value aligned as a pointer.
On 2009/12/16 14:19:16, Mads Ager wrote:
> POINTER -> MAP
> pointer -> map

Done.

http://codereview.chromium.org/504026/diff/3001/3002
File src/heap.cc (right):

http://codereview.chromium.org/504026/diff/3001/3002#newcode1146
src/heap.cc:1146: // If the map bject is aligned fill the padding area with Smi
0 objects.
On 2009/12/16 14:08:43, antonm wrote:
> nit: <o>bject

Done.

http://codereview.chromium.org/504026/diff/3001/3002#newcode1146
src/heap.cc:1146: // If the map bject is aligned fill the padding area with Smi
0 objects.
On 2009/12/16 14:19:16, Mads Ager wrote:
> bject -> object

Done.

http://codereview.chromium.org/504026/diff/3001/3008
File src/objects-inl.h (right):

http://codereview.chromium.org/504026/diff/3001/3008#newcode979
src/objects-inl.h:979: << kMapAlignmentBits);
On 2009/12/16 14:19:16, Mads Ager wrote:
> I would move the << to the previous line.

Done.

http://codereview.chromium.org/504026/diff/3001/3005
File src/objects.h (right):

http://codereview.chromium.org/504026/diff/3001/3005#newcode902
src/objects.h:902: //          |                 |          kMapPageIndexBits
On 2009/12/16 14:08:43, antonm wrote:
> may you also update (or just remove and add a reference) text in
mark-compact.cc
> (around ln. 834) and in spaces.h (ln. 109)?

Thanks for spotting this redundancy. Replaced the other comments with a
reference.

http://codereview.chromium.org/504026/diff/3001/3011
File test/mjsunit/regress/regress-524.js (right):

http://codereview.chromium.org/504026/diff/3001/3011#newcode30
test/mjsunit/regress/regress-524.js:30: var i = 500000
On 2009/12/16 14:19:16, Mads Ager wrote:
> Does this run fast enough in debug mode with extra checking that it will not
> cause timeouts?

It runs in ~25s in debug mode on my machine (~1s in release mode). I will just
skip it debug mode (if I can get the mjsunit.status syntax correct).

[antonm, 2009-12-16 15:18:22]

LGTM

just curious: do you know if it fixes the Chromium issue w/ dashboard?

[antonm, 2009-12-16 16:26:30]

http://codereview.chromium.org/504026/diff/6001/6004
File src/spaces.h (left):

http://codereview.chromium.org/504026/diff/6001/6004#oldcode1744
src/spaces.h:1744: static const int kMaxMapPageIndex = (1 <<
MapWord::kMapPageIndexBits) - 1;
BTW, do I miss something or this constant should be increased to be something
like (1 << (MapWord::kMapPageIndexBits + kMapAlignmentBits -
kObjectAlignmentBits))?

If yes, could you merge them to get rid of duplication?

[Søren Thygesen Gjesse, 2009-12-16 22:19:41]

http://codereview.chromium.org/504026/diff/6001/6004
File src/spaces.h (left):

http://codereview.chromium.org/504026/diff/6001/6004#oldcode1744
src/spaces.h:1744: static const int kMaxMapPageIndex = (1 <<
MapWord::kMapPageIndexBits) - 1;
On 2009/12/16 16:26:30, antonm wrote:
> BTW, do I miss something or this constant should be increased to be something
> like (1 << (MapWord::kMapPageIndexBits + kMapAlignmentBits -
> kObjectAlignmentBits))?
> 
> If yes, could you merge them to get rid of duplication?

This is correct, as we need one entry for each possible page of map space. What
is wrong however is the calculation of kMaxMapSpaceSize which should be

  static const int kMaxMapSpaceSize =
      (1 << (MapWord::kMapPageIndexBits)) * Page::kPageSize;

Thanks for spotting this inconsistency.

[antonm, 2009-12-17 06:49:45]

http://codereview.chromium.org/504026/diff/6001/6005
File src/objects.h (right):

http://codereview.chromium.org/504026/diff/6001/6005#newcode919
src/objects.h:919: #ifndef V8_HOST_ARCH_64_BIT
just an idea: maybe instead of STATIC_CHECK declare kMapPageIndexBits as 32 -
(kForwardingOffsetBits + kMapPageOffsetBits)?

Up to you.

http://codereview.chromium.org/504026/diff/6001/6005#newcode925
src/objects.h:925: // Bit masks covering the different parts the encodeding.
nit: encod<de>ing

http://codereview.chromium.org/504026/diff/6001/6004
File src/spaces.h (left):

http://codereview.chromium.org/504026/diff/6001/6004#oldcode1744
src/spaces.h:1744: static const int kMaxMapPageIndex = (1 <<
MapWord::kMapPageIndexBits) - 1;
On 2009/12/16 22:19:41, Søren Gjesse wrote:
> On 2009/12/16 16:26:30, antonm wrote:
> > BTW, do I miss something or this constant should be increased to be
something
> > like (1 << (MapWord::kMapPageIndexBits + kMapAlignmentBits -
> > kObjectAlignmentBits))?
> > 
> > If yes, could you merge them to get rid of duplication?
> 
> This is correct, as we need one entry for each possible page of map space.
What
> is wrong however is the calculation of kMaxMapSpaceSize which should be
> 
>   static const int kMaxMapSpaceSize =
>       (1 << (MapWord::kMapPageIndexBits)) * Page::kPageSize;
> 
> Thanks for spotting this inconsistency.

You're right. 

The fact that Chromium didn't crash for dashboard makes me somewhat uneasy as
map space might have grown to the size when pointers are not encodable (but
probably this point wasn't reached).

http://codereview.chromium.org/504026/diff/6001/6004
File src/spaces.h (right):

http://codereview.chromium.org/504026/diff/6001/6004#newcode74
src/spaces.h:74: #define ASSERT_MAP_ALIGNED(address)                \
nit: maybe align \ to \ above?

[Søren Thygesen Gjesse, 2009-12-17 08:39:37]

Anton, thank you for the thorough review of this CL.

http://codereview.chromium.org/504026/diff/6001/6005
File src/objects.h (right):

http://codereview.chromium.org/504026/diff/6001/6005#newcode919
src/objects.h:919: #ifndef V8_HOST_ARCH_64_BIT
On 2009/12/17 06:49:46, antonm wrote:
> just an idea: maybe instead of STATIC_CHECK declare kMapPageIndexBits as 32 -
> (kForwardingOffsetBits + kMapPageOffsetBits)?
> 
> Up to you.

Good point, now kMapPageIndexBits is only set to a fixed value on 64-bit and its
maximum on 32-bit.

http://codereview.chromium.org/504026/diff/6001/6005#newcode925
src/objects.h:925: // Bit masks covering the different parts the encodeding.
On 2009/12/17 06:49:46, antonm wrote:
> nit: encod<de>ing

Done.

http://codereview.chromium.org/504026/diff/6001/6004
File src/spaces.h (left):

http://codereview.chromium.org/504026/diff/6001/6004#oldcode1744
src/spaces.h:1744: static const int kMaxMapPageIndex = (1 <<
MapWord::kMapPageIndexBits) - 1;
On 2009/12/17 06:49:46, antonm wrote:
> On 2009/12/16 22:19:41, Søren Gjesse wrote:
> > On 2009/12/16 16:26:30, antonm wrote:
> > > BTW, do I miss something or this constant should be increased to be
> something
> > > like (1 << (MapWord::kMapPageIndexBits + kMapAlignmentBits -
> > > kObjectAlignmentBits))?
> > > 
> > > If yes, could you merge them to get rid of duplication?
> > 
> > This is correct, as we need one entry for each possible page of map space.
> What
> > is wrong however is the calculation of kMaxMapSpaceSize which should be
> > 
> >   static const int kMaxMapSpaceSize =
> >       (1 << (MapWord::kMapPageIndexBits)) * Page::kPageSize;
> > 
> > Thanks for spotting this inconsistency.
> 
> You're right. 
> 
> The fact that Chromium didn't crash for dashboard makes me somewhat uneasy as
> map space might have grown to the size when pointers are not encodable (but
> probably this point wasn't reached).

It sure means that no test fails, which is not so good. I tried running the
regress-524.js allocating even more with this bug, and it allocated > 10000
pages in map space before running out of memory. With the correct max-size of
map space OOM happens with 8K pages in map space.

http://codereview.chromium.org/504026/diff/6001/6004
File src/spaces.h (right):

http://codereview.chromium.org/504026/diff/6001/6004#newcode74
src/spaces.h:74: #define ASSERT_MAP_ALIGNED(address)                \
On 2009/12/17 06:49:46, antonm wrote:
> nit: maybe align \ to \ above?

Moved all \'s to position 79.