Sometimes, the DFG uses a GetById for typed array length accesses despite profiling...

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):

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

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 62a6bde..70757bd 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,20 @@
+2013-08-15  Filip Pizlo  <fpizlo@apple.com>
+
+        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  <mark.lam@apple.com>
 
         Fix crash when performing activation tearoff.

diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
index 82ee293..d649356 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
@@ -725,7 +725,8 @@ private:
             break;
         }
             
-        case GetById: {
+        case GetById:
+        case GetByIdFlush: {
             if (!node->child1()->shouldSpeculateCell())
                 break;
             setUseKindAndUnboxIfProfitable<CellUse>(node->child1());
@@ -741,9 +742,19 @@ private:
                 ConcurrentJITLocker locker(profiledBlock->m_lock);
                 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) {
                 // Check if the input is something that we can't get array length for, but for which we
@@ -771,12 +782,6 @@ private:
             break;
         }
             
-        case GetByIdFlush: {
-            if (node->child1()->shouldSpeculateCell())
-                setUseKindAndUnboxIfProfitable<CellUse>(node->child1());
-            break;
-        }
-            
         case CheckExecutable:
         case CheckStructure:
         case StructureTransitionWatchpoint: