Changeset 210971 in webkit

Timestamp:

Jan 20, 2017, 10:10:55 AM (10 years ago)

Author:

sbarati@apple.com

Message:

We should flash a safepoint before each DFG/FTL phase
​https://bugs.webkit.org/show_bug.cgi?id=167234

Reviewed by Filip Pizlo.

The recent GC changes caused us to regress Kraken because of a
longstanding issue that happened to be hit with higher frequency because
of a change in timing between when a particular GC was happening and
when a particular FTL compilation was happening. The regression is caused
by the GC was waiting for a large function to make it through the DFG portion
of an FTL compilation. This was taking 20ms-30ms and started happened during a
particular test with much higher frequency.

This means that anytime the GC waits for this compilation, the test ran at least
~20ms slower because the GC waits for the compiler threads the mutator is stopped.

It's good that we have such an easily reproducible case of this performance
issue because it will effect many real JS programs, especially ones with
large functions that get hot.

The most straight forward solution to fix this is to flash a safepoint before
each phase, allowing the GC to suspend the compiler if needed. In my testing,
this progresses Kraken in the browser, and doesn't regress anything else. This
solution also makes the most sense. I did some analysis on the compilation time
of this function that took ~20-30ms to pass through the DFG phases, and
the phase times were mostly evenly distributed. Some took longer than others,
but no phase was longer than 3ms. Most were in the 0.25ms to 1.5ms range.

• dfg/DFGPlan.cpp:

(JSC::DFG::Plan::compileInThreadImpl):

• dfg/DFGSafepoint.cpp:

(JSC::DFG::Safepoint::begin):

• runtime/Options.h:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGPlan.cpp (modified) (12 diffs)
• dfg/DFGSafepoint.cpp (modified) (2 diffs)
• runtime/Options.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2017-01-20  Saam Barati  <sbarati@apple.com>
+
+        We should flash a safepoint before each DFG/FTL phase
+        https://bugs.webkit.org/show_bug.cgi?id=167234
+
+        Reviewed by Filip Pizlo.
+
+        The recent GC changes caused us to regress Kraken because of a
+        longstanding issue that happened to be hit with higher frequency because
+        of a change in timing between when a particular GC was happening and 
+        when a particular FTL compilation was happening. The regression is caused
+        by the GC was waiting for a large function to make it through the DFG portion
+        of an FTL compilation. This was taking 20ms-30ms and started happened during a
+        particular test with much higher frequency.
+        
+        This means that anytime the GC waits for this compilation, the test ran at least
+        ~20ms slower because the GC waits for the compiler threads the mutator is stopped.
+        
+        It's good that we have such an easily reproducible case of this performance
+        issue because it will effect many real JS programs, especially ones with
+        large functions that get hot.
+        
+        The most straight forward solution to fix this is to flash a safepoint before
+        each phase, allowing the GC to suspend the compiler if needed. In my testing,
+        this progresses Kraken in the browser, and doesn't regress anything else. This
+        solution also makes the most sense. I did some analysis on the compilation time
+        of this function that took ~20-30ms to pass through the DFG phases, and
+        the phase times were mostly evenly distributed. Some took longer than others,
+        but no phase was longer than 3ms. Most were in the 0.25ms to 1.5ms range.
+
+        * dfg/DFGPlan.cpp:
+        (JSC::DFG::Plan::compileInThreadImpl):
+        * dfg/DFGSafepoint.cpp:
+        (JSC::DFG::Safepoint::begin):
+        * runtime/Options.h:
+
 2017-01-20  Skachkov Oleksandr  <gskachkov@gmail.com>
 

trunk/Source/JavaScriptCore/dfg/DFGPlan.cpp

@@ -255 +255 @@
 
     codeBlock->setCalleeSaveRegisters(RegisterSet::dfgCalleeSaveRegisters());
+
+    bool changed = false;
+
+#define RUN_PHASE(phase)                                         \
+    do {                                                         \
+        if (Options::safepointBeforeEachPhase()) {               \
+            Safepoint::Result safepointResult;                   \
+            {                                                    \
+                GraphSafepoint safepoint(dfg, safepointResult);  \
+            }                                                    \
+            if (safepointResult.didGetCancelled())               \
+                return CancelPath;                               \
+        }                                                        \
+        changed |= phase(dfg);                                   \
+    } while (false);                                             \
+
     
     // By this point the DFG bytecode parser will have potentially mutated various tables
@@ -270 +286 @@
     }
 
-    performLiveCatchVariablePreservationPhase(dfg);
+    RUN_PHASE(performLiveCatchVariablePreservationPhase);
 
     if (Options::useMaximalFlushInsertionPhase())
