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. http://codereview.chromium.org/504026 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
