[heap] Don't unmap new space pages while sweeping is active

[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=chromium:628984
R=hpayer@chromium.org

Committed: https://crrev.com/982b399423e6bd941cabb2b825031cd8d5eb4980
Cr-Commit-Position: refs/heads/master@{#38710}

[Michael Lippautz, 2016-08-17 14:03:50]

Description was changed from

==========
[heap] Fix sweeping/unmapping

BUG=
==========

to

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
It could not check whether a concurrent task was still trying to lock the page
for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
get's emptied upon the next call to the unmapper.

BUG=
==========

[Michael Lippautz, 2016-08-17 14:17:00]

Patchset #3 (id:40001) has been deleted

[Michael Lippautz, 2016-08-17 14:17:10]

The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 14:17:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Lippautz, 2016-08-17 14:18:23]

Description was changed from

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
It could not check whether a concurrent task was still trying to lock the page
for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
get's emptied upon the next call to the unmapper.

BUG=
==========

to

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=
==========

[Michael Lippautz, 2016-08-17 14:18:35]

Description was changed from

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=
==========

to

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=chromium:628984
==========

[Michael Lippautz, 2016-08-17 14:28:01]

Description was changed from

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=chromium:628984
==========

to

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=chromium:628984
==========

[Michael Lippautz, 2016-08-17 14:28:01]

mlippautz@chromium.org changed reviewers:
+ hpayer@chromium.org

[Michael Lippautz, 2016-08-17 14:28:27]

Description was changed from

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=chromium:628984
==========

to

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=chromium:628984
R=hpayer@chromium.org
==========

[commit-bot: I haz the power, 2016-08-17 14:49:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 14:49:20]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2016-08-18 09:38:43]

https://codereview.chromium.org/2250423002/diff/60001/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/2250423002/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:387: void
MemoryAllocator::Unmapper::ReconsiderDelayedChunks() {
I would rename ReconsiderDelayedChunks to AddDelayedChunks which is only called
when sweeping completed.
This method would then unconditionally add the delayed items to the regular
list.
WDYT?

https://codereview.chromium.org/2250423002/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:402: !(mc != nullptr && mc->sweeper().IsSweepingCompleted());
Simplify the second part of the boolean condition.
(mc == nullptr || !mc->sweeper().IsSweepingCompleted())

https://codereview.chromium.org/2250423002/diff/60001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/2250423002/diff/60001/src/heap/spaces.h#newco...
src/heap/spaces.h:1232: // of dependencies. See
MemoryAllocator::CanFreeMemoryChunk.
Just spell out the "dependencies" here.

[Michael Lippautz, 2016-08-18 09:59:40]

https://codereview.chromium.org/2250423002/diff/60001/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/2250423002/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:387: void
MemoryAllocator::Unmapper::ReconsiderDelayedChunks() {
On 2016/08/18 09:38:42, Hannes Payer wrote:
> I would rename ReconsiderDelayedChunks to AddDelayedChunks which is only
called
> when sweeping completed.
> This method would then unconditionally add the delayed items to the regular
> list.
> WDYT?

As discussed offline, I'd rather keep it that way. The # of affected pages
should be very small and this way we have a nice abstraction.

https://codereview.chromium.org/2250423002/diff/60001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:402: !(mc != nullptr && mc->sweeper().IsSweepingCompleted());
On 2016/08/18 09:38:42, Hannes Payer wrote:
> Simplify the second part of the boolean condition.
> (mc == nullptr || !mc->sweeper().IsSweepingCompleted())

Done.

https://codereview.chromium.org/2250423002/diff/60001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/2250423002/diff/60001/src/heap/spaces.h#newco...
src/heap/spaces.h:1232: // of dependencies. See
MemoryAllocator::CanFreeMemoryChunk.
On 2016/08/18 09:38:42, Hannes Payer wrote:
> Just spell out the "dependencies" here.

Done.

[Michael Lippautz, 2016-08-18 09:59:46]

The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 09:59:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 10:31:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 10:31:36]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2016-08-18 10:34:22]

lgtm

[Michael Lippautz, 2016-08-18 10:54:21]

The CQ bit was checked by mlippautz@chromium.org

[commit-bot: I haz the power, 2016-08-18 10:54:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 10:56:07]

Description was changed from

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=chromium:628984
R=hpayer@chromium.org
==========

to

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=chromium:628984
R=hpayer@chromium.org
==========

[commit-bot: I haz the power, 2016-08-18 10:56:08]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-08-18 10:56:25]

Description was changed from

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=chromium:628984
R=hpayer@chromium.org
==========

to

==========
[heap] Don't unmap new space pages while sweeping is active

- The barrier for scavenge only checked for whether new space pages were swept.
  This is not enough as a concurrent task could still hang right before trying
to
  lock the page for sweeping. Remove the barrier completely.
- Avoid unmapping of new space pages while sweeping using a delayed list that
  gets emptied upon the next call to the unmapper.

BUG=chromium:628984
R=hpayer@chromium.org

Committed: https://crrev.com/982b399423e6bd941cabb2b825031cd8d5eb4980
Cr-Commit-Position: refs/heads/master@{#38710}
==========

[commit-bot: I haz the power, 2016-08-18 10:56:26]

Patchset 4 (id:??) landed as
https://crrev.com/982b399423e6bd941cabb2b825031cd8d5eb4980
Cr-Commit-Position: refs/heads/master@{#38710}

[Michael Lippautz, 2016-08-18 11:29:46]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/2244233007/ by mlippautz@chromium.org.

The reason for reverting is: The barrier in newspace is still needed..