Changeset 198296 in webkit for trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

Timestamp:

Mar 16, 2016, 1:12:27 PM (9 years ago)

Author:

[email protected]

Message:

ASSERTION FAILED: !edge->isPhantomAllocation() in regress/script-tests/sink-huge-activation.js.ftl-eager in debug mode
​https://bugs.webkit.org/show_bug.cgi?id=153805

Reviewed by Mark Lam.

The object allocation sinking phase uses InferredValue::isStillValid() in the opposite
way from most clients: it will do an *extra* optimization if it returns false. The
phase will first compute sink candidates and then it will compute materialization
points. If something is a sink candidate then it is not a materialization point. A
NewFunction node may appear as not being a sink candidate during the first pass, so it's
not added to the set of things that will turn into PhantomNewFunction. But on the second
pass where we add materializations, we check isStillValid() again. Now this may become
false, so that second pass thinks that NewFunction is a sink candidate (even though it's
not in the sink candidates set) and so is not a materialization point.

This manifests as the NewFunction referring to a PhantomCreateActivation or whatever.

The solution is to have the phase cache results of calls to isStillValid(). It's OK if
we just remember the result of the first call and assume that it's not a sink candidate.
That's the worst that can happen.

No new tests since this is a super hard race and sink-huge-activation seemed to already
be catching it.

• dfg/DFGObjectAllocationSinkingPhase.cpp:

File:

• trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp (modified) (4 diffs)

trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

@@ -839 +839 @@
         case NewArrowFunction:
         case NewGeneratorFunction: {
-            if (node->castOperand<FunctionExecutable*>()->singletonFunction()->isStillValid()) {
+            if (isStillValid(node->castOperand<FunctionExecutable*>()->singletonFunction())) {
                 m_heap.escape(node->child1().node());
                 break;
@@ -856 +856 @@
 
         case CreateActivation: {
-            if (node->castOperand<SymbolTable*>()->singletonScope()->isStillValid()) {
+            if (isStillValid(node->castOperand<SymbolTable*>()->singletonScope())) {
                 m_heap.escape(node->child1().node());
                 break;
@@ -2174 +2174 @@
     }
 
+    // This is a great way of asking value->isStillValid() without having to worry about getting
+    // different answers. It turns out that this analysis works OK regardless of what this
+    // returns but breaks badly if this changes its mind for any particular InferredValue. This
+    // method protects us from that.
+    bool isStillValid(InferredValue* value)
+    {
+        return m_validInferredValues.add(value, value->isStillValid()).iterator->value;
+    }
+
     SSACalculator m_pointerSSA;
     SSACalculator m_allocationSSA;
@@ -2184 +2193 @@
     CombinedLiveness m_combinedLiveness;
 
+    HashMap<InferredValue*, bool> m_validInferredValues;
+
     HashMap<Node*, Node*> m_materializationToEscapee;
     HashMap<Node*, Vector<Node*>> m_materializationSiteToMaterializations;