Remove special case code for the no-parallel-GC case
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 Sep 2015 20:13:07 +0000 (20:13 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 Sep 2015 20:13:07 +0000 (20:13 +0000)
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):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@190217 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/heap/SlotVisitor.cpp

index be2c03c..b1162d2 100644 (file)
@@ -1,5 +1,24 @@
 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>
+
         PolymorphicAccess should remember that it checked an ObjectPropertyCondition with a check on some structure
         https://bugs.webkit.org/show_bug.cgi?id=149514
 
index d04f32f..4d3770b 100644 (file)
@@ -428,6 +428,7 @@ private:
     Condition m_markingConditionVariable;
     MarkStackArray m_sharedMarkStack;
     unsigned m_numberOfActiveParallelMarkers { 0 };
+    unsigned m_numberOfWaitingParallelMarkers { 0 };
     bool m_parallelMarkersShouldExit { false };
 
     Lock m_opaqueRootsMutex;
index 76d25f6..a46d1f5 100644 (file)
@@ -147,8 +147,7 @@ void SlotVisitor::donateKnownParallel()
     // Otherwise, assume that a thread will go idle soon, and donate.
     m_stack.donateSomeCellsTo(m_heap.m_sharedMarkStack);
 
-    if (m_heap.m_numberOfActiveParallelMarkers < Options::numberOfGCMarkers())
-        m_heap.m_markingConditionVariable.notifyAll();
+    m_heap.m_markingConditionVariable.notifyAll();
 }
 
 void SlotVisitor::drain()
@@ -156,23 +155,14 @@ void SlotVisitor::drain()
     StackStats::probe();
     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();
 }
 
 void SlotVisitor::drainFromShared(SharedDrainMode sharedDrainMode)
@@ -182,18 +172,6 @@ void SlotVisitor::drainFromShared(SharedDrainMode sharedDrainMode)
     
     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);
         m_heap.m_numberOfActiveParallelMarkers++;
@@ -202,6 +180,7 @@ void SlotVisitor::drainFromShared(SharedDrainMode sharedDrainMode)
         {
             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.
             if (sharedDrainMode == MasterDrain) {
@@ -209,7 +188,8 @@ void SlotVisitor::drainFromShared(SharedDrainMode sharedDrainMode)
                 // for us to do.
                 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();
                         return;
@@ -226,19 +206,26 @@ void SlotVisitor::drainFromShared(SharedDrainMode sharedDrainMode)
                 ASSERT(sharedDrainMode == SlaveDrain);
                 
                 // 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.
                 if (m_heap.m_parallelMarkersShouldExit)
                     return;
             }
            
-            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--;
         }
         
         drain();