Having a bad time has a really awful time when it runs at the same time as the JIT
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Dec 2015 22:25:26 +0000 (22:25 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Dec 2015 22:25:26 +0000 (22:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=151882
rdar://problem/23547038

Reviewed by Geoffrey Garen.

The DFG's use of watchpoints for havingABadTime goes back a long time. We introduced this feature
when we first introduced watchpoints. That left it open to a lot of bitrot. On top of that, this
code doesn't get tested much because having a bad time is not something that is really supposed to
happen.

Well, now I've got reports that it does happen - or at least, we know that it is because of
crashes in an assertion that could only be triggered if a bad time was had. In the meantime, we
added two new features without adequately testing havingABadTime: concurrent JIT and FTL.
Concurrency means that we have to worry about the havingABadTime watchpoint triggering during
compilation. FTL means that we have new code and new optimizations that needs to deal with this
feature correctly.

The bug can arise via race condition or just goofy profiling. As in the newly added test, we could
first profile an allocation thinking that it will allocate sane arrays. Then we might have a bad
time, and then compile that function with the FTL. The ByteCodeParser will represent the
allocation with a NewArray node that has a sane indexingType(). But when we go to lower the Node,
we observe that the Structure* that the JSGlobalObject tells us to use has a different indexing
type. This is a feature of havingABadTime that the DFG knew about, but the FTL didn't. The FTL
didn't know about it because we didn't have adequate tests, and this code rarely gets triggered in
the wild. So, the FTL had a silly assertion that the indexing types match. They absolutely don't
have to match.

There is another bug, a race condition, that remains even if we remove the bad assertion. We set
the havingABadTime watchpoint late in compilation, and we do it based on whether the watchpoint is
still OK. This means that we could parse a function before we have a bad time and then do
optimizations (for example in AbstractInterpreter) like proving that the structure set associated
with the value returned by the NewArray is the one with a sane indexing type. Then, after those
optimizations have already been done, we will go to set the watchpoint. But just as we are doing
this, we could haveABadTime on the main thread. Currently this sort of almost works because
having a bad time requires doing a GC, and we can't GC until the watchpoint collection phase. But
that feels too fragile. So, this phase moves the setting of the watchpoint to the FixupPhase. This
is consistent with our long-term goal of removing the WatchpointCollectionPhase. Moving this to
FixupPhase means that we set the watchpoint before doing any optimizations. So, if having a bad
time happens before the FixupPhase then all optimizations will agree that we're having a bad time
and so everything is fine; if we have a bad time after FixupPhase then we will cancel the
compilation anyway.

* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleConstantInternalFunction):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::watchHavingABadTime):
(JSC::DFG::FixupPhase::createToString):
* dfg/DFGNode.h:
(JSC::DFG::Node::hasIndexingType):
(JSC::DFG::Node::indexingType):
* dfg/DFGWatchpointCollectionPhase.cpp:
(JSC::DFG::WatchpointCollectionPhase::handle):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::compileNewArray):
(JSC::FTL::DFG::LowerDFGToLLVM::compileNewArrayBuffer):
* tests/stress/ftl-has-a-bad-time.js: Added.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGNode.h
Source/JavaScriptCore/dfg/DFGWatchpointCollectionPhase.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp

