[webkit-reviews] review denied: [Bug 89123] JSLock should be per-JSGlobalData : [Attachment 147925] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 89123: JSLock should be per-JSGlobalData
https://bugs.webkit.org/show_bug.cgi?id=89123

Attachment 147925: Patch
https://bugs.webkit.org/attachment.cgi?id=147925&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=147925&action=review


Good first step, but it looks like this needs a bit more work.

> Source/JavaScriptCore/ChangeLog:109
> +	   (JSC::GlobalJSLock::GlobalJSLock): This is our new global lock that
must be taken when creating new contexts.
> +	   (JSC::GlobalJSLock::~GlobalJSLock):

I didn't understand this part. What data does the global lock protect?

> Source/JavaScriptCore/jsc.cpp:686
> +    globalData.clear();

This should be automatic.

> Source/JavaScriptCore/heap/Heap.cpp:173
> +    if (globalData->isSharedInstance() &&
!isValidSharedInstanceThreadState(globalData))

Even non-"shared instance" VMs need locking, since they need mutual exclusion
against GC on another thread. So, I think you should remove the
isSharedInstance() escape from this ASSERT, and similar ASSERTs.

> Source/JavaScriptCore/heap/Heap.cpp:326
> +    ASSERT(m_globalData->apiLock().currentThreadIsHoldingLock() ||
!m_globalData->isSharedInstance());

Same comment about isSharedInstance() ASSERTs.

> Source/JavaScriptCore/heap/Heap.cpp:337
> +    ASSERT(m_globalData->apiLock().currentThreadIsHoldingLock() ||
!m_globalData->isSharedInstance());

Same comment about isSharedInstance() ASSERTs.

> Source/JavaScriptCore/heap/Heap.cpp:691
> +    ASSERT(globalData()->apiLock().currentThreadIsHoldingLock());

Same comment about isSharedInstance() ASSERTs.

> Source/JavaScriptCore/runtime/JSGlobalData.cpp:230
> +    // This must be the very first thing we do before tearing down any more
of this object
> +    // because the Heap's GCActivityCallback could run another GC on a
remote thread.
> +    heap.activityCallback()->signalAndWaitForExit();
> +    heap.sweeper()->signalAndWaitForExit();

Trying to reason through this signal and wait tear-down phase hurt my brain a
little bit.

The fundamental problem we're trying to solve here is that a timer might fire
with a pointer to an invalid context (since JSGlobalData tear-down may have
freed the memory pointed to by the context). (The timer itself will still be
valid since it's referenced by the run loop.)

One possible alternative is to have our timers ref / deref the JSGlobalData.
Maybe we can come up with something better.

> Source/JavaScriptCore/runtime/JSGlobalData.h:185
> +	   JSLock m_apiLock;

Do we only need this lock for API clients, or does WebKit need to use it too?

> Source/JavaScriptCore/runtime/JSLock.cpp:79
> +    if (m_globalData->refCount() > 1)
> +	   m_globalData->apiLock().unlock();

It's a bit sketchy to test the refCount of something that can be ref'd/deref'd
on multiple threads. Is there another way to do this?

> Source/JavaScriptCore/runtime/JSLock.cpp:99
> +    if (m_lockBehavior == SilenceAssertionsOnly)

I don't think we can support SilenceAssertionsOnly mode anymore, can we?

> Source/JavaScriptCore/runtime/JSLock.cpp:106
> +	       SpinLockHolder holder(&m_spinLock);

I don't think the extra layer of locking buys us anything. Is this a speedup on
something?