Changeset 179609 in webkit

Timestamp:

Feb 4, 2015, 10:00:05 AM (10 years ago)

Author:

mark.lam@apple.com

Message:

Remove concept of makeUsableFromMultipleThreads().
<​https://webkit.org/b/141221>

Reviewed by Mark Hahnenberg.

Source/JavaScriptCore:

Currently, we rely on VM::makeUsableFromMultipleThreads() being called before we
start acquiring the JSLock and entering the VM from different threads.
Acquisition of the JSLock will register the acquiring thread with the VM's thread
registry if not already registered. However, it will only do this if the VM's
thread specific key has been initialized by makeUsableFromMultipleThreads().

This is fragile, and also does not read intuitively because one would expect to
acquire the JSLock before calling any methods on the VM. This is exactly what
JSGlobalContextCreateInGroup() did (i.e. acquire the lock before calling
makeUsableFromMultipleThreads()), but is wrong. The result is that the invoking
thread will not have been registered with the VM during that first entry into
the VM.

The fix is to make it so that we initialize the VM's thread specific key on
construction of the VM's MachineThreads registry instead of relying on
makeUsableFromMultipleThreads() being called. With this, we can eliminate
makeUsableFromMultipleThreads() altogether.

Performance results are neutral in aggregate.

• API/JSContextRef.cpp:

(JSGlobalContextCreateInGroup):

• heap/MachineStackMarker.cpp:

(JSC::MachineThreads::MachineThreads):
(JSC::MachineThreads::~MachineThreads):
(JSC::MachineThreads::addCurrentThread):
(JSC::MachineThreads::removeThread):
(JSC::MachineThreads::gatherConservativeRoots):
(JSC::MachineThreads::makeUsableFromMultipleThreads): Deleted.

• heap/MachineStackMarker.h:
• runtime/VM.cpp:

(JSC::VM::sharedInstance):

• runtime/VM.h:

(JSC::VM::makeUsableFromMultipleThreads): Deleted.

Source/WebCore:

No new tests.

• bindings/js/JSDOMWindowBase.cpp:

(WebCore::JSDOMWindowBase::commonVM):

Location:

trunk/Source

Files:

• JavaScriptCore/API/JSContextRef.cpp (modified) (1 diff)
• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/heap/MachineStackMarker.cpp (modified) (4 diffs)
• JavaScriptCore/heap/MachineStackMarker.h (modified) (1 diff)
• JavaScriptCore/runtime/VM.cpp (modified) (1 diff)
• JavaScriptCore/runtime/VM.h (modified) (1 diff)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/bindings/js/JSDOMWindowBase.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/API/JSContextRef.cpp

@@ -139 +139 @@
 
     JSLockHolder locker(vm.get());
