Get rid of distinction between below- and above-watermark in page allocation....

Get rid of distinction between below- and above-watermark in page allocation.
Now all allocation is in linear areas taken from the free list.

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

[Vyacheslav Egorov (Chromium), 2011-03-15 09:20:08]

first round of comments

http://codereview.chromium.org/6639024/diff/1/src/debug.cc
File src/debug.cc (right):

http://codereview.chromium.org/6639024/diff/1/src/debug.cc#newcode1877
src/debug.cc:1877: // scripts which are no longer referenced.
Comment is outdated add more about precise sweeping?

http://codereview.chromium.org/6639024/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6639024/diff/1/src/heap.cc#newcode4217
src/heap.cc:4217: typedef bool (*CheckStoreBufferFilter)(Object**addr);
space after **

http://codereview.chromium.org/6639024/diff/1/src/heap.cc#newcode4224
src/heap.cc:4224: mod < Map:: kPointerFieldsEndOffset;
remove space after ::

http://codereview.chromium.org/6639024/diff/1/src/heap.cc#newcode4372
src/heap.cc:4372: // This function iterates over all the pointers in a paged
space in the heap,
Special garbage section is utterly confusing.

Maybe draw page layout?

[ header | garbage section | whatever ]

http://codereview.chromium.org/6639024/diff/1/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6639024/diff/1/src/heap.h#newcode768
src/heap.h:768: static const int kSweepPreciselyMask = 2;
maybe better hide the concept of conservative and precise sweep and just call it
kEnsureIterableMask or something like that?