-        performMaximalFlushInsertion(dfg);
-    
-    performCPSRethreading(dfg);
-    performUnification(dfg);
-    performPredictionInjection(dfg);
-    
-    performStaticExecutionCountEstimation(dfg);
+        RUN_PHASE(performMaximalFlushInsertion);
+    
+    RUN_PHASE(performCPSRethreading);
+    RUN_PHASE(performUnification);
+    RUN_PHASE(performPredictionInjection);
+    
+    RUN_PHASE(performStaticExecutionCountEstimation);
     
     if (mode == FTLForOSREntryMode) {
@@ -287 +303 @@
             return FailPath;
         }
-        performCPSRethreading(dfg);
+        RUN_PHASE(performCPSRethreading);
     }
     
@@ -293 +309 @@
         validate(dfg);
     
-    performBackwardsPropagation(dfg);
-    performPredictionPropagation(dfg);
-    performFixup(dfg);
-    performStructureRegistration(dfg);
-    performInvalidationPointInjection(dfg);
-    performTypeCheckHoisting(dfg);
+    RUN_PHASE(performBackwardsPropagation);
+    RUN_PHASE(performPredictionPropagation);
+    RUN_PHASE(performFixup);
+    RUN_PHASE(performStructureRegistration);
+    RUN_PHASE(performInvalidationPointInjection);
+    RUN_PHASE(performTypeCheckHoisting);
     
     dfg.m_fixpointState = FixpointNotConverged;
@@ -310 +326 @@
         validate(dfg);
         
-    performStrengthReduction(dfg);
-    performCPSRethreading(dfg);
-    performCFA(dfg);
-    performConstantFolding(dfg);
-    bool changed = false;
-    changed |= performCFGSimplification(dfg);
-    changed |= performLocalCSE(dfg);
+    RUN_PHASE(performStrengthReduction);
+    RUN_PHASE(performCPSRethreading);
+    RUN_PHASE(performCFA);
+    RUN_PHASE(performConstantFolding);
+    changed = false;
+    RUN_PHASE(performCFGSimplification);
+    RUN_PHASE(performLocalCSE);
     
     if (validationEnabled())
         validate(dfg);
     
-    performCPSRethreading(dfg);
+    RUN_PHASE(performCPSRethreading);
     if (!isFTL(mode)) {
         // Only run this if we're not FTLing, because currently for a LoadVarargs that is forwardable and
@@ -342 +358 @@
         // pathology.
         
-        changed |= performVarargsForwarding(dfg); // Do this after CFG simplification and CPS rethreading.
+        RUN_PHASE(performVarargsForwarding); // Do this after CFG simplification and CPS rethreading.
     }
     if (changed) {
-        performCFA(dfg);
-        performConstantFolding(dfg);
+        RUN_PHASE(performCFA);
+        RUN_PHASE(performConstantFolding);
     }
     
@@ -361 +377 @@
         dfg.m_fixpointState = FixpointConverged;
     
-        performTierUpCheckInjection(dfg);
-
-        performFastStoreBarrierInsertion(dfg);
-        performStoreBarrierClustering(dfg);
-        performCleanUp(dfg);
-        performCPSRethreading(dfg);
-        performDCE(dfg);
-        performPhantomInsertion(dfg);
-        performStackLayout(dfg);
-        performVirtualRegisterAllocation(dfg);
-        performWatchpointCollection(dfg);
+        RUN_PHASE(performTierUpCheckInjection);
+
+        RUN_PHASE(performFastStoreBarrierInsertion);
+        RUN_PHASE(performStoreBarrierClustering);
+        RUN_PHASE(performCleanUp);
+        RUN_PHASE(performCPSRethreading);
+        RUN_PHASE(performDCE);
+        RUN_PHASE(performPhantomInsertion);
+        RUN_PHASE(performStackLayout);
+        RUN_PHASE(performVirtualRegisterAllocation);
+        RUN_PHASE(performWatchpointCollection);
         dumpAndVerifyGraph(dfg, "Graph after optimization:");
         
@@ -391 +407 @@
         }
         
-        performCleanUp(dfg); // Reduce the graph size a bit.
-        performCriticalEdgeBreaking(dfg);
+        RUN_PHASE(performCleanUp); // Reduce the graph size a bit.
+        RUN_PHASE(performCriticalEdgeBreaking);
         if (Options::createPreHeaders())
-            performLoopPreHeaderCreation(dfg);
-        performCPSRethreading(dfg);
-        performSSAConversion(dfg);
-        performSSALowering(dfg);
+            RUN_PHASE(performLoopPreHeaderCreation);
+        RUN_PHASE(performCPSRethreading);
+        RUN_PHASE(performSSAConversion);
+        RUN_PHASE(performSSALowering);
         
         // Ideally, these would be run to fixpoint with the object allocation sinking phase.
-        performArgumentsElimination(dfg);
+        RUN_PHASE(performArgumentsElimination);
         if (Options::usePutStackSinking())
