Refactoring incremental marking

Refactoring incremental marking


BUG=

Committed: https://code.google.com/p/v8/source/detail?r=12918

[Michael Starzinger, 2012-11-08 11:10:38]

This is already looking good, just a few comments.

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
src/incremental-marking.cc:207:
IncrementalMarkingMarkingVisitor::VisitNativeContext(map, context);
This method is actually in the StaticMarkingVisitor, so I would just remove the
class prefix in this tail call to VisitNativeContext.

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
src/incremental-marking.cc:632: void IncrementalMarking::MarkObject(Map* map,
HeapObject* obj) {
This should actually be called "ProcessObject" or "VisitObject", because the
marking already happened when doing white->grey. Now that we do grey->black we
are actually visiting the object and iterating it.

Also I checked the disassembly and this is currently not inlined into the two
processing loops. It has to be marked as INLINE(...) in the header.

Also even in the inlined version, SizeFromMap() is called twice in the stepping
case. We could pass in the size as an argument. But at this point I am inclined
to actually manually inline it into the two processing loops. It shouldn't be
too much code duplication in this case. WDYT?

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
src/incremental-marking.cc:648: void IncrementalMarking::ProcessMarking(intptr_t
bytes_to_process) {
Let's rename that to ProcessMarkingDeque, that's more descriptive.

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
src/incremental-marking.cc:665: void IncrementalMarking::ProcessMarking() {
Likewise.

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
File src/incremental-marking.h (right):

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
src/incremental-marking.h:261: void ProcessMarking();
Add an empty newline in front of these three method declarations.

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/objects-visiting.h
File src/objects-visiting.h (right):

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/objects-visitin...
src/objects-visiting.h:409: static inline void
VisitNativeContextIncremental(Map* map,
Remove the declaration of the VisitNativeContextIncremental declaration, there
is no such method in this base class.

[payer, 2012-11-08 12:12:10]

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
src/incremental-marking.cc:207:
IncrementalMarkingMarkingVisitor::VisitNativeContext(map, context);
On 2012/11/08 11:10:39, Michael Starzinger wrote:
> This method is actually in the StaticMarkingVisitor, so I would just remove
the
> class prefix in this tail call to VisitNativeContext.

Done.

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
src/incremental-marking.cc:632: void IncrementalMarking::MarkObject(Map* map,
HeapObject* obj) {
I added a size parameter to the VisitObject method. That keeps the flexibility.

Done.

On 2012/11/08 11:10:39, Michael Starzinger wrote:
> This should actually be called "ProcessObject" or "VisitObject", because the
> marking already happened when doing white->grey. Now that we do grey->black we
> are actually visiting the object and iterating it.
> 
> Also I checked the disassembly and this is currently not inlined into the two
> processing loops. It has to be marked as INLINE(...) in the header.
> 
> Also even in the inlined version, SizeFromMap() is called twice in the
stepping
> case. We could pass in the size as an argument. But at this point I am
inclined
> to actually manually inline it into the two processing loops. It shouldn't be
> too much code duplication in this case. WDYT?

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
src/incremental-marking.cc:648: void IncrementalMarking::ProcessMarking(intptr_t
bytes_to_process) {
On 2012/11/08 11:10:39, Michael Starzinger wrote:
> Let's rename that to ProcessMarkingDeque, that's more descriptive.

Done.

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
src/incremental-marking.cc:665: void IncrementalMarking::ProcessMarking() {
On 2012/11/08 11:10:39, Michael Starzinger wrote:
> Likewise.

Done.

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
File src/incremental-marking.h (right):

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/incremental-mar...
src/incremental-marking.h:261: void ProcessMarking();
On 2012/11/08 11:10:39, Michael Starzinger wrote:
> Add an empty newline in front of these three method declarations.

Done.

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/objects-visiting.h
File src/objects-visiting.h (right):

https://chromiumcodereview.appspot.com/11368137/diff/2001/src/objects-visitin...
src/objects-visiting.h:409: static inline void
VisitNativeContextIncremental(Map* map,
On 2012/11/08 11:10:39, Michael Starzinger wrote:
> Remove the declaration of the VisitNativeContextIncremental declaration, there
> is no such method in this base class.

Done.

[Michael Starzinger, 2012-11-08 12:16:12]

LGTM (with a micro-nit).

https://codereview.chromium.org/11368137/diff/6001/src/incremental-marking.cc
File src/incremental-marking.cc (right):

https://codereview.chromium.org/11368137/diff/6001/src/incremental-marking.cc...
src/incremental-marking.cc:678: 
Add a second newline between these two methods.

[payer, 2012-11-08 12:22:30]

https://codereview.chromium.org/11368137/diff/6001/src/incremental-marking.cc
File src/incremental-marking.cc (right):

https://codereview.chromium.org/11368137/diff/6001/src/incremental-marking.cc...
src/incremental-marking.cc:678: 
On 2012/11/08 12:16:12, Michael Starzinger wrote:
> Add a second newline between these two methods.

Done.