-    vm->makeUsableFromMultipleThreads();
 
     if (!globalObjectClass) {

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-02-04  Mark Lam  <mark.lam@apple.com>
+
+        Remove concept of makeUsableFromMultipleThreads().
+        <https://webkit.org/b/141221>
+
+        Reviewed by Mark Hahnenberg.
+
+        Currently, we rely on VM::makeUsableFromMultipleThreads() being called before we
+        start acquiring the JSLock and entering the VM from different threads.
+        Acquisition of the JSLock will register the acquiring thread with the VM's thread
+        registry if not already registered.  However, it will only do this if the VM's
+        thread specific key has been initialized by makeUsableFromMultipleThreads().
+
+        This is fragile, and also does not read intuitively because one would expect to
+        acquire the JSLock before calling any methods on the VM.  This is exactly what
+        JSGlobalContextCreateInGroup() did (i.e. acquire the lock before calling
+        makeUsableFromMultipleThreads()), but is wrong.  The result is that the invoking
+        thread will not have been registered with the VM during that first entry into
+        the VM.
+
+        The fix is to make it so that we initialize the VM's thread specific key on
+        construction of the VM's MachineThreads registry instead of relying on
+        makeUsableFromMultipleThreads() being called.  With this, we can eliminate
+        makeUsableFromMultipleThreads() altogether.
+
+        Performance results are neutral in aggregate.
+
+        * API/JSContextRef.cpp:
+        (JSGlobalContextCreateInGroup):
+        * heap/MachineStackMarker.cpp:
+        (JSC::MachineThreads::MachineThreads):
+        (JSC::MachineThreads::~MachineThreads):
+        (JSC::MachineThreads::addCurrentThread):
+        (JSC::MachineThreads::removeThread):
+        (JSC::MachineThreads::gatherConservativeRoots):
+        (JSC::MachineThreads::makeUsableFromMultipleThreads): Deleted.
+        * heap/MachineStackMarker.h:
+        * runtime/VM.cpp:
+        (JSC::VM::sharedInstance):
+        * runtime/VM.h:
+        (JSC::VM::makeUsableFromMultipleThreads): Deleted.
+
 2015-02-04  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp

@@ -124 +124 @@
 {
     UNUSED_PARAM(heap);
+    threadSpecificKeyCreate(&m_threadSpecific, removeThread);
 }
 
 MachineThreads::~MachineThreads()
 {
-    if (m_threadSpecific)
-        threadSpecificKeyDelete(m_threadSpecific);
+    threadSpecificKeyDelete(m_threadSpecific);
 
     MutexLocker registeredThreadsLock(m_registeredThreadsMutex);
@@ -161 +161 @@
 }
 
-void MachineThreads::makeUsableFromMultipleThreads()
-{
-    if (m_threadSpecific)
+void MachineThreads::addCurrentThread()
+{
+    ASSERT(!m_heap->vm()->hasExclusiveThread() || m_heap->vm()->exclusiveThread() == std::this_thread::get_id());
+
+    if (threadSpecificGet(m_threadSpecific)) {
+        ASSERT(threadSpecificGet(m_threadSpecific) == this);
         return;
-
-    threadSpecificKeyCreate(&m_threadSpecific, removeThread);
-}
-
-void MachineThreads::addCurrentThread()
-{
-    ASSERT(!m_heap->vm()->hasExclusiveThread() || m_heap->vm()->exclusiveThread() == std::this_thread::get_id());
-
-    if (!m_threadSpecific || threadSpecificGet(m_threadSpecific))
-        return;
+    }
 
     threadSpecificSet(m_threadSpecific, this);
@@ -187 +181 @@
 void MachineThreads::removeThread(void* p)
 {
-    if (p)
-        static_cast<MachineThreads*>(p)->removeCurrentThread();
+    static_cast<MachineThreads*>(p)->removeCurrentThread();
 }
 
@@ -522 +515 @@
     gatherFromCurrentThread(conservativeRoots, jitStubRoutines, codeBlocks, stackCurrent, currentThreadRegisters);
 
-    if (!m_threadSpecific)
-        return;
-
     size_t size;
     size_t capacity = 0;

trunk/Source/JavaScriptCore/heap/MachineStackMarker.h

@@ -45 +45 @@
         void gatherConservativeRoots(ConservativeRoots&, JITStubRoutineSet&, CodeBlockSet&, void* stackCurrent, RegisterState& registers);
 
-        JS_EXPORT_PRIVATE void makeUsableFromMultipleThreads();
         JS_EXPORT_PRIVATE void addCurrentThread(); // Only needs to be called by clients that can use the same heap from multiple threads.
 

trunk/Source/JavaScriptCore/runtime/VM.cpp

@@ -358 +358 @@
     GlobalJSLock globalLock;
     VM*& instance = sharedInstanceInternal();
-    if (!instance) {
+    if (!instance)
         instance = adoptRef(new VM(APIShared, SmallHeap)).leakRef();
-        instance->makeUsableFromMultipleThreads();
-    }
     return *instance;
 }

trunk/Source/JavaScriptCore/runtime/VM.h

@@ -218 +218 @@
     JS_EXPORT_PRIVATE ~VM();
 
-    void makeUsableFromMultipleThreads() { heap.machineThreads().makeUsableFromMultipleThreads(); }
-
 private:
     RefPtr<JSLock> m_apiLock;

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-02-04  Mark Lam  <mark.lam@apple.com>
+
+        Remove concept of makeUsableFromMultipleThreads().
+        <https://webkit.org/b/141221>
+
+        Reviewed by Mark Hahnenberg.
+
+        No new tests.
+
+        * bindings/js/JSDOMWindowBase.cpp:
+        (WebCore::JSDOMWindowBase::commonVM):
+
 2015-02-04  Simon Fraser  <simon.fraser@apple.com>
 

trunk/Source/WebCore/bindings/js/JSDOMWindowBase.cpp

@@ -213 +213 @@
 #endif
         vm->heap.setIncrementalSweeper(std::make_unique<WebSafeIncrementalSweeper>(&vm->heap));
-        vm->makeUsableFromMultipleThreads();
         vm->heap.machineThreads().addCurrentThread();
 #endif