Use a lock in pages to synchronize sweeper threads to allow others to wait on concurrently swept pa…

Use a lock in pages to synchronize sweeper threads to allow others to wait on concurrently swept pages.

BUG=

Committed: https://crrev.com/05ec0ff08381b3780195069250f658da6040f131
Cr-Commit-Position: refs/heads/master@{#29804}

[Hannes Payer (out of office), 2015-07-22 09:38:13]

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

[Michael Lippautz, 2015-07-22 13:17:01]

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

https://codereview.chromium.org/1244353002/diff/100001/src/heap/spaces.h#newc...
src/heap/spaces.h:547: kPointerSize + kPointerSize;
Can we make this more verbose by adding a comment? E.g.:
 + kXXX  // ...
 + kPointerSize  // Field: mutex_

[Hannes Payer (out of office), 2015-07-22 13:29:25]

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

https://codereview.chromium.org/1244353002/diff/100001/src/heap/spaces.h#newc...
src/heap/spaces.h:547: kPointerSize + kPointerSize;
On 2015/07/22 13:17:01, Michael Lippautz wrote:
> Can we make this more verbose by adding a comment? E.g.:
>  + kXXX  // ...
>  + kPointerSize  // Field: mutex_

Done.

[Michael Lippautz, 2015-07-22 14:18:57]

lgtm

[Jarin, 2015-07-23 05:45:04]

lgtm. Oh boy, this is ugly. 

At this point, can't we just use a standard lock + condition variable mechanism
to protect and wait for the page's sweeping status? Would it be too slow? E.g.,
in Heap::CanMoveObjectStart we would have to lock inside the SweepingCompleted
check.

This funky combination of lock + atomic variable seems to be quite subtle. E.g.,
I do not like that WaitUntilSweepingCompleted only works if you know that
sweeping has started, otherwise you might be surprised. Perhaps you could add a
comment to WaitUntilSweepingCompleted that documents the proper usage?

[Hannes Payer (out of office), 2015-07-23 09:36:12]

The page flags prevent us from getting rid of the sweeping state per page. As
discussed offline, we could make the sweeping state non-atomic and move it into
the lock section. However, we would loose the fast-path for checking the state
for a bit less complexity. Therefore I decided to stick with the current
solution.

I added a comment about WaitUntilSweepingCompleted.

[Hannes Payer (out of office), 2015-07-23 09:36:16]

The CQ bit was checked by hpayer@chromium.org

[Hannes Payer (out of office), 2015-07-23 09:36:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlippautz@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1244353002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-07-23 09:36:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1244353002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1244353002/140001

[commit-bot: I haz the power, 2015-07-23 10:03:06]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-23 10:03:19]

Patchset 8 (id:??) landed as
https://crrev.com/05ec0ff08381b3780195069250f658da6040f131
Cr-Commit-Position: refs/heads/master@{#29804}