Changeset 149834 in webkit

Timestamp:

May 9, 2013, 12:38:23 PM (12 years ago)

Author:

[email protected]

Message:

DFGArrayMode::fromObserved is too liberal when it sees different Array and NonArray shapes
​https://bugs.webkit.org/show_bug.cgi?id=115805

Source/JavaScriptCore:

Reviewed by Geoffrey Garen.

It checks the observed ArrayModes to see if we have seen any ArrayWith* first. If so, it assumes it's
an Array::Array, even if we've also observed any NonArrayWith* in the ArrayProfile. This leads to the
code generated by jumpSlowForUnwantedArrayMode to check the indexing type against (shape | IsArray)
instead of just shape, which can cause us to exit a lot in the case that we saw a NonArray.

To fix this we need to add a case that checks for both ArrayWith* and NonArrayWith* cases first, which
should then use Array::PossiblyArray, then do the checks we were already doing.

• bytecode/ArrayProfile.h:

(JSC::hasSeenArray):
(JSC::hasSeenNonArray):

• dfg/DFGArrayMode.cpp:

(JSC::DFG::ArrayMode::fromObserved):

LayoutTests:

Added regression test for array access over polymorphic array vs. non-array indexing types.
With the fix, we get 3.666x faster on this microbenchmark.

Reviewed by Geoffrey Garen.

• fast/js/regress/array-nonarray-polymorphic-access-expected.txt: Added.
• fast/js/regress/array-nonarray-polymorphic-access.html: Added.
• fast/js/regress/script-tests/array-nonarray-polymorphic-access.js: Added.

(f):
(run):

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (2 diffs)
• LayoutTests/fast/js/regress/array-nonarray-polymorphic-access-expected.txt (added)
• LayoutTests/fast/js/regress/array-nonarray-polymorphic-access.html (added)
• LayoutTests/fast/js/regress/script-tests/array-nonarray-polymorhpic-access.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecode/ArrayProfile.h (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGArrayMode.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2013-05-08  Mark Hahnenberg  <[email protected]>
+
+        DFGArrayMode::fromObserved is too liberal when it sees different Array and NonArray shapes
+        https://bugs.webkit.org/show_bug.cgi?id=115805
+
+        Added regression test for array access over polymorphic array vs. non-array indexing types.
+        With the fix, we get 3.666x faster on this microbenchmark.
+
+        Reviewed by Geoffrey Garen.
+
+        * fast/js/regress/array-nonarray-polymorphic-access-expected.txt: Added.
+        * fast/js/regress/array-nonarray-polymorphic-access.html: Added.
+        * fast/js/regress/script-tests/array-nonarray-polymorphic-access.js: Added.
+        (f):
+        (run):
+
+<<<<<<< .mine
+2013-05-08  Mark Hahnenberg  <[email protected]>
+
+        DFGArrayMode::fromObserved is too liberal when it sees different Array and NonArray shapes
+        https://bugs.webkit.org/show_bug.cgi?id=115805
+
+        Reviewed by Geoffrey Garen.
+
+        Added regression test for array access over polymorphic array vs. non-array indexing types.
+        With the fix, we get 3.666x faster on this microbenchmark.
+
+        * fast/js/regress/array-nonarray-polymorphic-access-expected.txt: Added.
+        * fast/js/regress/array-nonarray-polymorphic-access.html: Added.
+        * fast/js/regress/script-tests/array-nonarray-polymorhpic-access.js: Added.
+        (f):
+        (run):
+
+=======
 2013-05-09  Radu Stavila  <[email protected]>
 
@@ -97 +131 @@
         * platform/qt/fast/js/global-constructors-attributes-expected.txt: Added after r149758.
 
+>>>>>>> .r149833
 2013-05-08  Seokju Kwon  <[email protected]>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2013-05-08  Mark Hahnenberg  <[email protected]>
+
+        DFGArrayMode::fromObserved is too liberal when it sees different Array and NonArray shapes
+        https://bugs.webkit.org/show_bug.cgi?id=115805
+
+        Reviewed by Geoffrey Garen.
+
+        It checks the observed ArrayModes to see if we have seen any ArrayWith* first. If so, it assumes it's 
+        an Array::Array, even if we've also observed any NonArrayWith* in the ArrayProfile. This leads to the 
+        code generated by jumpSlowForUnwantedArrayMode to check the indexing type against (shape | IsArray) 
+        instead of just shape, which can cause us to exit a lot in the case that we saw a NonArray.
+
+        To fix this we need to add a case that checks for both ArrayWith* and NonArrayWith* cases first, which 
+        should then use Array::PossiblyArray, then do the checks we were already doing.
+
+        * bytecode/ArrayProfile.h:
+        (JSC::hasSeenArray):
+        (JSC::hasSeenNonArray):
+        * dfg/DFGArrayMode.cpp:
+        (JSC::DFG::ArrayMode::fromObserved):
+
 2013-05-09  Joe Mason  <[email protected]>
 

trunk/Source/JavaScriptCore/bytecode/ArrayProfile.h

@@ -116 +116 @@
 }
 
+inline bool hasSeenArray(ArrayModes arrayModes)
+{
+    return arrayModes & ALL_ARRAY_ARRAY_MODES;
+}
+
+inline bool hasSeenNonArray(ArrayModes arrayModes)
+{
+    return arrayModes & ALL_NON_ARRAY_ARRAY_MODES;
+}
+
 class ArrayProfile {
 public:

trunk/Source/JavaScriptCore/dfg/DFGArrayMode.cpp

@@ -113 +113 @@
             type = Array::Undecided;
         
-        if (observed & (asArrayModes(ArrayWithUndecided) | asArrayModes(ArrayWithInt32) | asArrayModes(ArrayWithDouble) | asArrayModes(ArrayWithContiguous) | asArrayModes(ArrayWithArrayStorage) | asArrayModes(ArrayWithSlowPutArrayStorage)))
+        if (hasSeenArray(observed) && hasSeenNonArray(observed))
+            arrayClass = Array::PossiblyArray;
+        else if (hasSeenArray(observed))
             arrayClass = Array::Array;
-        else if (observed & (asArrayModes(NonArray) | asArrayModes(NonArrayWithInt32) | asArrayModes(NonArrayWithDouble) | asArrayModes(NonArrayWithContiguous) | asArrayModes(NonArrayWithArrayStorage) | asArrayModes(NonArrayWithSlowPutArrayStorage)))
+        else if (hasSeenNonArray(observed))
             arrayClass = Array::NonArray;
         else