Release evacuation candidates after parallel sweeper threads completed.

Release evacuation candidates after parallel sweeper threads completed.

BUG=

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

[Michael Starzinger, 2013-02-20 18:20:07]

This looks much better than the original solution. First round of comments.

https://codereview.chromium.org/12082101/diff/5004/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/12082101/diff/5004/src/mark-compact.cc#newcod...
src/mark-compact.cc:3868: WaitUntilSweepingCompleted();
The ordering of the calls is off, in the "parallel sweeping" mode this would
cause the evacuation candidates to be release before they are actually
evacuated.

I would suggest to add a FinalizeSweeping() method that is called after
WaitUntilSweepingComplete() returns on all other call-sites ...

https://codereview.chromium.org/12082101/diff/5004/src/mark-compact.cc#newcod...
src/mark-compact.cc:3887: if (!AreSweeperThreadsActivated()) {
... but called down here at this call-site. Also I would change the if-cascade
to use the same predicates:

if (how_to_sweep == PARALLEL_CONSERVATIVE) {
  if (FLAG_parallel_sweeping && !FLAG_concurrent_sweeping) {
    FinalizeSweeping();
  }
}

[Hannes Payer (out of office), 2013-02-21 12:05:17]

https://codereview.chromium.org/12082101/diff/5004/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/12082101/diff/5004/src/mark-compact.cc#newcod...
src/mark-compact.cc:3868: WaitUntilSweepingCompleted();
On 2013/02/20 18:20:07, Michael Starzinger wrote:
> The ordering of the calls is off, in the "parallel sweeping" mode this would
> cause the evacuation candidates to be release before they are actually
> evacuated.
> 
> I would suggest to add a FinalizeSweeping() method that is called after
> WaitUntilSweepingComplete() returns on all other call-sites ...

Done.

https://codereview.chromium.org/12082101/diff/5004/src/mark-compact.cc#newcod...
src/mark-compact.cc:3887: if (!AreSweeperThreadsActivated()) {
The right condition is "if (!FLAG_concurrent_sweeping)" since it has to be
finalized in parallel and sequential mode.

On 2013/02/20 18:20:07, Michael Starzinger wrote:
> ... but called down here at this call-site. Also I would change the if-cascade
> to use the same predicates:
> 
> if (how_to_sweep == PARALLEL_CONSERVATIVE) {
>   if (FLAG_parallel_sweeping && !FLAG_concurrent_sweeping) {
>     FinalizeSweeping();
>   }
> }

[Michael Starzinger, 2013-02-21 12:22:09]

https://codereview.chromium.org/12082101/diff/12001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/12082101/diff/12001/src/mark-compact.cc#newco...
src/mark-compact.cc:538: bool MarkCompactCollector::WaitUntilSweepingCompleted()
{
Why do you return a boolean here? IMHO this makes the call sites too complex, I
would just rely on FinalizeSweeping() to be a no-op in those cases. I'll leave
that up to you.

https://codereview.chromium.org/12082101/diff/12001/src/mark-compact.cc#newco...
src/mark-compact.cc:3876: if (FLAG_parallel_sweeping &&
!FLAG_concurrent_sweeping) {
How about moving all of this magic into the how_to_sweep variable by introducing
a CONCURRENT_CONSERVATIVE on top of the existing PARALLEL_CONSERVATIVE?

https://codereview.chromium.org/12082101/diff/12001/src/mark-compact.cc#newco...
src/mark-compact.cc:3896: if (!FLAG_concurrent_sweeping) {
This is still not entirely correct, it _has_ to depend of the how_to_sweep
variable because that determines how the pages were swept.

[Hannes Payer (out of office), 2013-02-21 12:52:31]

https://codereview.chromium.org/12082101/diff/12001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/12082101/diff/12001/src/mark-compact.cc#newco...
src/mark-compact.cc:538: bool MarkCompactCollector::WaitUntilSweepingCompleted()
{
On 2013/02/21 12:22:09, Michael Starzinger wrote:
> Why do you return a boolean here? IMHO this makes the call sites too complex,
I
> would just rely on FinalizeSweeping() to be a no-op in those cases. I'll leave
> that up to you.

Done.

https://codereview.chromium.org/12082101/diff/12001/src/mark-compact.cc#newco...
src/mark-compact.cc:3876: if (FLAG_parallel_sweeping &&
!FLAG_concurrent_sweeping) {
On 2013/02/21 12:22:09, Michael Starzinger wrote:
> How about moving all of this magic into the how_to_sweep variable by
introducing
> a CONCURRENT_CONSERVATIVE on top of the existing PARALLEL_CONSERVATIVE?

Done.

https://codereview.chromium.org/12082101/diff/12001/src/mark-compact.cc#newco...
src/mark-compact.cc:3896: if (!FLAG_concurrent_sweeping) {
On 2013/02/21 12:22:09, Michael Starzinger wrote:
> This is still not entirely correct, it _has_ to depend of the how_to_sweep
> variable because that determines how the pages were swept.

Done.

[Michael Starzinger, 2013-02-21 12:54:59]

LGTM, with two final nits.

https://codereview.chromium.org/12082101/diff/14002/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/12082101/diff/14002/src/mark-compact.cc#newco...
src/mark-compact.cc:3875: // TODO(hpayer): fix race with concurrent sweeper
I think this TODO is done with this change. Let's drop it.

https://codereview.chromium.org/12082101/diff/14002/src/mark-compact.cc#newco...
src/mark-compact.cc:3883: 
Drop one of the two empty newlines.

[Hannes Payer (out of office), 2013-02-21 13:06:48]

https://codereview.chromium.org/12082101/diff/14002/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/12082101/diff/14002/src/mark-compact.cc#newco...
src/mark-compact.cc:3875: // TODO(hpayer): fix race with concurrent sweeper
There is still a race with VerifyEvacuation.

On 2013/02/21 12:54:59, Michael Starzinger wrote:
> I think this TODO is done with this change. Let's drop it.

https://codereview.chromium.org/12082101/diff/14002/src/mark-compact.cc#newco...
src/mark-compact.cc:3883: 
On 2013/02/21 12:54:59, Michael Starzinger wrote:
> Drop one of the two empty newlines.

Done.

[Hannes Payer (out of office), 2013-02-21 13:12:19]

Committed manually as r13708 (presubmit successful).