http://codereview.chromium.org/6639024/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/6639024/diff/1/src/mark-compact.cc#newcode1936
src/mark-compact.cc:1936: char kStartTable[kStartTableSize * kStartTableStride]
= {
Stride and Size are really mysterious names --- more comments would be great.

http://codereview.chromium.org/6639024/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/6639024/diff/1/src/objects-inl.h#newcode339
src/objects-inl.h:339: return instance_type == BYTE_ARRAY_TYPE || instance_type
== FILLER_TYPE;
maybe it is really time to make filler_array_type or whatever.

http://codereview.chromium.org/6639024/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6639024/diff/1/src/objects.h#newcode2597
src/objects.h:2597: inline int Size() { return RoundUp(length() + kHeaderSize,
kPointerSize); }
What's the difference from ByteArraySize()?

http://codereview.chromium.org/6639024/diff/1/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/6639024/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:2374: Heap::EnsureHeapIsIterable();
Sometimes we call Ensure, sometimes we don't.

This is too implicit. Can we move this decision into iterator constructor?

http://codereview.chromium.org/6639024/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6639024/diff/1/src/runtime.cc#newcode10527
src/runtime.cc:10527: Heap::EnsureHeapIsIterable();
This is very fragile. We definitely should encapsulate this decision in the
iterator itself.

http://codereview.chromium.org/6639024/diff/1/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode358
src/spaces.h:358: static const int kObjectStartAlignment = 32 * kPointerSize;
Why did not you just modify body offset?

http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode430
src/spaces.h:430: private:
does not make much sense to me to have a private section for a friend
declaration.

http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode843
src/spaces.h:843: // objects.
what if we allocate inside page in the hole? how starting from the top will
help?

or top is not top but something in the middle?

http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode858
src/spaces.h:858: inline HeapObject* Next() {
What if bytearray is really something important why do we skip it?

We are using bytearrays to keep relocation data for example.

http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode1046
src/spaces.h:1046: static const uintptr_t kFreeListZapValue = 0xfeed1eaf;
please move it to other zap values.

http://codereview.chromium.org/6639024/diff/1/test/mjsunit/debug-script.js
File test/mjsunit/debug-script.js (right):

http://codereview.chromium.org/6639024/diff/1/test/mjsunit/debug-script.js#ne...
test/mjsunit/debug-script.js:44: print(scripts[i].name);
please remove

[Erik Corry, 2011-03-17 13:39:17]

http://codereview.chromium.org/6639024/diff/1/src/debug.cc
File src/debug.cc (right):

http://codereview.chromium.org/6639024/diff/1/src/debug.cc#newcode1877
src/debug.cc:1877: // scripts which are no longer referenced.
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> Comment is outdated add more about precise sweeping?

Done.

http://codereview.chromium.org/6639024/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6639024/diff/1/src/heap.cc#newcode4217
src/heap.cc:4217: typedef bool (*CheckStoreBufferFilter)(Object**addr);
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> space after **

Done.

http://codereview.chromium.org/6639024/diff/1/src/heap.cc#newcode4224
src/heap.cc:4224: mod < Map:: kPointerFieldsEndOffset;
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> remove space after ::

Done.

http://codereview.chromium.org/6639024/diff/1/src/heap.cc#newcode4372
src/heap.cc:4372: // This function iterates over all the pointers in a paged
space in the heap,
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> Special garbage section is utterly confusing.
> 
> Maybe draw page layout?
> 
> [ header | garbage section | whatever ]

It's hard to draw in ASCII art, but I can do it for you on the white board.

http://codereview.chromium.org/6639024/diff/1/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6639024/diff/1/src/heap.h#newcode768
src/heap.h:768: static const int kSweepPreciselyMask = 2;
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> maybe better hide the concept of conservative and precise sweep and just call
it
> kEnsureIterableMask or something like that?

Done.

http://codereview.chromium.org/6639024/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/6639024/diff/1/src/mark-compact.cc#newcode1936
src/mark-compact.cc:1936: char kStartTable[kStartTableSize * kStartTableStride]
= {
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> Stride and Size are really mysterious names --- more comments would be great.

Done.

http://codereview.chromium.org/6639024/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/6639024/diff/1/src/objects-inl.h#newcode339
src/objects-inl.h:339: return instance_type == BYTE_ARRAY_TYPE || instance_type
== FILLER_TYPE;
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> maybe it is really time to make filler_array_type or whatever.

It seems most places we do care whether it is FILLER_TYPE (1 or 2 words) or
FREE_SPACE_TYPE (the replacement for BYTE_ARRAY_TYPE for larger areas).

http://codereview.chromium.org/6639024/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6639024/diff/1/src/objects.h#newcode2597
src/objects.h:2597: inline int Size() { return RoundUp(length() + kHeaderSize,
kPointerSize); }
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> What's the difference from ByteArraySize()?

None, but you have to have this one because it overrides a method in the
superclass.  I removed FreeSpaceSize()

http://codereview.chromium.org/6639024/diff/1/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/6639024/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:2374: Heap::EnsureHeapIsIterable();
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> Sometimes we call Ensure, sometimes we don't.
> 
> This is too implicit. Can we move this decision into iterator constructor?

Done.

http://codereview.chromium.org/6639024/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6639024/diff/1/src/runtime.cc#newcode10527
src/runtime.cc:10527: Heap::EnsureHeapIsIterable();
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> This is very fragile. We definitely should encapsulate this decision in the
> iterator itself.

Done.

http://codereview.chromium.org/6639024/diff/1/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode358
src/spaces.h:358: static const int kObjectStartAlignment = 32 * kPointerSize;
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> Why did not you just modify body offset?

kObjectStartOffset already existed.  I just changed it a bit.

http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode430
src/spaces.h:430: private:
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> does not make much sense to me to have a private section for a friend
> declaration.

Done.

http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode843
src/spaces.h:843: // objects.
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> what if we allocate inside page in the hole? how starting from the top will
> help?
> 
> or top is not top but something in the middle?

Comment clarified.

http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode858
src/spaces.h:858: inline HeapObject* Next() {
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> What if bytearray is really something important why do we skip it?
> 
> We are using bytearrays to keep relocation data for example.

I have created FreeSpace, which is used where the garbage collector used to use
ByteArray.

http://codereview.chromium.org/6639024/diff/1/src/spaces.h#newcode1046
src/spaces.h:1046: static const uintptr_t kFreeListZapValue = 0xfeed1eaf;
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> please move it to other zap values.

Done.

http://codereview.chromium.org/6639024/diff/1/test/mjsunit/debug-script.js
File test/mjsunit/debug-script.js (right):

http://codereview.chromium.org/6639024/diff/1/test/mjsunit/debug-script.js#ne...
test/mjsunit/debug-script.js:44: print(scripts[i].name);
On 2011/03/15 09:20:09, Vyacheslav Egorov wrote:
> please remove

Done.

[Vyacheslav Egorov (Chromium), 2011-03-17 16:11:00]

LGTM

http://codereview.chromium.org/6639024/diff/11003/src/spaces-inl.h
File src/spaces-inl.h (left):

http://codereview.chromium.org/6639024/diff/11003/src/spaces-inl.h#oldcode208
src/spaces-inl.h:208: IncrementalMarking::Step(size_in_bytes);
It seems that we are doing step less frequently. Why?

[Erik Corry, 2011-03-17 17:24:26]

http://codereview.chromium.org/6639024/diff/11003/src/spaces-inl.h
File src/spaces-inl.h (left):

http://codereview.chromium.org/6639024/diff/11003/src/spaces-inl.h#oldcode208
src/spaces-inl.h:208: IncrementalMarking::Step(size_in_bytes);
On 2011/03/17 16:11:00, Vyacheslav Egorov wrote:
> It seems that we are doing step less frequently. Why?

The idea is that we set the linear allocation limit lower so we will stop in
time.  This may need some tuning...