Diff
Modified: branches/safari-603-branch/Source/_javascript_Core/ChangeLog (211098 => 211099)
--- branches/safari-603-branch/Source/_javascript_Core/ChangeLog 2017-01-24 21:22:23 UTC (rev 211098)
+++ branches/safari-603-branch/Source/_javascript_Core/ChangeLog 2017-01-24 21:22:27 UTC (rev 211099)
@@ -1,3 +1,43 @@
+2017-01-24 Matthew Hanson <matthew_han...@apple.com>
+
+ Merge r210971. rdar://problem/30115838
+
+ 2017-01-20 Saam Barati <sbar...@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 Matthew Hanson <matthew_han...@apple.com>
Merge r210949. rdar://problem/30108531
Modified: branches/safari-603-branch/Source/_javascript_Core/dfg/DFGPlan.cpp (211098 => 211099)
--- branches/safari-603-branch/Source/_javascript_Core/dfg/DFGPlan.cpp 2017-01-24 21:22:23 UTC (rev 211098)
+++ branches/safari-603-branch/Source/_javascript_Core/dfg/DFGPlan.cpp 2017-01-24 21:22:27 UTC (rev 211099)
@@ -254,6 +254,22 @@
}
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
// in the CodeBlock. This is a good time to perform an early shrink, which is more
@@ -269,16 +285,16 @@
dfg.dump();
}
- performLiveCatchVariablePreservationPhase(dfg);
+ RUN_PHASE(performLiveCatchVariablePreservationPhase);
if (Options::useMaximalFlushInsertionPhase())
- performMaximalFlushInsertion(dfg);
+ RUN_PHASE(performMaximalFlushInsertion);
- performCPSRethreading(dfg);
- performUnification(dfg);
- performPredictionInjection(dfg);
+ RUN_PHASE(performCPSRethreading);
+ RUN_PHASE(performUnification);
+ RUN_PHASE(performPredictionInjection);
- performStaticExecutionCountEstimation(dfg);
+ RUN_PHASE(performStaticExecutionCountEstimation);
if (mode == FTLForOSREntryMode) {
bool result = performOSREntrypointCreation(dfg);
@@ -286,18 +302,18 @@
finalizer = std::make_unique<FailedFinalizer>(*this);
return FailPath;
}
- performCPSRethreading(dfg);
+ RUN_PHASE(performCPSRethreading);
}
if (validationEnabled())
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;
@@ -309,18 +325,18 @@
if (validationEnabled())
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
// in a non-varargs inlined call frame, this will generate ForwardVarargs while the FTL
@@ -341,11 +357,11 @@
// ArgumentsEliminationPhase does everything that this phase does, and it doesn't introduce this
// 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);
}
// If we're doing validation, then run some analyses, to give them an opportunity
@@ -360,17 +376,17 @@
case DFGMode: {
dfg.m_fixpointState = FixpointConverged;
- performTierUpCheckInjection(dfg);
+ RUN_PHASE(performTierUpCheckInjection);
- performFastStoreBarrierInsertion(dfg);
- performStoreBarrierClustering(dfg);
- performCleanUp(dfg);
- performCPSRethreading(dfg);
- performDCE(dfg);
- performPhantomInsertion(dfg);
- performStackLayout(dfg);
- performVirtualRegisterAllocation(dfg);
- performWatchpointCollection(dfg);
+ 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:");
JITCompiler dataFlowJIT(dfg);
@@ -390,37 +406,37 @@
return FailPath;
}
- 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);
+ RUN_PHASE(performPutStackSinking);
- performConstantHoisting(dfg);
- performGlobalCSE(dfg);
- performLivenessAnalysis(dfg);
- performCFA(dfg);
- performConstantFolding(dfg);
- performCleanUp(dfg); // Reduce the graph size a lot.
+ 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);
}
// Currently, this relies on pre-headers still being valid. That precludes running CFG
@@ -428,9 +444,9 @@
// wrong with running LICM earlier, if we wanted to put other CFG transforms above this point.
// 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.
//
@@ -439,29 +455,29 @@
// by IntegerRangeOptimization.
//
// Ideally, the dependencies should be explicit. See https://bugs.webkit.org/show_bug.cgi?id=157534.
- performLivenessAnalysis(dfg);
- performIntegerRangeOptimization(dfg);
+ RUN_PHASE(performLivenessAnalysis);
+ RUN_PHASE(performIntegerRangeOptimization);
- performCleanUp(dfg);
- performIntegerCheckCombining(dfg);
- performGlobalCSE(dfg);
+ 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
// about code motion assumes that it's OK to insert GC points in random places.
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) {
finalizer = std::make_unique<FailedFinalizer>(*this);
@@ -521,6 +537,8 @@
RELEASE_ASSERT_NOT_REACHED();
return FailPath;
}
+
+#undef RUN_PHASE
}
bool Plan::isStillValid()
Modified: branches/safari-603-branch/Source/_javascript_Core/dfg/DFGSafepoint.cpp (211098 => 211099)
--- branches/safari-603-branch/Source/_javascript_Core/dfg/DFGSafepoint.cpp 2017-01-24 21:22:23 UTC (rev 211098)
+++ branches/safari-603-branch/Source/_javascript_Core/dfg/DFGSafepoint.cpp 2017-01-24 21:22:27 UTC (rev 211099)
@@ -1,5 +1,5 @@
/*
- * 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
* modification, are permitted provided that the following conditions
@@ -80,7 +80,7 @@
if (ThreadData* data = "" {
RELEASE_ASSERT(!data->m_safepoint);
data->m_safepoint = this;
- data->m_rightToRun.unlock();
+ data->m_rightToRun.unlockFairly();
}
}
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/Options.h (211098 => 211099)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/Options.h 2017-01-24 21:22:23 UTC (rev 211098)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/Options.h 2017-01-24 21:22:27 UTC (rev 211099)
@@ -158,6 +158,7 @@
v(bool, dumpB3GraphAtEachPhase, false, Normal, "dumps the B3 graph at each phase of compilation") \
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) \
v(bool, logCompilationChanges, false, Normal, nullptr) \
