Bug 128450 – Remove unnecessary spinLock in JSLock.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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.

[Mark Lam]

Created attachment 223560 [details]
the patch.

[Geoffrey Garen]

Comment on attachment 223560 [details]
the patch.

This code implies that the spinlock guards m_lockCount, and that multiple threads might point to the same JSLock. If so, it's not valid to read m_lockCount without holding the spinlock.

[Mark Lam]

(In reply to comment #2)
> (From update of attachment 223560 [details])
> This code implies that the spinlock guards m_lockCount, and that multiple threads might point to the same JSLock. If so, it's not valid to read m_lockCount without holding the spinlock.

I think you misread the code.  But I’ll rename some variables and field to make things clearer first before we revisit this patch.

[Mark Lam]

Upon further inspection of the JSLock code, we discovered that the spinLock is not actually necessary because the JSLock’s mutex already provide the needed protection for JSLock’s internal state.  The only remaining issue is that we’ll need to add some memory barriers to so we don’t have issues with out of order read-writes on Weak Memory Ordering architectures like ARM.

We also need to clear JSLock::m_ownerThread when unlocking the JSLock.

Patch coming soon.

[Mark Lam]

Created attachment 223678 [details]
patch 2: removes spinLock.

[Filip Pizlo]

Comment on attachment 223678 [details]
patch 2: removes spinLock.

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

This looks right to me.  I recommend getting a second review since this is tricky code.  Also please remove any unnecessary scopes now that you're not using them for the destruction of SpinLockHolders.

> Source/JavaScriptCore/runtime/JSLock.cpp:113
>      {

This nesting scope is no longer necessary now that you've removed the SpinLockHolder.

[Mark Lam]

(In reply to comment #4)
> Upon further inspection of the JSLock code, we discovered that the spinLock is not actually necessary because the JSLock’s mutex already provide the needed protection for JSLock’s internal state.  The only remaining issue is that we’ll need to add some memory barriers to so we don’t have issues with out of order read-writes on Weak Memory Ordering architectures like ARM.

The out of order read-writes here refer to reads and writes of the value of JSLock::m_ownerThread since we’re using it for the ownership test without requiring a lock around its access.

(In reply to comment #6)
> > Source/JavaScriptCore/runtime/JSLock.cpp:113
> >      {
> 
> This nesting scope is no longer necessary now that you've removed the SpinLockHolder.

Thanks for the review.  As indicated in the ChangeLog, I deliberately left the scope in place so that the diff does not look bigger than it has to (to make it clearer as to what actually change).  I had intend to remove it in a subsequent patch.  However, if you and the 2nd reviewer agrees on removing it right away, I can remove it before landing this patch.

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #4)
> > Upon further inspection of the JSLock code, we discovered that the spinLock is not actually necessary because the JSLock’s mutex already provide the needed protection for JSLock’s internal state.  The only remaining issue is that we’ll need to add some memory barriers to so we don’t have issues with out of order read-writes on Weak Memory Ordering architectures like ARM.
> 
> The out of order read-writes here refer to reads and writes of the value of JSLock::m_ownerThread since we’re using it for the ownership test without requiring a lock around its access.
> 
> (In reply to comment #6)
> > > Source/JavaScriptCore/runtime/JSLock.cpp:113
> > >      {
> > 
> > This nesting scope is no longer necessary now that you've removed the SpinLockHolder.
> 
> Thanks for the review.  As indicated in the ChangeLog, I deliberately left the scope in place so that the diff does not look bigger than it has to (to make it clearer as to what actually change).  I had intend to remove it in a subsequent patch.  However, if you and the 2nd reviewer agrees on removing it right away, I can remove it before landing this patch.

FWIW, I would have found this change easier to read if you had removed the scope.

[Geoffrey Garen]

Comment on attachment 223678 [details]
patch 2: removes spinLock.

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

> Source/JavaScriptCore/runtime/JSLock.h:122
>          ThreadIdentifier m_ownerThread;

Let's use std::atomic for m_ownerThread.

Advantages:

(1) You can remove 5 paragraphs of text from your ChangeLog, and from the minds of any programmers who work with this code.

(2) The code will not mysteriously start failing if someone changes ThreadIdentifier into a struct.

Disadvantages:

(1) Maybe a little slower.

I assert that (1) does not matter, but if you think it does, you can always specify memory_order_acquire / memory_order_release to get identical performance to what you have here.

[Filip Pizlo]

(In reply to comment #9)
> (From update of attachment 223678 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223678&action=review
> 
> > Source/JavaScriptCore/runtime/JSLock.h:122
> >          ThreadIdentifier m_ownerThread;
> 
> Let's use std::atomic for m_ownerThread.
> 
> Advantages:
> 
> (1) You can remove 5 paragraphs of text from your ChangeLog, and from the minds of any programmers who work with this code.

I would have removed those 5 paragraphs anyway. ;-)  I also don't think that the memory fence is necessary.  The memory allocator will have already executed the required fences.

The allocator would have *had* to execute a fence because:

- If this kind of weirdness was actually possible, I suspect a lot of intentionally racy algorithms would start failing.

- A block of freed memory cannot make its way from thread A to thread B without some synchronization, which would lead to all the required fences being executed on thread A and thread B.

> 
> (2) The code will not mysteriously start failing if someone changes ThreadIdentifier into a struct.

I think there is a lot of code that will mysteriously start failing if someone made that change.

> 
> Disadvantages:
> 
> (1) Maybe a little slower.
> 
> I assert that (1) does not matter, but if you think it does, you can always specify memory_order_acquire / memory_order_release to get identical performance to what you have here.

Using std::atomic without it being necessary risks creating a cargo cult.  I actually don't think there's anything crazy about this field, or the way it is accessed, and we don't need a memory fence here.

[Filip Pizlo]

Comment on attachment 223678 [details]
patch 2: removes spinLock.

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

> Source/JavaScriptCore/ChangeLog:62
> +           Consider the following scenario:
> +           1. Thread A allocated some heap memory M1 for some use.
> +           2. Thread A writes its thread id to memory M1.
> +              The written value is flushed to memory.
> +           3. Thread A frees memory M1. Thread A's thread id remains in memory M1.
> +
> +           4. Thread B allocates a JSLock which coincidentally gets located in M1.
> +              Thread B writes its thread id to the lock's m_ownerThread.
> +              The thread id is present in the CPU write back buffer but is not
> +              flushed to memory yet.
> +
> +           5. Before B's thread id is flushed to memory, Thread A gets a hold
> +              of the JSLock and checks its m_ownerThread.
> +
> +              Because Thread B's thread id is still in the write back buffer.
> +              the m_ownerThread value that Thread A sees is the stale value of
> +              Thread A's thread id. This erroneously tells Thread A that it
> +              owns the JSLock when it doesn't.
> +
> +           To prevent this issue (unlikely as it may be), we execute a memory
> +           barrier instruction right before we read m_ownerThread and right
> +           after we write to it to ensure the store to m_ownerThread in memory
> +           is completed before reads from it are executed.

I don't think that this issue is a real thing.  There's no way for thread B to allocate memory in the same place where thread A freed memory without all of the required synchronization being executed.

> Source/JavaScriptCore/runtime/JSLock.cpp:158
> +    WTF::loadLoadFence();    

Get rid of this.

> Source/JavaScriptCore/runtime/JSLock.cpp:165
> +    WTF::storeStoreFence();

Get rid of this.

[Geoffrey Garen]

I agree: the malloc case would be a bug in malloc. Malloc's one job is to ensure that all writes to a memory location "happens before" reuse of that memory location.

[Mark Lam]

Created attachment 223725 [details]
patch for landing.

Thanks for Geoff’s and Filip’s feedback.  In this updated patch, I did the following:
1. removed the memory fences
2. changed setOwnerThread() into an inline function since it is now a trivial single assignment.
3. removed the unnecessary scoping in JSLock::lock() now that reviewers have had a chance to see the diffs before its removal.
4. removed the ChangeLog comments about the necessity for a memory barrier/fence.
5. added a paragraph to explain how weak memory ordering can cause Thread B to not see Thread A’s id in m_ownerThread when doing an ownership test, but that is immaterial because Thread B will see a 0 (written out courtesy of the memory fence in the last mutex unlock) and know that it does not own the lock.

[Mark Lam]

Comment on attachment 223725 [details]
patch for landing.

Per my offline conversation with fpizlo, his r+ from the previous patch applies to this updated patch as well.  Will land shortly.

[Mark Lam]

Landed in r163804: <http://trac.webkit.org/r163804>.