[oilpan]: Add SafePointAwareMutexLocker to allow GC when waiting for lock.

[oilpan]: Add SafePointAwareMutexLocker to allow GC when waiting for lock.

This change add a mutex locker which enters a safepoint while trying to acquire the mutex lock. This means that threads waiting to acquire a lock would be parked allowing a GC.
In addition to entering a safepoint the locker will also ensure that if we block while leaving the safepoint, due to another thread having scheduled a GC, the lock will be released and the safepoint reentered. This allows for the lock to be acquired during sweeping, specifically in weak processing and finalization which could otherwise deadlock and cause the other scheduled GC to time out.

This change does however not fix the issue where the lock has been acquired and another safepoint is entered while having the lock. This could still cause a timeout. There is an instance of this scenario in the webdatabase's DatabaseBackendBase.cpp::performOpenAndVerify where we get the guidMutex and while commiting the transaction enter a safepoint in the SQLiteStatement::prepare and SQLiteStatement::step calls.
To fix the above we have to restructure the webdatabase code to not hold the global guidMutex lock while doing sync operations.

This change does make it less likely that we hit the empty assert in the terminate-during-sync-websql.html test, but due to the above issue it can still happen. We should consider still doing the restructuring in https://codereview.chromium.org/335823002/ to completely get rid of the flakiness.


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=375671
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176347

[zerny-chromium, 2014-06-17 11:12:56]

lgtm

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

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:610: // SafePointBarrier::checkAndPark.
Nits:
The SafePointAwareMutexLocker is -> SafePointAwareMutexLocker is
for mutex lock -> for a mutex lock.

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:614: SafePointAwareMutexLocker(Mutex& mutex)
: m_mutex(mutex), m_locked(false)
nit: explicit

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:626: state->leaveSafePoint(this);
Maybe add a comment here about "leaveSafePoint" possibly releasing the lock via
reset().

[Mads Ager (chromium), 2014-06-17 11:57:19]

LGTM

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

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:195: // lock to be acquired in the sweep
phase, e.g. during weak processing
lock to be -> the lock to be

[wibling-chromium, 2014-06-17 12:06:41]

Thanks for the reviews! I have updated the diff accordingly.

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

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:195: // lock to be acquired in the sweep
phase, e.g. during weak processing
On 2014/06/17 11:57:19, Mads Ager (chromium) wrote:
> lock to be -> the lock to be

Done.

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

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:610: // SafePointBarrier::checkAndPark.
On 2014/06/17 11:12:56, zerny-chromium wrote:
> Nits:
> The SafePointAwareMutexLocker is -> SafePointAwareMutexLocker is
> for mutex lock -> for a mutex lock.

Done.

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:614: SafePointAwareMutexLocker(Mutex& mutex)
: m_mutex(mutex), m_locked(false)
On 2014/06/17 11:12:56, zerny-chromium wrote:
> nit: explicit

Done.

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:626: state->leaveSafePoint(this);
On 2014/06/17 11:12:56, zerny-chromium wrote:
> Maybe add a comment here about "leaveSafePoint" possibly releasing the lock
via
> reset().

Done.

[haraken, 2014-06-17 12:25:41]

This is a nice change! LGTM.

Just to confirm: Can (or should) we replace all MutexLockers with
SafePointAwareMutexLockers (unless the MutexLocker is explicitly written in some
safe point scope)?

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

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:189: void checkAndPark(ThreadState* state,
SafePointAwareMutexLocker* locker = 0)

Does the |locker| need to be an optional parameter?

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:755: ASSERT(!m_atSafePoint);

Just to confirm: The SafePointAwareLocker is reentrant, but the SafePoint itself
is not reentrant. In other words, the SafePointAwareLocker is implemented in a
way that it doesn't nest SafePoints used by the SafePointAwareLocker. Right?

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:759: m_atSafePoint = false;

Nice catch. We should have had these lines...

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

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:610: // SafePointBarrier::checkAndPark.

