Bug 128451

Summary: Change JSLock::dropAllLocks() and friends to use lock() and unlock()
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, mhahnenberg, mmirman, msaboff, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 128450    
Bug Blocks:    
Attachments:
Description Flags
work in progress.
ggaren: review-
updated patch now that 128450 has landed. ggaren: review+

Description Mark Lam 2014-02-08 00:39:28 PST
The current implementations of JSLock's dropAllLocks(), dropAllLocksUnconditionally(), and grabAllLocks() currently implement locking / unlocking by duplicating the code from lock() and unlock().  Instead, they should just call lock() and unlock() instead.
Comment 1 Mark Lam 2014-02-08 10:02:43 PST
Created attachment 223569 [details]
work in progress.

Let's try this on the EWS bots.  This patch is dependent on <https://webkit.org/b/128450> landing.  Once 128450 lands, this patch should reduce in size a bit.
Comment 2 Geoffrey Garen 2014-02-08 10:56:49 PST
Comment on attachment 223569 [details]
work in progress.

View in context: https://bugs.webkit.org/attachment.cgi?id=223569&action=review

> Source/JavaScriptCore/ChangeLog:11
> +        lock() and unlock(). Instead, they should just call lock() and unlock()
> +        instead.

Instead instead.

> Source/JavaScriptCore/ChangeLog:19
> +        - Also clear m_ownerThread when we're going to relinquish the lock.

Let's not mix this behavior change into a refactoring patch.

> Source/JavaScriptCore/ChangeLog:55
> +2014-02-08  Mark Lam  <mark.lam@apple.com>
> +
> +        DropAllLocks should let JSLock lock its spinLock.
> +        <https://webkit.org/b/128450>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Instead of having DropAllLock acquire JSLock's spinLock and then passing
> +        it into dropAllLocks(), dropAllLocksUnconditionally(), and grabAllLocks(),
> +        let those JSLock methods acquire the spinLock themselves.
> +
> +        * runtime/JSLock.cpp:
> +        (JSC::JSLock::dropAllLocks):
> +        (JSC::JSLock::dropAllLocksUnconditionally):
> +        (JSC::JSLock::grabAllLocks):
> +        (JSC::JSLock::DropAllLocks::DropAllLocks):
> +        (JSC::JSLock::DropAllLocks::~DropAllLocks):
> +        * runtime/JSLock.h:

Remove please.

> Source/JavaScriptCore/runtime/JSLock.cpp:228
>      // Don't drop the locks if they've already been dropped once.
>      // (If the prior drop came from another thread, and it resumed first,
>      // it could trash our register file).
>      if (m_lockDropDepth)
>          return 0;

m_lockDropDepth is a shared value, so it's not sound to read m_lockDropDepth without holding the spinlock.

> Source/JavaScriptCore/runtime/JSLock.cpp:276
> +    ASSERT(!m_vm || !m_vm->stackPointerAtVMEntry);

I don't think anything prevents another thread from setting these values once we've relinquished the lock, so this ASSERT seems invalid.
Comment 3 Mark Lam 2014-02-09 23:46:36 PST
Comment on attachment 223569 [details]
work in progress.

View in context: https://bugs.webkit.org/attachment.cgi?id=223569&action=review

>> Source/JavaScriptCore/runtime/JSLock.cpp:228
>>          return 0;
> 
> m_lockDropDepth is a shared value, so it's not sound to read m_lockDropDepth without holding the spinlock.

We don’t need the spinLock here because m_lockDropDepth is (well ... should have been) protected by the JSLock’s own mutex.  Only the owner of the JSLock may read or write to m_lockDropDepth.  So, I still have a bug here i.e. I cannot move this m_lockDropDepth check before the ownership check below (which was previously above it).  Will fix.

>> Source/JavaScriptCore/runtime/JSLock.cpp:276
>> +    ASSERT(!m_vm || !m_vm->stackPointerAtVMEntry);
> 
> I don't think anything prevents another thread from setting these values once we've relinquished the lock, so this ASSERT seems invalid.

Thanks for catching this.  I thought of it when I contemplated adding a similar assert to grabAllLocks() (which is why I didn’t add such an assert there), but then went on to miss the invalidity of it here.  Will remove.
Comment 4 Mark Lam 2014-02-10 12:55:50 PST
Created attachment 223735 [details]
updated patch now that 128450 has landed.
Comment 5 Mark Lam 2014-02-10 13:00:23 PST
(In reply to comment #4)
> Created an attachment (id=223735) [details]
> updated patch now that 128451 has landed.

Correction: “that 128450 has landed”, not 128451.
Comment 6 Geoffrey Garen 2014-02-10 14:16:04 PST
Comment on attachment 223735 [details]
updated patch now that 128450 has landed.

r=me
Comment 7 Mark Lam 2014-02-10 14:23:28 PST
Thanks.  Landed in r163820: <http://trac.webkit.org/r163820>.