Changeset 167733 in webkit

Timestamp:

Apr 23, 2014 5:43:15 PM (10 years ago)

Author:

mark.lam@apple.com

Message:

The GC should only resume compiler threads that it suspended in the same GC pass.
<​https://webkit.org/b/132088>

Reviewed by Mark Hahnenberg.

Previously, this scenario can occur:

1. Thread 1 starts a GC and tries to suspend DFG worklist threads. However, no worklists were created yet at the that time.
2. Thread 2 starts to compile some functions and creates a DFG worklist, and acquires the worklist thread's lock.
3. Thread 1's GC completes and tries to resume suspended DFG worklist thread. This time, it sees the worklist created by Thread 2 and ends up unlocking the worklist thread's lock that is supposedly held by Thread 2.

Thereafter, chaos ensues.

The fix is to cache the worklists that were actually suspended by each GC pass,
and only resume those when the GC is done.

This issue was discovered by enabling COLLECT_ON_EVERY_ALLOCATION and running
the fast/workers layout tests.

• heap/Heap.cpp:

(JSC::Heap::visitCompilerWorklists):
(JSC::Heap::deleteAllCompiledCode):
(JSC::Heap::suspendCompilerThreads):
(JSC::Heap::resumeCompilerThreads):

• heap/Heap.h:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/Heap.cpp (modified) (4 diffs)
• heap/Heap.h (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2014-04-23  Mark Lam  <mark.lam@apple.com>
+
+        The GC should only resume compiler threads that it suspended in the same GC pass.
+        <https://webkit.org/b/132088>
+
+        Reviewed by Mark Hahnenberg.
+
+        Previously, this scenario can occur:
+        1. Thread 1 starts a GC and tries to suspend DFG worklist threads.  However,
+           no worklists were created yet at the that time.
+        2. Thread 2 starts to compile some functions and creates a DFG worklist, and
+           acquires the worklist thread's lock.
+        3. Thread 1's GC completes and tries to resume suspended DFG worklist thread.
+           This time, it sees the worklist created by Thread 2 and ends up unlocking
+           the worklist thread's lock that is supposedly held by Thread 2.
+        Thereafter, chaos ensues.
+
+        The fix is to cache the worklists that were actually suspended by each GC pass,
+        and only resume those when the GC is done.
+
+        This issue was discovered by enabling COLLECT_ON_EVERY_ALLOCATION and running
+        the fast/workers layout tests.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::visitCompilerWorklists):
+        (JSC::Heap::deleteAllCompiledCode):
+        (JSC::Heap::suspendCompilerThreads):
+        (JSC::Heap::resumeCompilerThreads):
+        * heap/Heap.h:
+
 2014-04-23  Mark Hahnenberg  <mhahnenberg@apple.com>
 

trunk/Source/JavaScriptCore/heap/Heap.cpp

@@ -627 +627 @@
 #if ENABLE(DFG_JIT)
     GCPHASE(VisitDFGWorklists);
-    for (unsigned i = DFG::numberOfWorklists(); i--;) {
-        if (DFG::Worklist* worklist = DFG::worklistForIndexOrNull(i))
-            worklist->visitChildren(m_slotVisitor, m_codeBlocks);
-    }
+    for (auto worklist : m_suspendedCompilerWorklists)
+        worklist->visitChildren(m_slotVisitor, m_codeBlocks);
 
     if (Options::logGC() == GCLogging::Verbose)
@@ -882 +880 @@
     // recompilations isn't a good idea.
 #if ENABLE(DFG_JIT)
-    for (unsigned i = DFG::numberOfWorklists(); i--;) {
-        if (DFG::Worklist* worklist = DFG::worklistForIndexOrNull(i)) {
-            if (worklist->isActiveForVM(*vm()))
-                return;
-        }
+    for (auto worklist : m_suspendedCompilerWorklists) {
+        if (worklist->isActiveForVM(*vm()))
+            return;
     }
 #endif // ENABLE(DFG_JIT)
@@ -1023 +1019 @@
 #if ENABLE(DFG_JIT)
     GCPHASE(SuspendCompilerThreads);
+    ASSERT(m_suspendedCompilerWorklists.isEmpty());
     for (unsigned i = DFG::numberOfWorklists(); i--;) {
-        if (DFG::Worklist* worklist = DFG::worklistForIndexOrNull(i))
+        if (DFG::Worklist* worklist = DFG::worklistForIndexOrNull(i)) {
+            m_suspendedCompilerWorklists.append(worklist);
             worklist->suspendAllThreads();
+        }
     }
 #endif
@@ -1228 +1227 @@
 #if ENABLE(DFG_JIT)
     GCPHASE(ResumeCompilerThreads);
-    for (unsigned i = DFG::numberOfWorklists(); i--;) {
-        if (DFG::Worklist* worklist = DFG::worklistForIndexOrNull(i))
-            worklist->resumeAllThreads();
-    }
+    for (auto worklist : m_suspendedCompilerWorklists)
+        worklist->resumeAllThreads();
+    m_suspendedCompilerWorklists.clear();
 #endif
 }

trunk/Source/JavaScriptCore/heap/Heap.h

@@ -71 +71 @@
 class WeakGCHandlePool;
 class SlotVisitor;
+
+namespace DFG {
+class Worklist;
+}
 
 typedef std::pair<JSValue, WTF::String> ValueStringPair;
@@ -374 +378 @@
     
     unsigned m_deferralDepth;
+    Vector<DFG::Worklist*> m_suspendedCompilerWorklists;
 };