... It also ensures that the lock is not held while waiting for the
leaveSafePoint() to complete a GC. This is realized by releasing the lock when
the leaveSafePoint() cannot complete without blocking. See a comment in
SafePointBarrier::checkAndPark.

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:625: if (leaveSafePoint) {

Do we need |leaveSafePoint|? Probably we can use 'if (state->isAtSafePoint)'.

[wibling-chromium, 2014-06-17 12:38:10]

On 2014/06/17 12:25:41, haraken wrote:
> This is a nice change! LGTM.
> 
> Just to confirm: Can (or should) we replace all MutexLockers with
> SafePointAwareMutexLockers (unless the MutexLocker is explicitly written in
some
> safe point scope)?

Yes, we should at least investigate which MutexLockers it could make sense to
change. There are a couple in the webdatabase which could probably use this, but
I would prefer to do it in smaller increments to ensure we are not introducing
any unforseen issues. In most cases it should just work to replace them AFAICT.

[wibling-chromium, 2014-06-17 12:38:21]

Thanks for the review!

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

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:189: void checkAndPark(ThreadState* state,
SafePointAwareMutexLocker* locker = 0)
On 2014/06/17 12:25:41, haraken wrote:
> 
> Does the |locker| need to be an optional parameter?

I prefer it that way to avoid having to pass in 0 from the normal call sites. It
is a style preference, both would work.

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:755: ASSERT(!m_atSafePoint);
On 2014/06/17 12:25:41, haraken wrote:
> 
> Just to confirm: The SafePointAwareLocker is reentrant, but the SafePoint
itself
> is not reentrant. In other words, the SafePointAwareLocker is implemented in a
> way that it doesn't nest SafePoints used by the SafePointAwareLocker. Right?

The SafePointAwareLocker is only entering the safepoint when trying to acquire
the lock, ie. only during the constructor. This means that we are not in a
safepoint after leaving the SafePointAwareMutexLocker's constructor.
In that sense you can argue the SafePointAwareMutexLocker is reentrant, but it
is more that it is only in the safepoint while waiting on the acquisition of the
lock.

As you have noticed we also only try to enter the safepoint in the
SafePointAwareMutexLocker's constructor if we are not already in a safepoint.
This avoids us hitting the above assert. 

Basically we don't support entering a safepoint while in a safepoint, but we do
support using the SafePointAwareMutexLocker while in a safepoint.

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

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:625: if (leaveSafePoint) {
On 2014/06/17 12:25:41, haraken wrote:
> 
> Do we need |leaveSafePoint|? Probably we can use 'if (state->isAtSafePoint)'.

No, unfortunately not since we only want to leave the safepoint if we were the
one entering it.  Ie. we need to track that we explicitly called enterSafePoint
to make sure we don't release that was nesting the SafePointAwareMutexLocker.

[wibling-chromium, 2014-06-17 14:21:13]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-06-17 14:21:56]

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

[haraken, 2014-06-17 14:25:36]

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

https://codereview.chromium.org/332393002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:755: ASSERT(!m_atSafePoint);
On 2014/06/17 12:38:21, wibling-chromium wrote:
> On 2014/06/17 12:25:41, haraken wrote:
> > 
> > Just to confirm: The SafePointAwareLocker is reentrant, but the SafePoint
> itself
> > is not reentrant. In other words, the SafePointAwareLocker is implemented in
a
> > way that it doesn't nest SafePoints used by the SafePointAwareLocker. Right?
> 
> The SafePointAwareLocker is only entering the safepoint when trying to acquire
> the lock, ie. only during the constructor. This means that we are not in a
> safepoint after leaving the SafePointAwareMutexLocker's constructor.
> In that sense you can argue the SafePointAwareMutexLocker is reentrant, but it
> is more that it is only in the safepoint while waiting on the acquisition of
the
> lock.
> 
> As you have noticed we also only try to enter the safepoint in the
> SafePointAwareMutexLocker's constructor if we are not already in a safepoint.
> This avoids us hitting the above assert. 
> 
> Basically we don't support entering a safepoint while in a safepoint, but we
do
> support using the SafePointAwareMutexLocker while in a safepoint.

Thanks, makes a lot of sense!

[commit-bot: I haz the power, 2014-06-17 19:21:30]

Change committed as 176347