Changeset 190217 in webkit

Timestamp:

Sep 24, 2015 1:13:07 PM (9 years ago)

Author:

fpizlo@apple.com

Message:

Remove special case code for the no-parallel-GC case
​https://bugs.webkit.org/show_bug.cgi?id=149512

Reviewed by Mark Lam.

Make serial GC just a parallel GC where the helper threads don't do anything. Also make the
idle thread calculation a bit more explicit.

The main outcome is that we no longer use Options::numberOfGCMarkers() as much, so the code is
resilient against the number of GC markers changing.

• heap/Heap.h:
• heap/SlotVisitor.cpp:

(JSC::SlotVisitor::donateKnownParallel):
(JSC::SlotVisitor::drain):
(JSC::SlotVisitor::drainFromShared):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/Heap.h (modified) (1 diff)
• heap/SlotVisitor.cpp (modified) (7 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-09-23  Filip Pizlo  <fpizlo@apple.com>
+
+        Remove special case code for the no-parallel-GC case
+        https://bugs.webkit.org/show_bug.cgi?id=149512
+
+        Reviewed by Mark Lam.
+
+        Make serial GC just a parallel GC where the helper threads don't do anything. Also make the
+        idle thread calculation a bit more explicit.
+
+        The main outcome is that we no longer use Options::numberOfGCMarkers() as much, so the code is
+        resilient against the number of GC markers changing.
+
+        * heap/Heap.h:
+        * heap/SlotVisitor.cpp:
+        (JSC::SlotVisitor::donateKnownParallel):
+        (JSC::SlotVisitor::drain):
+        (JSC::SlotVisitor::drainFromShared):
+
 2015-09-23  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/heap/Heap.h

@@ -429 +429 @@
     MarkStackArray m_sharedMarkStack;
     unsigned m_numberOfActiveParallelMarkers { 0 };
+    unsigned m_numberOfWaitingParallelMarkers { 0 };
     bool m_parallelMarkersShouldExit { false };
 

trunk/Source/JavaScriptCore/heap/SlotVisitor.cpp

@@ -148 +148 @@
     m_stack.donateSomeCellsTo(m_heap.m_sharedMarkStack);
 
-    if (m_heap.m_numberOfActiveParallelMarkers < Options::numberOfGCMarkers())
-        m_heap.m_markingConditionVariable.notifyAll();
+    m_heap.m_markingConditionVariable.notifyAll();
 }
 
@@ -157 +156 @@
     ASSERT(m_isInParallelMode);
    
-    if (Options::numberOfGCMarkers() > 1) {
-        while (!m_stack.isEmpty()) {
-            m_stack.refill();
-            for (unsigned countdown = Options::minimumNumberOfScansBetweenRebalance(); m_stack.canRemoveLast() && countdown--;)
-                visitChildren(*this, m_stack.removeLast());
-            donateKnownParallel();
-        }
-        
-        mergeOpaqueRootsIfNecessary();
-        return;
-    }
-    
     while (!m_stack.isEmpty()) {
         m_stack.refill();
-        while (m_stack.canRemoveLast())
+        for (unsigned countdown = Options::minimumNumberOfScansBetweenRebalance(); m_stack.canRemoveLast() && countdown--;)
             visitChildren(*this, m_stack.removeLast());
-    }
+        donateKnownParallel();
+    }
+    
+    mergeOpaqueRootsIfNecessary();
 }
 
@@ -183 +173 @@
     ASSERT(Options::numberOfGCMarkers());
     
-    bool shouldBeParallel;
-
-    shouldBeParallel = Options::numberOfGCMarkers() > 1;
-    
-    if (!shouldBeParallel) {
-        // This call should be a no-op.
-        ASSERT_UNUSED(sharedDrainMode, sharedDrainMode == MasterDrain);
-        ASSERT(m_stack.isEmpty());
-        ASSERT(m_heap.m_sharedMarkStack.isEmpty());
-        return;
-    }
-    
     {
         std::lock_guard<Lock> lock(m_heap.m_markingMutex);
@@ -203 +181 @@
             std::unique_lock<Lock> lock(m_heap.m_markingMutex);
             m_heap.m_numberOfActiveParallelMarkers--;
+            m_heap.m_numberOfWaitingParallelMarkers++;
 
             // How we wait differs depending on drain mode.
@@ -210 +189 @@
                 while (true) {
                     // Did we reach termination?
-                    if (!m_heap.m_numberOfActiveParallelMarkers && m_heap.m_sharedMarkStack.isEmpty()) {
+                    if (!m_heap.m_numberOfActiveParallelMarkers
+                        && m_heap.m_sharedMarkStack.isEmpty()) {
                         // Let any sleeping slaves know it's time for them to return;
                         m_heap.m_markingConditionVariable.notifyAll();
@@ -227 +207 @@
                 
                 // Did we detect termination? If so, let the master know.
-                if (!m_heap.m_numberOfActiveParallelMarkers && m_heap.m_sharedMarkStack.isEmpty())
+                if (!m_heap.m_numberOfActiveParallelMarkers
+                    && m_heap.m_sharedMarkStack.isEmpty())
                     m_heap.m_markingConditionVariable.notifyAll();
 
-                m_heap.m_markingConditionVariable.wait(lock, [this] { return !m_heap.m_sharedMarkStack.isEmpty() || m_heap.m_parallelMarkersShouldExit; });
+                m_heap.m_markingConditionVariable.wait(
+                    lock,
+                    [this] {
+                        return !m_heap.m_sharedMarkStack.isEmpty()
+                            || m_heap.m_parallelMarkersShouldExit;
+                    });
                 
                 // Is the current phase done? If so, return from this function.
@@ -237 +223 @@
             }
            
-            size_t idleThreadCount = Options::numberOfGCMarkers() - m_heap.m_numberOfActiveParallelMarkers;
-            m_stack.stealSomeCellsFrom(m_heap.m_sharedMarkStack, idleThreadCount);
+            m_stack.stealSomeCellsFrom(
+                m_heap.m_sharedMarkStack, m_heap.m_numberOfWaitingParallelMarkers);
             m_heap.m_numberOfActiveParallelMarkers++;
+            m_heap.m_numberOfWaitingParallelMarkers--;
         }