index fb2abaa..4432237 100644 (file)
@@ -1,3 +1,64 @@
+2015-12-04  Filip Pizlo  <fpizlo@apple.com>
+
+        Having a bad time has a really awful time when it runs at the same time as the JIT
+        https://bugs.webkit.org/show_bug.cgi?id=151882
+        rdar://problem/23547038
+
+        Reviewed by Geoffrey Garen.
+
+        The DFG's use of watchpoints for havingABadTime goes back a long time. We introduced this feature
+        when we first introduced watchpoints. That left it open to a lot of bitrot. On top of that, this
+        code doesn't get tested much because having a bad time is not something that is really supposed to
+        happen.
+
+        Well, now I've got reports that it does happen - or at least, we know that it is because of
+        crashes in an assertion that could only be triggered if a bad time was had. In the meantime, we
+        added two new features without adequately testing havingABadTime: concurrent JIT and FTL.
+        Concurrency means that we have to worry about the havingABadTime watchpoint triggering during
+        compilation. FTL means that we have new code and new optimizations that needs to deal with this
+        feature correctly.
+
+        The bug can arise via race condition or just goofy profiling. As in the newly added test, we could
+        first profile an allocation thinking that it will allocate sane arrays. Then we might have a bad
+        time, and then compile that function with the FTL. The ByteCodeParser will represent the
+        allocation with a NewArray node that has a sane indexingType(). But when we go to lower the Node,
+        we observe that the Structure* that the JSGlobalObject tells us to use has a different indexing
+        type. This is a feature of havingABadTime that the DFG knew about, but the FTL didn't. The FTL
+        didn't know about it because we didn't have adequate tests, and this code rarely gets triggered in
+        the wild. So, the FTL had a silly assertion that the indexing types match. They absolutely don't
+        have to match.
+
+        There is another bug, a race condition, that remains even if we remove the bad assertion. We set
+        the havingABadTime watchpoint late in compilation, and we do it based on whether the watchpoint is
+        still OK. This means that we could parse a function before we have a bad time and then do
+        optimizations (for example in AbstractInterpreter) like proving that the structure set associated
+        with the value returned by the NewArray is the one with a sane indexing type. Then, after those
+        optimizations have already been done, we will go to set the watchpoint. But just as we are doing
+        this, we could haveABadTime on the main thread. Currently this sort of almost works because
+        having a bad time requires doing a GC, and we can't GC until the watchpoint collection phase. But
+        that feels too fragile. So, this phase moves the setting of the watchpoint to the FixupPhase. This
+        is consistent with our long-term goal of removing the WatchpointCollectionPhase. Moving this to
+        FixupPhase means that we set the watchpoint before doing any optimizations. So, if having a bad
+        time happens before the FixupPhase then all optimizations will agree that we're having a bad time
+        and so everything is fine; if we have a bad time after FixupPhase then we will cancel the
+        compilation anyway.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::handleConstantInternalFunction):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        (JSC::DFG::FixupPhase::watchHavingABadTime):
+        (JSC::DFG::FixupPhase::createToString):
+        * dfg/DFGNode.h:
+        (JSC::DFG::Node::hasIndexingType):
+        (JSC::DFG::Node::indexingType):
+        * dfg/DFGWatchpointCollectionPhase.cpp:
+        (JSC::DFG::WatchpointCollectionPhase::handle):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::DFG::LowerDFGToLLVM::compileNewArray):
+        (JSC::FTL::DFG::LowerDFGToLLVM::compileNewArrayBuffer):
+        * tests/stress/ftl-has-a-bad-time.js: Added.
+
 2015-12-04  Benjamin Poulain  <bpoulain@apple.com>
 
         [JSC] Use Div and ChillDiv in FTL(B3)Output