-            performPutStackSinking(dfg);
-        
-        performConstantHoisting(dfg);
-        performGlobalCSE(dfg);
-        performLivenessAnalysis(dfg);
-        performCFA(dfg);
-        performConstantFolding(dfg);
-        performCleanUp(dfg); // Reduce the graph size a lot.
+            RUN_PHASE(performPutStackSinking);
+        
+        RUN_PHASE(performConstantHoisting);
+        RUN_PHASE(performGlobalCSE);
+        RUN_PHASE(performLivenessAnalysis);
+        RUN_PHASE(performCFA);
+        RUN_PHASE(performConstantFolding);
+        RUN_PHASE(performCleanUp); // Reduce the graph size a lot.
         changed = false;
-        changed |= performStrengthReduction(dfg);
+        RUN_PHASE(performStrengthReduction);
         if (Options::useObjectAllocationSinking()) {
-            changed |= performCriticalEdgeBreaking(dfg);
-            changed |= performObjectAllocationSinking(dfg);
+            RUN_PHASE(performCriticalEdgeBreaking);
+            RUN_PHASE(performObjectAllocationSinking);
         }
         if (changed) {
             // State-at-tail and state-at-head will be invalid if we did strength reduction since
             // it might increase live ranges.
-            performLivenessAnalysis(dfg);
-            performCFA(dfg);
-            performConstantFolding(dfg);
+            RUN_PHASE(performLivenessAnalysis);
+            RUN_PHASE(performCFA);
+            RUN_PHASE(performConstantFolding);
         }
         
@@ -429 +445 @@
         // Alternatively, we could run loop pre-header creation after SSA conversion - but if we did that
         // then we'd need to do some simple SSA fix-up.
-        performLivenessAnalysis(dfg);
-        performCFA(dfg);
-        performLICM(dfg);
+        RUN_PHASE(performLivenessAnalysis);
+        RUN_PHASE(performCFA);
+        RUN_PHASE(performLICM);
 
         // FIXME: Currently: IntegerRangeOptimization *must* be run after LICM.
@@ -440 +456 @@
         //
         // Ideally, the dependencies should be explicit. See https://bugs.webkit.org/show_bug.cgi?id=157534.
-        performLivenessAnalysis(dfg);
-        performIntegerRangeOptimization(dfg);
-        
-        performCleanUp(dfg);
-        performIntegerCheckCombining(dfg);
-        performGlobalCSE(dfg);
+        RUN_PHASE(performLivenessAnalysis);
+        RUN_PHASE(performIntegerRangeOptimization);
+        
+        RUN_PHASE(performCleanUp);
+        RUN_PHASE(performIntegerCheckCombining);
+        RUN_PHASE(performGlobalCSE);
         
         // At this point we're not allowed to do any further code motion because our reasoning
@@ -451 +467 @@
         dfg.m_fixpointState = FixpointConverged;
         
-        performLivenessAnalysis(dfg);
-        performCFA(dfg);
-        performGlobalStoreBarrierInsertion(dfg);
-        performStoreBarrierClustering(dfg);
+        RUN_PHASE(performLivenessAnalysis);
+        RUN_PHASE(performCFA);
+        RUN_PHASE(performGlobalStoreBarrierInsertion);
+        RUN_PHASE(performStoreBarrierClustering);
         if (Options::useMovHintRemoval())
-            performMovHintRemoval(dfg);
-        performCleanUp(dfg);
-        performDCE(dfg); // We rely on this to kill dead code that won't be recognized as dead by B3.
-        performStackLayout(dfg);
-        performLivenessAnalysis(dfg);
-        performOSRAvailabilityAnalysis(dfg);
-        performWatchpointCollection(dfg);
+            RUN_PHASE(performMovHintRemoval);
+        RUN_PHASE(performCleanUp);
+        RUN_PHASE(performDCE); // We rely on this to kill dead code that won't be recognized as dead by B3.
+        RUN_PHASE(performStackLayout);
+        RUN_PHASE(performLivenessAnalysis);
+        RUN_PHASE(performOSRAvailabilityAnalysis);
+        RUN_PHASE(performWatchpointCollection);
         
         if (FTL::canCompile(dfg) == FTL::CannotCompile) {
@@ -522 +538 @@
         return FailPath;
     }
+
+#undef RUN_PHASE
 }
 

trunk/Source/JavaScriptCore/dfg/DFGSafepoint.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2014, 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -81 +81 @@
         RELEASE_ASSERT(!data->m_safepoint);
         data->m_safepoint = this;
-        data->m_rightToRun.unlock();
+        data->m_rightToRun.unlockFairly();
     }
 }

trunk/Source/JavaScriptCore/runtime/Options.h

@@ -159 +159 @@
     v(bool, dumpAirGraphAtEachPhase, false, Normal, "dumps the Air graph at each phase of compilation") \
     v(bool, verboseDFGByteCodeParsing, false, Normal, nullptr) \
+    v(bool, safepointBeforeEachPhase, true, Normal, nullptr) \
     v(bool, verboseCompilation, false, Normal, nullptr) \
     v(bool, verboseFTLCompilation, false, Normal, nullptr) \