It should be possible to disable concurrent GC timeslicing
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Nov 2016 22:02:01 +0000 (22:02 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Nov 2016 22:02:01 +0000 (22:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164788

Reviewed by Saam Barati.

Collector timeslicing means that the collector will try to pause once every 2ms. This is
great because it throttles the mutator and prevents it from outpacing the collector. But
it reduces some of the efficacy of the collectContinuously=true configuration: while
it's great that collecting continuously means that the collector will also pause more
frequently and so it will test the pausing code, it also means that the collector will
spend less time running concurrently. The primary purpose of collectContinuously is to
maximize the amount of time that the collector is running concurrently to the mutator to
maximize the likelihood that a race will cause a detectable error.

This adds an option to disable collector timeslicing (useCollectorTimeslicing=false).
The idea is that we will usually use this in conjunction with collectContinuously=true
to find race conditions during marking, but we can also use the two options
independently to focus our testing on other things.

* heap/Heap.cpp:
(JSC::Heap::markToFixpoint):
* heap/SlotVisitor.cpp:
(JSC::SlotVisitor::drainInParallel): We should have added this helper ages ago.
* heap/SlotVisitor.h:
* runtime/Options.h:

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

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

index d94145b..1aa4c20 100644 (file)
@@ -1,5 +1,33 @@
 2016-11-15  Filip Pizlo  <fpizlo@apple.com>
 
+        It should be possible to disable concurrent GC timeslicing
+        https://bugs.webkit.org/show_bug.cgi?id=164788
+
+        Reviewed by Saam Barati.
+        
+        Collector timeslicing means that the collector will try to pause once every 2ms. This is
+        great because it throttles the mutator and prevents it from outpacing the collector. But
+        it reduces some of the efficacy of the collectContinuously=true configuration: while
+        it's great that collecting continuously means that the collector will also pause more
+        frequently and so it will test the pausing code, it also means that the collector will
+        spend less time running concurrently. The primary purpose of collectContinuously is to
+        maximize the amount of time that the collector is running concurrently to the mutator to
+        maximize the likelihood that a race will cause a detectable error.
+        
+        This adds an option to disable collector timeslicing (useCollectorTimeslicing=false).
+        The idea is that we will usually use this in conjunction with collectContinuously=true
+        to find race conditions during marking, but we can also use the two options
+        independently to focus our testing on other things.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::markToFixpoint):
+        * heap/SlotVisitor.cpp:
+        (JSC::SlotVisitor::drainInParallel): We should have added this helper ages ago.
+        * heap/SlotVisitor.h:
+        * runtime/Options.h:
+
+2016-11-15  Filip Pizlo  <fpizlo@apple.com>
+
         The concurrent GC should have a timeslicing controller
         https://bugs.webkit.org/show_bug.cgi?id=164783
 
index 41d6cff..9611892 100644 (file)
@@ -669,21 +669,23 @@ void Heap::markToFixpoint(double gcStartTime)
         {
             TimingScope traceTimingScope(*this, "Heap::markToFixpoint tracing");
             ParallelModeEnabler enabler(*m_collectorSlotVisitor);
-            for (;;) {
-                MonotonicTime now = MonotonicTime::now();
-                SlotVisitor::SharedDrainResult drainResult;
-                if (shouldBeResumed(now)) {
-                    ResumeTheWorldScope resumeTheWorldScope(*this);
-                    m_collectorSlotVisitor->donateAndDrain(timeToStop(now));
-                    drainResult = m_collectorSlotVisitor->drainFromShared(
-                        SlotVisitor::MasterDrain, timeToStop(now));
-                } else {
-                    m_collectorSlotVisitor->donateAndDrain(timeToResume(now));
-                    drainResult = m_collectorSlotVisitor->drainFromShared(
-                        SlotVisitor::MasterDrain, timeToResume(now));
+            if (Options::useCollectorTimeslicing()) {
+                for (;;) {
+                    MonotonicTime now = MonotonicTime::now();
+                    SlotVisitor::SharedDrainResult drainResult;
+                    if (shouldBeResumed(now)) {
+                        ResumeTheWorldScope resumeTheWorldScope(*this);
+                        drainResult = m_collectorSlotVisitor->drainInParallel(timeToStop(now));
+                    } else
+                        drainResult = m_collectorSlotVisitor->drainInParallel(timeToResume(now));
+                    if (drainResult == SlotVisitor::SharedDrainResult::Done)
+                        break;
                 }
-                if (drainResult == SlotVisitor::SharedDrainResult::Done)
-                    break;
+            } else {
+                // Disabling collector timeslicing is meant to be used together with
+                // --collectContinuously=true to maximize the opportunity for harmful races.
+                ResumeTheWorldScope resumeTheWorldScope(*this);
+                m_collectorSlotVisitor->drainInParallel();
             }
         }
     }
index 8b6d357..eedbb64 100644 (file)
@@ -473,6 +473,12 @@ SlotVisitor::SharedDrainResult SlotVisitor::drainFromShared(SharedDrainMode shar
     }
 }
 
+SlotVisitor::SharedDrainResult SlotVisitor::drainInParallel(MonotonicTime timeout)
+{
+    donateAndDrain(timeout);
+    return drainFromShared(MasterDrain, timeout);
+}
+
 void SlotVisitor::addOpaqueRoot(void* root)
 {
     if (Options::numberOfGCMarkers() == 1) {
index 1715177..faa70bd 100644 (file)
@@ -105,6 +105,8 @@ public:
     enum class SharedDrainResult { Done, TimedOut };
     SharedDrainResult drainFromShared(SharedDrainMode, MonotonicTime timeout = MonotonicTime::infinity());
 
+    SharedDrainResult drainInParallel(MonotonicTime timeout = MonotonicTime::infinity());
+
     void harvestWeakReferences();
     void finalizeUnconditionalFinalizers();
     
index 4b26fb8..7c3e941 100644 (file)
@@ -194,6 +194,7 @@ typedef const char* optionString;
     v(double, mediumHeapRAMFraction, 0.5, Normal, nullptr) \
     v(double, mediumHeapGrowthFactor, 1.5, Normal, nullptr) \
     v(double, largeHeapGrowthFactor, 1.24, Normal, nullptr) \
+    v(bool, useCollectorTimeslicing, true, Normal, nullptr) \
     v(double, concurrentGCHeadroomRatio, 1.5, Normal, nullptr) \
     v(double, concurrentGCPeriodMS, 2, Normal, nullptr) \
     v(bool, scribbleFreeCells, false, Normal, nullptr) \