Merging scavenge into sweeping phase of mark-sweep(-compact) collector.

Merging scavenge into sweeping phase of mark-sweep(-compact) collector. 

Should reduce full GC pauses. 

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

[Mads Ager (chromium), 2010-03-25 09:59:23]

Kevin had a version where these were combined. Kevin, could you review this one?

[Kevin Millikin (Chromium), 2010-03-25 14:48:32]

First round of comments, mostly just style comments.

http://codereview.chromium.org/1217011/diff/8001/9001
File src/heap.cc (right):

http://codereview.chromium.org/1217011/diff/8001/9001#newcode864
src/heap.cc:864: return 0;
Style issue: we write NULL instead of 0 for pointers.

http://codereview.chromium.org/1217011/diff/8001/9001#newcode884
src/heap.cc:884: String *target = updater_func(p);
Style issue: the star goes next to "String" not "target".

http://codereview.chromium.org/1217011/diff/8001/9001#newcode886
src/heap.cc:886: if (target == 0) {
NULL not 0.  Style issue: for single line bodies you are welcome to put them on
the same line as the if with no braces.  I think that's more readable here:

if (target == NULL) continue;

http://codereview.chromium.org/1217011/diff/8001/9004
File src/mark-compact.cc (right):

http://codereview.chromium.org/1217011/diff/8001/9004#newcode52
src/mark-compact.cc:52: static inline void MoveBlock(Address dst, Address src,
size_t n) {
You might consider making this a static function on Heap (and move it into
heap-inl.h) to go with Heap::CopyBlock.

"n" is not a good name for the argument.  Something like "size_in_bytes" or
something that indicates the dimensions is better.

http://codereview.chromium.org/1217011/diff/8001/9004#newcode54
src/mark-compact.cc:54: ASSERT(IsAligned<size_t>(n, kPointerSize));
Lift this ASSERT outside the if?

http://codereview.chromium.org/1217011/diff/8001/9004#newcode57
src/mark-compact.cc:57: Address* p = reinterpret_cast<Address*>(dst);
This is not an Address*.  Object**?

http://codereview.chromium.org/1217011/diff/8001/9004#newcode61
src/mark-compact.cc:61: while (q != end)
The style rule is that if the body is one line and on the same line as the
while, you can leave off the braces.  If the body begins on the next line, you
need the braces.

http://codereview.chromium.org/1217011/diff/8001/9004#newcode63
src/mark-compact.cc:63: } else if (dst >= (src + n)) {
Isn't this case the same as the previous one?  It seems like the two cases
should just be combined.

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1101
src/mark-compact.cc:1101: // First pass migrates all alive objects from one
semispace to another or
"The first pass"..."promotes".  "Forwading addresses a" ==> "Forwarding
addresses are".

Written directly into what?

"The second pass".

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1107
src/mark-compact.cc:1107: // map space sweeping is done after new space
sweeping.
This comment might be clarified to mention that we sweep the remembered sets in
the map space.

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1118
src/mark-compact.cc:1118: // It does not expect to  encounter pointers to dead
objects.
Extra space between "to" and "encounter".

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1134
src/mark-compact.cc:1134: reinterpret_cast<Code*>(target)->instruction_start());
If you use reinterpret_cast instead of Code::cast because the object is marked,
say so in a short comment so the reader doesn't wonder.

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1172
src/mark-compact.cc:1172: if (new_addr == 0) return;
NULL.

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1249
src/mark-compact.cc:1249: Object *target = space->AllocateRaw(size);
Star goes next to Object not target.

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1257
src/mark-compact.cc:1257: Memory::Address_at(current) = 0;
NULL.

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1261
src/mark-compact.cc:1261: // Second Pass: find pointers to new space and update
them.
"Pass" ==> "pass".

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1264
src/mark-compact.cc:1264: // Update pointers in new space.
Maybe "new space" ==> "to space"....

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1265
src/mark-compact.cc:1265: HeapObject *object;
Star goes next to HeapObject.

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1287
src/mark-compact.cc:1287: cell != NULL; cell = cell_iterator.next()) {
I think you should be each part of the for loop header on their own line, unless
they fit on a single line.

[Vyacheslav Egorov (Chromium), 2010-03-29 16:40:51]

Fixed all style issues.

http://codereview.chromium.org/1217011/diff/8001/9004
File src/mark-compact.cc (right):

http://codereview.chromium.org/1217011/diff/8001/9004#newcode1134
src/mark-compact.cc:1134: reinterpret_cast<Code*>(target)->instruction_start());
On 2010/03/25 14:48:33, Kevin Millikin wrote:
> If you use reinterpret_cast instead of Code::cast because the object is
marked,
> say so in a short comment so the reader doesn't wonder.

Replaced reinterpret_cast<Code*> with Code::cast(). Only maps are still marked
at this point, everything else was swept thus it is safe to dereference Code's
map-pointer.

[Kevin Millikin (Chromium), 2010-03-30 06:33:36]

LGTM.  You'll need to add your changes to heap-inl.h to this changelist before
committing.

[Vyacheslav Egorov (Chromium), 2010-03-30 11:19:15]

Kevin, please take a look at heap-inl.h. 

On 2010/03/30 06:33:36, Kevin Millikin wrote:
> LGTM.  You'll need to add your changes to heap-inl.h to this changelist before
> committing.

[Kevin Millikin (Chromium), 2010-03-30 11:28:14]

LGTM.