Parallel and concurrent sweeping.

Parallel and concurrent sweeping.

Sweep old pointer space and old data space concurrently to the main mutator thread and in parallel.

BUG=

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

[Michael Starzinger, 2013-01-24 17:15:53]

This is going into the right direction. A first round of high level comments.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/flag-definitio...
File src/flag-definitions.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/flag-definitio...
src/flag-definitions.h:416: DEFINE_bool(concurrent_sweeping, false, "enable
concurrent sweeping")
As discussed offline, for now it makes sense to have both flags until we
stabilize the implementation. Later we should unify them into one.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/heap.h
File src/heap.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/heap.h#newcode...
src/heap.h:1091: bool IsConcurrentSweepingPending();
We should try to move most of that functionality into MarkCompact and minimize
the entry points in Heap. IMHO parallel sweeping should be considered a
subsystem of MarkCompact.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/heap.h#newcode...
src/heap.h:1578: void SweepInParallel(PagedSpace* paged_space,
This dispatcher should not be necessary.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/heap.h#newcode...
src/heap.h:1588: bool AdvanceSweepers(int step_size) {
This is still being called from the idle notification. For both modes it should
be a NOP.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/isolate.cc#new...
src/isolate.cc:1733: if (FLAG_concurrent_sweeping || FLAG_parallel_sweeping) {
Move this above the stopping of the optimizing compiler so that de-init order is
the reverse of init order.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/isolate.cc#new...
src/isolate.cc:2106: if (FLAG_sweeper_threads < 1) {
Just set it to one if it's smaller than one.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/spaces.cc
File src/spaces.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/spaces.cc#newc...
src/spaces.cc:2464: if (heap()->IsConcurrentSweepingActivated()) {
The SlowAllocateRaw() function is getting too complicated. Would it be possible
to move all the sweeper-related code into a separate EnsureSweeperProgress()
function and basically keep the original implementation of SlowAllocateRaw()
almost untouched?

[Michael Starzinger, 2013-01-24 17:28:57]

One more comment.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/mark-compact.c...
src/mark-compact.cc:385: heap_->WaitUntilParallelSweepingCompleted();
This should be moved into MarkCompact::Prepare, before the lazy swept pages are
abandoned. As discussed offline, let's for now stick with the "waiting on the
threads" and maybe later try to "abort sweeping threads" if needed.

[Hannes Payer (out of office), 2013-01-25 10:46:49]

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/flag-definitio...
File src/flag-definitions.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/flag-definitio...
src/flag-definitions.h:416: DEFINE_bool(concurrent_sweeping, false, "enable
concurrent sweeping")
On 2013/01/24 17:15:54, Michael Starzinger wrote:
> As discussed offline, for now it makes sense to have both flags until we
> stabilize the implementation. Later we should unify them into one.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/heap.h
File src/heap.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/heap.h#newcode...
src/heap.h:1091: bool IsConcurrentSweepingPending();
On 2013/01/24 17:15:54, Michael Starzinger wrote:
> We should try to move most of that functionality into MarkCompact and minimize
> the entry points in Heap. IMHO parallel sweeping should be considered a
> subsystem of MarkCompact.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/heap.h#newcode...
src/heap.h:1578: void SweepInParallel(PagedSpace* paged_space,
On 2013/01/24 17:15:54, Michael Starzinger wrote:
> This dispatcher should not be necessary.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/heap.h#newcode...
src/heap.h:1588: bool AdvanceSweepers(int step_size) {
On 2013/01/24 17:15:54, Michael Starzinger wrote:
> This is still being called from the idle notification. For both modes it
should
> be a NOP.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/isolate.cc#new...
src/isolate.cc:1733: if (FLAG_concurrent_sweeping || FLAG_parallel_sweeping) {
On 2013/01/24 17:15:54, Michael Starzinger wrote:
> Move this above the stopping of the optimizing compiler so that de-init order
is
> the reverse of init order.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/isolate.cc#new...
src/isolate.cc:2106: if (FLAG_sweeper_threads < 1) {
On 2013/01/24 17:15:54, Michael Starzinger wrote:
> Just set it to one if it's smaller than one.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/mark-compact.c...
src/mark-compact.cc:385: heap_->WaitUntilParallelSweepingCompleted();
On 2013/01/24 17:28:57, Michael Starzinger wrote:
> This should be moved into MarkCompact::Prepare, before the lazy swept pages
are
> abandoned. As discussed offline, let's for now stick with the "waiting on the
> threads" and maybe later try to "abort sweeping threads" if needed.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/spaces.cc
File src/spaces.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/19002/src/spaces.cc#newc...
src/spaces.cc:2464: if (heap()->IsConcurrentSweepingActivated()) {
On 2013/01/24 17:15:54, Michael Starzinger wrote:
> The SlowAllocateRaw() function is getting too complicated. Would it be
possible
> to move all the sweeper-related code into a separate EnsureSweeperProgress()
> function and basically keep the original implementation of SlowAllocateRaw()
> almost untouched?

Done.

[Michael Starzinger, 2013-01-28 16:30:18]

Already looking good. This should be the last round of comments I think. Cool
stuff, I start to really like this! :)

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/flag-definitio...
File src/flag-definitions.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/flag-definitio...
src/flag-definitions.h:414: 
Remove this empty newline, or move this block up in front if the verify_heap
flag completely.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/heap.cc#newcod...
src/heap.cc:1283: }
What we actually wanna ensure here is that we made enough progress in sweeping
so that the Scavanger can in the worst case promote all objects from new-space.
So I think the right thing here is to call PagedSpace::EnsureSweeperProgress on
the old-data-space and old-pointer-space, no matter which mode we are in.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/heap.cc#newcod...
src/heap.cc:5382: 
I know it's not your change, but drop one of the two empty newlines.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/isolate.h
File src/isolate.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/isolate.h#newc...
src/isolate.h:45: #include "sweeper-thread.h"
There is no need to include the full header, just forward declare the
SweeperThread class below.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:835: if (AreSweeperThreadsActivated() &&
FLAG_concurrent_sweeping) {
Needs a comment that for now we wait, but it might make sense to abandon work in
the future.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:2771: FreeList* free_list,
No need to pass the free-list, will not be called from a sweeper thread.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3073: SweepConservatively(space, space->free_list(), p);
This call-path leads to a mismatch in the accounting stats, because the space's
free-list is used directly, but the non-space specific freeing function is used.
This proves that having the PagedSpace::free_list() accessor is dangerous. See
comments about templatizing SweepConservatively below on how to fix this.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3429: intptr_t MarkCompactCollector::Free(PagedSpace* space,
Make this a static helper ... this should never ever be used by someone else
besides the sweeper below.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3449: intptr_t
MarkCompactCollector::SweepConservatively(PagedSpace* space,
There are basically two different sweepers at work here, one for parallel and
one for non-parallel sweeping. We should templatize this function accordingly.
Something like ...

enum SweepingParallelism {
  SWEEP_ON_MAIN_THREAD,
  SWEEP_IN_PARALLEL
}

For parallel sweeping the free_list parameter will contain the target free-list,
whereas for on the main-thread it will always be NULL (and we should actually
assert that).

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3519: freed_bytes += Free(space, free_list, free_start,
This looks like it will tank non-parallel sweeping. The templatization will
resolve this problem.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3543: void
MarkCompactCollector::PrepareParallelSweeping(PagedSpace* space) {
Please merge all of this into SweepSpace() and introduce a fourth SweeperType
(i.e. PARALLEL_CONSERVATIVE) that indicates that a "1" should be put into some
pages that are eligible for parallel sweeping.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3598: continue;
In concurrent mode we have a race here, because the main thread is evacuating
while the sweeper threads are sweeping. So you might end up with a case where
the page is no longer and evacuation candidate but was swept already.

The wayt to solve this is to get rid of this check and instead solely rely on
the TryParallelSweeping to pick the pages that should be swept in parallel. This
will require the sweeper thread to basically steal a "1" out of the page header
instead of stealing the "0".

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3607: if (sweeper_type == CONSERVATIVE || sweeper_type ==
LAZY_CONSERVATIVE) {
This logic is no longer needed, we always sweep conservatively.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3627: FLAG_concurrent_sweeping));
These assertions are bogus, they will always be false, because the two flags are
exclusive. Boolean algebra at its best.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3748: PrepareParallelSweeping(heap()->old_pointer_space());
See the comment in PrepareParallelSweeping about this, better use SweepSpace
with a new SweeperType here.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3750: StartSweeperThreads(how_to_sweep);
Move the StartSweeperThreads to below EvacuateNewSpaceAndCandidates for now to
prevent races. AFAICT it should be possible to start the threads here, but let's
play it safe for now and move it down.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.h
File src/mark-compact.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.h...
src/mark-compact.h:503: enum SweeperType {
Move this back into MarkCompactCollector, no longer needed outside.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.h...
src/mark-compact.h:676: SweeperType sweeper_type,
As discussed offline: Parallel sweeping will always do conservative sweeping. We
need precise sweeping only when the heap needs to be made iterable, and that is
inherently a blocking operation where any lazyness or parallelism with the
mutator makes no sense. So we should just remove the SweeperType from the API.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h
File src/spaces.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newco...
src/spaces.h:1438: Mutex* mutex() { return mutex_; }
Empty newline after the accessor.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newco...
src/spaces.h:1668: FreeList* free_list() { return &free_list_; }
Remove this accessor, it is dangerous. Since the SweeperThread is the last
caller, better make the SweeperThread a friend of PagedSpace.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newco...
src/spaces.h:1774: void AddToAccountingStats(intptr_t bytes) {
Same comment as for free_list() apply to this function.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newco...
src/spaces.h:1827: 
Drop the second empty new-lined. Needs a short comment of what the function
does.

[Hannes Payer (out of office), 2013-01-30 10:11:27]

There is currently a race with concurrent sweeping and modification of the pages
list of a space, as discussed offline. I will fix that in a separate CL.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/flag-definitio...
File src/flag-definitions.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/flag-definitio...
src/flag-definitions.h:414: 
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> Remove this empty newline, or move this block up in front if the verify_heap
> flag completely.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/heap.cc#newcod...
src/heap.cc:1283: }
Right, but this changes the current non-parallel non-concurrent behavior.

On 2013/01/28 16:30:18, Michael Starzinger wrote:
> What we actually wanna ensure here is that we made enough progress in sweeping
> so that the Scavanger can in the worst case promote all objects from
new-space.
> So I think the right thing here is to call PagedSpace::EnsureSweeperProgress
on
> the old-data-space and old-pointer-space, no matter which mode we are in.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/heap.cc#newcod...
src/heap.cc:5382: 
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> I know it's not your change, but drop one of the two empty newlines.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/isolate.h
File src/isolate.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/isolate.h#newc...
src/isolate.h:45: #include "sweeper-thread.h"
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> There is no need to include the full header, just forward declare the
> SweeperThread class below.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:835: if (AreSweeperThreadsActivated() &&
FLAG_concurrent_sweeping) {
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> Needs a comment that for now we wait, but it might make sense to abandon work
in
> the future.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:2771: FreeList* free_list,
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> No need to pass the free-list, will not be called from a sweeper thread.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3073: SweepConservatively(space, space->free_list(), p);
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> This call-path leads to a mismatch in the accounting stats, because the
space's
> free-list is used directly, but the non-space specific freeing function is
used.
> This proves that having the PagedSpace::free_list() accessor is dangerous. See
> comments about templatizing SweepConservatively below on how to fix this.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3429: intptr_t MarkCompactCollector::Free(PagedSpace* space,
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> Make this a static helper ... this should never ever be used by someone else
> besides the sweeper below.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3449: intptr_t
MarkCompactCollector::SweepConservatively(PagedSpace* space,
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> There are basically two different sweepers at work here, one for parallel and
> one for non-parallel sweeping. We should templatize this function accordingly.
> Something like ...
> 
> enum SweepingParallelism {
>   SWEEP_ON_MAIN_THREAD,
>   SWEEP_IN_PARALLEL
> }
> 
> For parallel sweeping the free_list parameter will contain the target
free-list,
> whereas for on the main-thread it will always be NULL (and we should actually
> assert that).

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3519: freed_bytes += Free(space, free_list, free_start,
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> This looks like it will tank non-parallel sweeping. The templatization will
> resolve this problem.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3543: void
MarkCompactCollector::PrepareParallelSweeping(PagedSpace* space) {
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> Please merge all of this into SweepSpace() and introduce a fourth SweeperType
> (i.e. PARALLEL_CONSERVATIVE) that indicates that a "1" should be put into some
> pages that are eligible for parallel sweeping.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3598: continue;
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> In concurrent mode we have a race here, because the main thread is evacuating
> while the sweeper threads are sweeping. So you might end up with a case where
> the page is no longer and evacuation candidate but was swept already.
> 
> The wayt to solve this is to get rid of this check and instead solely rely on
> the TryParallelSweeping to pick the pages that should be swept in parallel.
This
> will require the sweeper thread to basically steal a "1" out of the page
header
> instead of stealing the "0".

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3607: if (sweeper_type == CONSERVATIVE || sweeper_type ==
LAZY_CONSERVATIVE) {
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> This logic is no longer needed, we always sweep conservatively.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3627: FLAG_concurrent_sweeping));
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> These assertions are bogus, they will always be false, because the two flags
are
> exclusive. Boolean algebra at its best.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3748: PrepareParallelSweeping(heap()->old_pointer_space());
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> See the comment in PrepareParallelSweeping about this, better use SweepSpace
> with a new SweeperType here.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.c...
src/mark-compact.cc:3750: StartSweeperThreads(how_to_sweep);
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> Move the StartSweeperThreads to below EvacuateNewSpaceAndCandidates for now to
> prevent races. AFAICT it should be possible to start the threads here, but
let's
> play it safe for now and move it down.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.h
File src/mark-compact.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.h...
src/mark-compact.h:503: enum SweeperType {
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> Move this back into MarkCompactCollector, no longer needed outside.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.h...
src/mark-compact.h:676: SweeperType sweeper_type,
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> As discussed offline: Parallel sweeping will always do conservative sweeping.
We
> need precise sweeping only when the heap needs to be made iterable, and that
is
> inherently a blocking operation where any lazyness or parallelism with the
> mutator makes no sense. So we should just remove the SweeperType from the API.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h
File src/spaces.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newco...
src/spaces.h:1438: Mutex* mutex() { return mutex_; }
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> Empty newline after the accessor.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newco...
src/spaces.h:1668: FreeList* free_list() { return &free_list_; }
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> Remove this accessor, it is dangerous. Since the SweeperThread is the last
> caller, better make the SweeperThread a friend of PagedSpace.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newco...
src/spaces.h:1774: void AddToAccountingStats(intptr_t bytes) {
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> Same comment as for free_list() apply to this function.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newco...
src/spaces.h:1827: 
On 2013/01/28 16:30:18, Michael Starzinger wrote:
> Drop the second empty new-lined. Needs a short comment of what the function
> does.

Done.

[Michael Starzinger, 2013-01-30 11:01:19]

LGTM if final comments are addresed. I like this!

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/isolate.cc#new...
src/isolate.cc:1737: for (int i = 0; i < FLAG_sweeper_threads; i++) {
Can we merge the two for-loops, I think that's easier to read.

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.c...
src/mark-compact.cc:3478: if (mode == MarkCompactCollector::SWEEP_SEQUENTIALLY)
{
You could factor out this logic and move it into the static Free() helper,
templatize the helper and hope that GCC inlines it. Would make the source
cleaner and produce the same code. I would prefer that over the current
implementation, but I'll leave the decision whether to do that or not up to you.

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.c...
src/mark-compact.cc:3652: PrintF("Prepare conservatively parallel sweeping 0x%"
V8PRIxPTR ".\n",
Let's rephrase this log line to "Sweeping 0x??? conservatively in parallel.", I
think that's easier to understand.

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.c...
src/mark-compact.cc:3706: 
I think you have white-space on that empty line. Should trip a presubmit
failure.

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.c...
src/mark-compact.cc:3715: // The starting of the sweeper threads should be after
SweepSpace
Let's turn this comment into a TODO().

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.h
File src/mark-compact.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.h...
src/mark-compact.h:859: void PrepareParallelSweeping(PagedSpace* space);
The implementation of this method is gone, let's also drop the declaration.

[Hannes Payer (out of office), 2013-01-30 12:07:32]

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/isolate.cc#new...
src/isolate.cc:1737: for (int i = 0; i < FLAG_sweeper_threads; i++) {
On 2013/01/30 11:01:19, Michael Starzinger wrote:
> Can we merge the two for-loops, I think that's easier to read.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.c...
src/mark-compact.cc:3478: if (mode == MarkCompactCollector::SWEEP_SEQUENTIALLY)
{
I templatized Free, the code looks cleaner.

On 2013/01/30 11:01:19, Michael Starzinger wrote:
> You could factor out this logic and move it into the static Free() helper,
> templatize the helper and hope that GCC inlines it. Would make the source
> cleaner and produce the same code. I would prefer that over the current
> implementation, but I'll leave the decision whether to do that or not up to
you.

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.c...
src/mark-compact.cc:3652: PrintF("Prepare conservatively parallel sweeping 0x%"
V8PRIxPTR ".\n",
On 2013/01/30 11:01:19, Michael Starzinger wrote:
> Let's rephrase this log line to "Sweeping 0x??? conservatively in parallel.",
I
> think that's easier to understand.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.c...
src/mark-compact.cc:3706: 
On 2013/01/30 11:01:19, Michael Starzinger wrote:
> I think you have white-space on that empty line. Should trip a presubmit
> failure.

Done.

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.c...
src/mark-compact.cc:3715: // The starting of the sweeper threads should be after
SweepSpace
On 2013/01/30 11:01:19, Michael Starzinger wrote:
> Let's turn this comment into a TODO().

Done.

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.h
File src/mark-compact.h (right):

https://chromiumcodereview.appspot.com/11782028/diff/34001/src/mark-compact.h...
src/mark-compact.h:859: void PrepareParallelSweeping(PagedSpace* space);
On 2013/01/30 11:01:19, Michael Starzinger wrote:
> The implementation of this method is gone, let's also drop the declaration.

Done.