[oilpan]: Don't enter safepoint in SafePointAwareMutexLocker if sweeping.

[oilpan]: Don't enter safepoint in SafePointAwareMutexLocker if sweeping.

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177290

[wibling-chromium, 2014-07-01 09:24:26]

Change r177124 uncovered another bug where we try entering a safepoint while
sweeping. This should fix it.

[tkent, 2014-07-01 09:34:32]

lgtm.

Thanks!

[tkent, 2014-07-01 09:34:39]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-07-01 09:35:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wibling@chromium.org/367633002/1

[Erik Corry, 2014-07-01 09:36:55]

lgtm

[zerny-chromium, 2014-07-01 09:40:23]

lgtm

[haraken, 2014-07-01 10:35:10]

https://codereview.chromium.org/367633002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/367633002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:623: // due to this thread not being at a
safepoint and waiting on the lock.

Would you elaborate on why this doesn't cause dead lock?

(1) Thread 1 is sweeping.
(2) Thread 2 calls SafePointAwareMutexLocker. It acquires the mutex.
(3) Thread 2 triggers a GC.
(4) Thread 1 calls SafePointAwareMutexLocker. It tries to acquire the mutex.
(5) Dead lock?

Actually I don't fully understand why we cannot enter a safe point while
sweeping. It seems we can just enter a safe point, and if a GC is triggered we
can just clear the mark bits and restart the GC. I'd be happy if you could just
help me understand :)

[commit-bot: I haz the power, 2014-07-01 10:42:01]

Change committed as 177290

[wibling-chromium, 2014-07-01 11:10:59]

On 2014/07/01 10:35:10, haraken wrote:
>
https://codereview.chromium.org/367633002/diff/1/Source/platform/heap/ThreadS...
> File Source/platform/heap/ThreadState.h (right):
> 
>
https://codereview.chromium.org/367633002/diff/1/Source/platform/heap/ThreadS...
> Source/platform/heap/ThreadState.h:623: // due to this thread not being at a
> safepoint and waiting on the lock.
> 
> Would you elaborate on why this doesn't cause dead lock?
> 
> (1) Thread 1 is sweeping.
> (2) Thread 2 calls SafePointAwareMutexLocker. It acquires the mutex.
> (3) Thread 2 triggers a GC.
> (4) Thread 1 calls SafePointAwareMutexLocker. It tries to acquire the mutex.
> (5) Dead lock?

Thread 2 will time out in step 3 since Thread 1 will never enter a safepoint as
it was sweeping.
This will not dead-lock but only lead to a GC which does not do anything. It is
unfortunate, but 
at least it will continue after the timeout.

> Actually I don't fully understand why we cannot enter a safe point while
> sweeping. It seems we can just enter a safe point, and if a GC is triggered we
> can just clear the mark bits and restart the GC. I'd be happy if you could
just
> help me understand :)

There is a couple of reasons. We do both thread local weak processing as well as
sweeping in the "sweep" phase (ThreadState::performPendingSweep).
If we are doing weak processing and popping a weak callback we are using the
thread specific weak callback stack which is also used when tracing. We don't
currently support pushing weak callbacks from a GC thread while another thread
is in the middle of popping them.
If we are doing actual sweeping of a page and enters a safepoint in a destructor
we will get back and continue sweeping. However we will now have thrown away any
free list entries we have built up in the sweep so far. We will also not sweep
the pages that were already swept by this thread and clear the object mark bits
on these pages.
We might be able to trigger another sweep by abandoning the first sweep, but it
is a rather elaborate change and the above case should be very infrequent.

[haraken, 2014-07-01 11:39:36]

On 2014/07/01 11:10:59, wibling-chromium wrote:
> On 2014/07/01 10:35:10, haraken wrote:
> >
>
https://codereview.chromium.org/367633002/diff/1/Source/platform/heap/ThreadS...
> > File Source/platform/heap/ThreadState.h (right):
> > 
> >
>
https://codereview.chromium.org/367633002/diff/1/Source/platform/heap/ThreadS...
> > Source/platform/heap/ThreadState.h:623: // due to this thread not being at a
> > safepoint and waiting on the lock.
> > 
> > Would you elaborate on why this doesn't cause dead lock?
> > 
> > (1) Thread 1 is sweeping.
> > (2) Thread 2 calls SafePointAwareMutexLocker. It acquires the mutex.
> > (3) Thread 2 triggers a GC.
> > (4) Thread 1 calls SafePointAwareMutexLocker. It tries to acquire the mutex.
> > (5) Dead lock?
> 
> Thread 2 will time out in step 3 since Thread 1 will never enter a safepoint
as
> it was sweeping.
> This will not dead-lock but only lead to a GC which does not do anything. It
is
> unfortunate, but 
> at least it will continue after the timeout.
> 
> > Actually I don't fully understand why we cannot enter a safe point while
> > sweeping. It seems we can just enter a safe point, and if a GC is triggered
we
> > can just clear the mark bits and restart the GC. I'd be happy if you could
> just
> > help me understand :)
> 
> There is a couple of reasons. We do both thread local weak processing as well
as
> sweeping in the "sweep" phase (ThreadState::performPendingSweep).
> If we are doing weak processing and popping a weak callback we are using the
> thread specific weak callback stack which is also used when tracing. We don't
> currently support pushing weak callbacks from a GC thread while another thread
> is in the middle of popping them.
> If we are doing actual sweeping of a page and enters a safepoint in a
destructor
> we will get back and continue sweeping. However we will now have thrown away
any
> free list entries we have built up in the sweep so far. We will also not sweep
> the pages that were already swept by this thread and clear the object mark
bits
> on these pages.
> We might be able to trigger another sweep by abandoning the first sweep, but
it
> is a rather elaborate change and the above case should be very infrequent.

Thanks for the clarification, makes sense! I understand that a GC must not be
triggered during thread-specific weak processing (this is guaranteed by not
allowing allocation during thread-specific weak processing) and during sweeping
(this is guaranteed by not allowing to start a GC in Heap::allocate called in
destructors).

LGTM.