Changeset 154157 in webkit for trunk/Source/JavaScriptCore

Timestamp:

Aug 15, 2013, 7:30:37 PM (12 years ago)

Author:

[email protected]

Message:

Sometimes, the DFG uses a GetById for typed array length accesses despite profiling data that indicates that it's a typed array length access
​https://bugs.webkit.org/show_bug.cgi?id=119874

Reviewed by Oliver Hunt and Mark Hahnenberg.

It was a confusion between heuristics in DFG::ArrayMode that are assuming that
you'll use ForceExit if array profiles are empty, the JIT creating empty profiles
sometimes for typed array length accesses, and the FixupPhase assuming that a
ForceExit ArrayMode means that it should continue using a generic GetById.

This fixes the confusion.

• dfg/DFGFixupPhase.cpp:

(JSC::DFG::FixupPhase::fixupNode):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGFixupPhase.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2013-08-15  Filip Pizlo  <[email protected]>
+
+        Sometimes, the DFG uses a GetById for typed array length accesses despite profiling data that indicates that it's a typed array length access
+        https://bugs.webkit.org/show_bug.cgi?id=119874
+
+        Reviewed by Oliver Hunt and Mark Hahnenberg.
+        
+        It was a confusion between heuristics in DFG::ArrayMode that are assuming that
+        you'll use ForceExit if array profiles are empty, the JIT creating empty profiles
+        sometimes for typed array length accesses, and the FixupPhase assuming that a
+        ForceExit ArrayMode means that it should continue using a generic GetById.
+
+        This fixes the confusion.
+
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+
 2013-08-15  Mark Lam  <[email protected]>
 

trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

@@ -726 +726 @@
         }
             
-        case GetById: {
+        case GetById:
+        case GetByIdFlush: {
             if (!node->child1()->shouldSpeculateCell())
                 break;
@@ -742 +743 @@
                 arrayProfile->computeUpdatedPrediction(locker, profiledBlock);
                 arrayMode = ArrayMode::fromObserved(locker, arrayProfile, Array::Read, false);
-                arrayMode = arrayMode.refine(node->child1()->prediction(), node->prediction());
-            } else
-                arrayMode = arrayMode.refine(node->child1()->prediction(), node->prediction());
+                if (arrayMode.type() == Array::Unprofiled) {
+                    // For normal array operations, it makes sense to treat Unprofiled
+                    // accesses as ForceExit and get more data rather than using
+                    // predictions and then possibly ending up with a Generic. But here,
+                    // we treat anything that is Unprofiled as Generic and keep the
+                    // GetById. I.e. ForceExit = Generic. So, there is no harm - and only
+                    // profit - from treating the Unprofiled case as
+                    // SelectUsingPredictions.
+                    arrayMode = ArrayMode(Array::SelectUsingPredictions);
+                }
+            }
+            
+            arrayMode = arrayMode.refine(node->child1()->prediction(), node->prediction());
             
             if (arrayMode.type() == Array::Generic) {
@@ -769 +780 @@
             
             node->child2() = Edge(storage);
-            break;
-        }
-            
-        case GetByIdFlush: {
-            if (node->child1()->shouldSpeculateCell())
-                setUseKindAndUnboxIfProfitable<CellUse>(node->child1());
             break;
         }