- Revision
- 202402
- Author
- [email protected]
- Date
- 2016-06-23 16:28:50 -0700 (Thu, 23 Jun 2016)
Log Message
DFGSpeculativeJIT's m_slowPathLambdas should restore the current node field and DFG OSR entry functions should use DeferGCForAWhile instead of DeferGC
https://bugs.webkit.org/show_bug.cgi?id=159064
<rdar://problem/26599119>
Reviewed by Filip Pizlo.
The DFG has a list of m_slowPathLambdas that are code generators it emits
amongst its slow paths. These lambdas, however, did not update the m_currentNode field.
This caused us to use whatever Node happened to be used as the currentNode at the time
we call the slowPathLambda. This means the wrong CallSiteIndex was stored into the call
frame when we made a call. This may lead to a crash if the CallSiteIndex corresponds to
the wrong CodeOrigin. For example, the wrong CodeOrigin could have an InlineCallFrame with
a calleeRecovery that will not be in sync with the current stack state. Trying
to recover that callee will often lead to a crash. The solution is to update
m_currentNode to the DFG::Node* it corresponds to when emitting these slowPathLambdas.
I found this bug because we were inside this bad state when calling an operation
that happened to have a DeferGC. When this DeferGC actually GCed, it would
take a StackTrace, which would lead to a crash because we were updating
ShadowChicken with vm.topCallFrame. It just so happened that the CallSiteIndex
in the call frame in this program corresponded to an InlineCallFrame with a calleeRecover.
However, this CallSiteIndex didn't correspond to the actual state of execution
of the program. I'm adding new options to make reproducing DeferGC related
bugs easier by making DeferGC force a GC according to some probability. To
always have DeferGC force a GC, you can set that probability to 1.
There is a second bug that I discovered after solving the above bug. We were
using DeferGC instead of DeferGCForAWhile in the OSR entry related functions
in the DFG. This would cause us to take a stack trace when the call frame was
in an inconsistent state. For example, the operation would call FTL::prepareOSREntry,
which would replace the DFG CodeBlock in the call frame with the FTL CodeBlock.
However, we wouldn't update the CallSiteIndex to correspond to an FTL CallSiteIndex.
This would lead to a crash when taking a stack trace. The solution is to prevent
stack traces from being taken when the program is in this state by using
DeferGCForAWhie instead of DeferGC.
* dfg/DFGOperations.cpp:
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::addSlowPathGenerator):
(JSC::DFG::SpeculativeJIT::runSlowPathGenerators):
* dfg/DFGSpeculativeJIT.h:
* heap/Heap.h:
* heap/HeapInlines.h:
(JSC::Heap::collectIfNecessaryOrDefer):
(JSC::Heap::collectAccordingToDeferGCProbability):
(JSC::Heap::decrementDeferralDepthAndGCIfNeeded):
(JSC::Heap::markListSet):
* runtime/Options.cpp:
(JSC::recomputeDependentOptions):
(JSC::Options::initialize):
* runtime/Options.h:
* tests/stress/slow-path-generator-updating-current-node-dfg.js: Added.
(foo):
(bar):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (202401 => 202402)
--- trunk/Source/_javascript_Core/ChangeLog 2016-06-23 23:22:21 UTC (rev 202401)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-06-23 23:28:50 UTC (rev 202402)
@@ -1,3 +1,60 @@
+2016-06-23 Saam Barati <[email protected]>
+
+ DFGSpeculativeJIT's m_slowPathLambdas should restore the current node field and DFG OSR entry functions should use DeferGCForAWhile instead of DeferGC
+ https://bugs.webkit.org/show_bug.cgi?id=159064
+ <rdar://problem/26599119>
+
+ Reviewed by Filip Pizlo.
+
+ The DFG has a list of m_slowPathLambdas that are code generators it emits
+ amongst its slow paths. These lambdas, however, did not update the m_currentNode field.
+ This caused us to use whatever Node happened to be used as the currentNode at the time
+ we call the slowPathLambda. This means the wrong CallSiteIndex was stored into the call
+ frame when we made a call. This may lead to a crash if the CallSiteIndex corresponds to
+ the wrong CodeOrigin. For example, the wrong CodeOrigin could have an InlineCallFrame with
+ a calleeRecovery that will not be in sync with the current stack state. Trying
+ to recover that callee will often lead to a crash. The solution is to update
+ m_currentNode to the DFG::Node* it corresponds to when emitting these slowPathLambdas.
+
+ I found this bug because we were inside this bad state when calling an operation
+ that happened to have a DeferGC. When this DeferGC actually GCed, it would
+ take a StackTrace, which would lead to a crash because we were updating
+ ShadowChicken with vm.topCallFrame. It just so happened that the CallSiteIndex
+ in the call frame in this program corresponded to an InlineCallFrame with a calleeRecover.
+ However, this CallSiteIndex didn't correspond to the actual state of execution
+ of the program. I'm adding new options to make reproducing DeferGC related
+ bugs easier by making DeferGC force a GC according to some probability. To
+ always have DeferGC force a GC, you can set that probability to 1.
+
+ There is a second bug that I discovered after solving the above bug. We were
+ using DeferGC instead of DeferGCForAWhile in the OSR entry related functions
+ in the DFG. This would cause us to take a stack trace when the call frame was
+ in an inconsistent state. For example, the operation would call FTL::prepareOSREntry,
+ which would replace the DFG CodeBlock in the call frame with the FTL CodeBlock.
+ However, we wouldn't update the CallSiteIndex to correspond to an FTL CallSiteIndex.
+ This would lead to a crash when taking a stack trace. The solution is to prevent
+ stack traces from being taken when the program is in this state by using
+ DeferGCForAWhie instead of DeferGC.
+
+ * dfg/DFGOperations.cpp:
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::addSlowPathGenerator):
+ (JSC::DFG::SpeculativeJIT::runSlowPathGenerators):
+ * dfg/DFGSpeculativeJIT.h:
+ * heap/Heap.h:
+ * heap/HeapInlines.h:
+ (JSC::Heap::collectIfNecessaryOrDefer):
+ (JSC::Heap::collectAccordingToDeferGCProbability):
+ (JSC::Heap::decrementDeferralDepthAndGCIfNeeded):
+ (JSC::Heap::markListSet):
+ * runtime/Options.cpp:
+ (JSC::recomputeDependentOptions):
+ (JSC::Options::initialize):
+ * runtime/Options.h:
+ * tests/stress/slow-path-generator-updating-current-node-dfg.js: Added.
+ (foo):
+ (bar):
+
2016-06-23 Filip Pizlo <[email protected]>
Failing baseline JIT compilation of a code block and then trying to compile it from OSR from DFG/FTL will corrupt the CodeBlock
Modified: trunk/Source/_javascript_Core/dfg/DFGOperations.cpp (202401 => 202402)
--- trunk/Source/_javascript_Core/dfg/DFGOperations.cpp 2016-06-23 23:22:21 UTC (rev 202401)
+++ trunk/Source/_javascript_Core/dfg/DFGOperations.cpp 2016-06-23 23:28:50 UTC (rev 202402)
@@ -1818,7 +1818,7 @@
{
VM* vm = &exec->vm();
NativeCallFrameTracer tracer(vm, exec);
- DeferGC deferGC(vm->heap);
+ DeferGCForAWhile deferGC(vm->heap);
CodeBlock* codeBlock = exec->codeBlock();
if (codeBlock->jitType() != JITCode::DFGJIT) {
@@ -2000,7 +2000,7 @@
{
VM* vm = &exec->vm();
NativeCallFrameTracer tracer(vm, exec);
- DeferGC deferGC(vm->heap);
+ DeferGCForAWhile deferGC(vm->heap);
CodeBlock* codeBlock = exec->codeBlock();
if (codeBlock->jitType() != JITCode::DFGJIT) {
@@ -2034,7 +2034,7 @@
{
VM* vm = &exec->vm();
NativeCallFrameTracer tracer(vm, exec);
- DeferGC deferGC(vm->heap);
+ DeferGCForAWhile deferGC(vm->heap);
CodeBlock* codeBlock = exec->codeBlock();
if (codeBlock->jitType() != JITCode::DFGJIT) {
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (202401 => 202402)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2016-06-23 23:22:21 UTC (rev 202401)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2016-06-23 23:28:50 UTC (rev 202402)
@@ -362,7 +362,7 @@
void SpeculativeJIT::addSlowPathGenerator(std::function<void()> lambda)
{
- m_slowPathLambdas.append(std::make_pair(lambda, m_origin.semantic));
+ m_slowPathLambdas.append(std::make_pair(lambda, m_currentNode));
}
void SpeculativeJIT::runSlowPathGenerators(PCToCodeOriginMapBuilder& pcToCodeOriginMapBuilder)
@@ -372,7 +372,9 @@
slowPathGenerator->generate(this);
}
for (auto& generatorPair : m_slowPathLambdas) {
- pcToCodeOriginMapBuilder.appendItem(m_jit.label(), generatorPair.second);
+ Node* currentNode = generatorPair.second;
+ m_currentNode = currentNode;
+ pcToCodeOriginMapBuilder.appendItem(m_jit.label(), currentNode->origin.semantic);
generatorPair.first();
}
}
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.h (202401 => 202402)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.h 2016-06-23 23:22:21 UTC (rev 202401)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.h 2016-06-23 23:28:50 UTC (rev 202402)
@@ -2829,7 +2829,7 @@
MinifiedGraph* m_minifiedGraph;
Vector<std::unique_ptr<SlowPathGenerator>, 8> m_slowPathGenerators;
- Vector<std::pair<std::function<void()>, CodeOrigin>, 8> m_slowPathLambdas;
+ Vector<std::pair<std::function<void()>, Node*>, 8> m_slowPathLambdas;
Vector<SilentRegisterSavePlan> m_plans;
unsigned m_outOfLineStreamIndex { UINT_MAX };
};
Modified: trunk/Source/_javascript_Core/heap/Heap.h (202401 => 202402)
--- trunk/Source/_javascript_Core/heap/Heap.h 2016-06-23 23:22:21 UTC (rev 202401)
+++ trunk/Source/_javascript_Core/heap/Heap.h 2016-06-23 23:28:50 UTC (rev 202402)
@@ -170,6 +170,7 @@
bool shouldCollect();
JS_EXPORT_PRIVATE void collect(HeapOperation collectionType = AnyCollection);
bool collectIfNecessaryOrDefer(); // Returns true if it did collect.
+ void collectAccordingToDeferGCProbability();
void completeAllJITPlans();
Modified: trunk/Source/_javascript_Core/heap/HeapInlines.h (202401 => 202402)
--- trunk/Source/_javascript_Core/heap/HeapInlines.h 2016-06-23 23:22:21 UTC (rev 202401)
+++ trunk/Source/_javascript_Core/heap/HeapInlines.h 2016-06-23 23:28:50 UTC (rev 202402)
@@ -32,6 +32,7 @@
#include "Structure.h"
#include <type_traits>
#include <wtf/Assertions.h>
+#include <wtf/RandomNumber.h>
namespace JSC {
@@ -302,10 +303,28 @@
return true;
}
+inline void Heap::collectAccordingToDeferGCProbability()
+{
+ if (isDeferred() || !m_isSafeToCollect || m_operationInProgress != NoOperation)
+ return;
+
+ if (randomNumber() < Options::deferGCProbability()) {
+ collect();
+ return;
+ }
+
+ // If our coin flip told us not to GC, we still might GC,
+ // but we GC according to our memory pressure markers.
+ collectIfNecessaryOrDefer();
+}
+
inline void Heap::decrementDeferralDepthAndGCIfNeeded()
{
decrementDeferralDepth();
- collectIfNecessaryOrDefer();
+ if (UNLIKELY(Options::deferGCShouldCollectWithProbability()))
+ collectAccordingToDeferGCProbability();
+ else
+ collectIfNecessaryOrDefer();
}
inline HashSet<MarkedArgumentBuffer*>& Heap::markListSet()
Modified: trunk/Source/_javascript_Core/runtime/Options.cpp (202401 => 202402)
--- trunk/Source/_javascript_Core/runtime/Options.cpp 2016-06-23 23:22:21 UTC (rev 202401)
+++ trunk/Source/_javascript_Core/runtime/Options.cpp 2016-06-23 23:28:50 UTC (rev 202402)
@@ -388,6 +388,7 @@
ASSERT((static_cast<int64_t>(Options::thresholdForOptimizeAfterLongWarmUp()) << Options::reoptimizationRetryCounterMax()) > 0);
ASSERT((static_cast<int64_t>(Options::thresholdForOptimizeAfterLongWarmUp()) << Options::reoptimizationRetryCounterMax()) <= static_cast<int64_t>(std::numeric_limits<int32_t>::max()));
+ ASSERT(Options::deferGCProbability() >= 0.0 && Options::deferGCProbability() <= 1.0);
}
void Options::initialize()
Modified: trunk/Source/_javascript_Core/runtime/Options.h (202401 => 202402)
--- trunk/Source/_javascript_Core/runtime/Options.h 2016-06-23 23:22:21 UTC (rev 202401)
+++ trunk/Source/_javascript_Core/runtime/Options.h 2016-06-23 23:28:50 UTC (rev 202402)
@@ -300,6 +300,8 @@
v(double, minCopiedBlockUtilization, 0.9, Normal, nullptr) \
v(double, minMarkedBlockUtilization, 0.9, Normal, nullptr) \
v(unsigned, slowPathAllocsBetweenGCs, 0, Normal, "force a GC on every Nth slow path alloc, where N is specified by this option") \
+ v(bool, deferGCShouldCollectWithProbability, false, Normal, "If true, we perform a collection based on flipping a coin according the probability in the 'deferGCProbability' option when DeferGC is destructed.") \
+ v(double, deferGCProbability, 1.0, Normal, "Should be a number between 0 and 1. 1 means DeferGC always GCs when it's destructed and GCing is safe. 0.7 means we force GC 70% the time on DeferGC destruction.") \
\
v(double, percentCPUPerMBForFullTimer, 0.0003125, Normal, nullptr) \
v(double, percentCPUPerMBForEdenTimer, 0.0025, Normal, nullptr) \
Added: trunk/Source/_javascript_Core/tests/stress/slow-path-generator-updating-current-node-dfg.js (0 => 202402)
--- trunk/Source/_javascript_Core/tests/stress/slow-path-generator-updating-current-node-dfg.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/slow-path-generator-updating-current-node-dfg.js 2016-06-23 23:28:50 UTC (rev 202402)
@@ -0,0 +1,24 @@
+//@ runMiscFTLNoCJITTest("--deferGCShouldCollectWithProbability=true", "--deferGCProbability=1.0")
+
+function foo(a) {
+ return a.push(25);
+}
+
+function bar(a) {
+ for (let i = 0; i < a.length; i++) {
+ a[i] = i;
+ }
+ return foo(a);
+}
+
+noInline(bar);
+
+for (let i = 0; i < 100; i++) {
+ let smallArray = [1, 2, 3, 4, 5];
+ bar(smallArray);
+}
+
+let largeArray = [];
+for (let i = 0; i < 10000000; i++)
+ largeArray.push(i);
+bar(largeArray);