index a2020a4..99952b7 100644 (file)
@@ -2418,13 +2418,6 @@ bool ByteCodeParser::handleConstantInternalFunction(
     if (verbose)
         dataLog("    Handling constant internal function ", JSValue(function), "\n");
     
-    // If we ever find that we have a lot of internal functions that we specialize for,
-    // then we should probably have some sort of hashtable dispatch, or maybe even
-    // dispatch straight through the MethodTable of the InternalFunction. But for now,
-    // it seems that this case is hit infrequently enough, and the number of functions
-    // we know about is small enough, that having just a linear cascade of if statements
-    // is good enough.
-    
     if (function->classInfo() == ArrayConstructor::info()) {
         if (function->globalObject() != m_inlineStackTop->m_codeBlock->globalObject())
             return false;
index 65e4927..b219452 100644 (file)
@@ -925,6 +925,8 @@ private:
         }
             
         case NewArray: {
+            watchHavingABadTime(node);
+            
             for (unsigned i = m_graph.varArgNumChildren(node); i--;) {
                 node->setIndexingType(
                     leastUpperBoundOfIndexingTypeAndType(
@@ -962,6 +964,8 @@ private:
         }
             
         case NewTypedArray: {
+            watchHavingABadTime(node);
+            
             if (node->child1()->shouldSpeculateInt32()) {
                 fixEdge<Int32Use>(node->child1());
                 node->clearFlags(NodeMustGenerate);
@@ -971,6 +975,7 @@ private:
         }
             
         case NewArrayWithSize: {
+            watchHavingABadTime(node);
             fixEdge<Int32Use>(node->child1());
             break;
         }
@@ -1450,6 +1455,20 @@ private:
 #endif
         }
     }
+
+    void watchHavingABadTime(Node* node)
+    {
+        JSGlobalObject* globalObject = m_graph.globalObjectFor(node->origin.semantic);
+
+        // If this global object is not having a bad time, watch it. We go down this path anytime the code
+        // does an array allocation. The types of array allocations may change if we start to have a bad
+        // time. It's easier to reason about this if we know that whenever the types change after we start
+        // optimizing, the code just gets thrown out. Doing this at FixupPhase is just early enough, since
+        // prior to this point nobody should have been doing optimizations based on the indexing type of
+        // the allocation.
+        if (!globalObject->isHavingABadTime())
+            m_graph.watchpoints().addLazily(globalObject->havingABadTimeWatchpoint());
+    }
     
     template<UseKind useKind>
     void createToString(Node* node, Edge& edge)
index 1aa1516..f96a7ef 100644 (file)
@@ -949,7 +949,15 @@ struct Node {
             return false;
         }
     }
-    
+
+    // Return the indexing type that an array allocation *wants* to use. It may end up using a different
+    // type if we're having a bad time. You can determine the actual indexing type by asking the global
+    // object:
+    //
+    //     m_graph.globalObjectFor(node->origin.semantic)->arrayStructureForIndexingTypeDuringAllocation(node->indexingType())
+    //
+    // This will give you a Structure*, and that will have some indexing type that may be different from
+    // the this one.
     IndexingType indexingType()
     {
         ASSERT(hasIndexingType());
index 40caac8..c967f62 100644 (file)
@@ -97,13 +97,6 @@ private:
             }
             break;
             
-        case NewArray:
-        case NewArrayWithSize:
-        case NewArrayBuffer:
-            if (!globalObject()->isHavingABadTime() && !hasAnyArrayStorage(m_node->indexingType()))
-                addLazily(globalObject()->havingABadTimeWatchpoint());
-            break;
-            
         case VarInjectionWatchpoint:
             addLazily(globalObject()->varInjectionWatchpoint());
             break;
index 1886120..442634a 100644 (file)
@@ -3763,9 +3763,7 @@ private:
         JSGlobalObject* globalObject = m_graph.globalObjectFor(m_node->origin.semantic);
         Structure* structure = globalObject->arrayStructureForIndexingTypeDuringAllocation(
             m_node->indexingType());
-        
-        DFG_ASSERT(m_graph, m_node, structure->indexingType() == m_node->indexingType());
-        
+
         if (!globalObject->isHavingABadTime() && !hasAnyArrayStorage(m_node->indexingType())) {
             unsigned numElements = m_node->numChildren();
             
@@ -3842,8 +3840,6 @@ private:
         Structure* structure = globalObject->arrayStructureForIndexingTypeDuringAllocation(
             m_node->indexingType());
         
-        DFG_ASSERT(m_graph, m_node, structure->indexingType() == m_node->indexingType());
-        
         if (!globalObject->isHavingABadTime() && !hasAnyArrayStorage(m_node->indexingType())) {
             unsigned numElements = m_node->numConstants();