Various array access corner cases should take OSR exit feedback
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Feb 2015 22:44:45 +0000 (22:44 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Feb 2015 22:44:45 +0000 (22:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=142056

Reviewed by Geoffrey Garen.
Source/JavaScriptCore:

Two major changes here:

- Don't keep converting GetById into GetArrayLength if we exited due to any kind of array
  type check.

- Use a generic form of GetByVal/PutByVal if we exited due to any kind of exotic checks,
  like the Arguments safety checks. We use the "ExoticObjectMode" for out-of-bounds on
  arguments for now, since it's a convenient way of forcing out-of-bounds to be handled by
  the Generic array mode.

* bytecode/ExitKind.cpp:
(JSC::exitKindToString):
* bytecode/ExitKind.h:
* dfg/DFGArrayMode.cpp:
(JSC::DFG::ArrayMode::refine):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileGetByValOnArguments):
(JSC::DFG::SpeculativeJIT::compileGetArgumentsLength):
* tests/stress/array-length-array-storage-plain-object.js: Added.
(foo):
* tests/stress/array-length-plain-object.js: Added.
(foo):

LayoutTests:

* js/regress/arguments-out-of-bounds-expected.txt: Added.
* js/regress/arguments-out-of-bounds.html: Added.
* js/regress/exit-length-on-plain-object-expected.txt: Added.
* js/regress/exit-length-on-plain-object.html: Added.
* js/regress/script-tests/arguments-out-of-bounds.js: Added.
(foo):
(bar):
* js/regress/script-tests/exit-length-on-plain-object.js: Added.
(foo):
* js/regress/script-tests/string-out-of-bounds.js: Added.
(bar):
* js/regress/string-out-of-bounds-expected.txt: Added.
* js/regress/string-out-of-bounds.html: Added.

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

18 files changed:
LayoutTests/ChangeLog
LayoutTests/js/regress/arguments-out-of-bounds-expected.txt [new file with mode: 0644]
LayoutTests/js/regress/arguments-out-of-bounds.html [new file with mode: 0644]
LayoutTests/js/regress/exit-length-on-plain-object-expected.txt [new file with mode: 0644]
LayoutTests/js/regress/exit-length-on-plain-object.html [new file with mode: 0644]
LayoutTests/js/regress/script-tests/arguments-out-of-bounds.js [new file with mode: 0644]
LayoutTests/js/regress/script-tests/exit-length-on-plain-object.js [new file with mode: 0644]
LayoutTests/js/regress/script-tests/string-out-of-bounds.js [new file with mode: 0644]
LayoutTests/js/regress/string-out-of-bounds-expected.txt [new file with mode: 0644]
LayoutTests/js/regress/string-out-of-bounds.html [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/ExitKind.cpp
Source/JavaScriptCore/bytecode/ExitKind.h
Source/JavaScriptCore/dfg/DFGArrayMode.cpp
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/tests/stress/array-length-array-storage-plain-object.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/array-length-plain-object.js [new file with mode: 0644]

index 585a3c5..ed13f99 100644 (file)
@@ -1,3 +1,24 @@
+2015-02-26  Filip Pizlo  <fpizlo@apple.com>
+
+        Various array access corner cases should take OSR exit feedback
+        https://bugs.webkit.org/show_bug.cgi?id=142056
+
+        Reviewed by Geoffrey Garen.
+
+        * js/regress/arguments-out-of-bounds-expected.txt: Added.
+        * js/regress/arguments-out-of-bounds.html: Added.
+        * js/regress/exit-length-on-plain-object-expected.txt: Added.
+        * js/regress/exit-length-on-plain-object.html: Added.
+        * js/regress/script-tests/arguments-out-of-bounds.js: Added.
+        (foo):
+        (bar):
+        * js/regress/script-tests/exit-length-on-plain-object.js: Added.
+        (foo):
+        * js/regress/script-tests/string-out-of-bounds.js: Added.
+        (bar):
+        * js/regress/string-out-of-bounds-expected.txt: Added.
+        * js/regress/string-out-of-bounds.html: Added.
+
 2015-02-26  Mark Lam  <mark.lam@apple.com>
 
         Rolling out r180602, r180608, r180613, r180617, r180671.
diff --git a/LayoutTests/js/regress/arguments-out-of-bounds-expected.txt b/LayoutTests/js/regress/arguments-out-of-bounds-expected.txt
new file mode 100644 (file)
index 0000000..9f64b4d
--- /dev/null
@@ -0,0 +1,10 @@
+JSRegress/arguments-out-of-bounds
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS no exception thrown
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/regress/arguments-out-of-bounds.html b/LayoutTests/js/regress/arguments-out-of-bounds.html
new file mode 100644 (file)
index 0000000..52142a4
--- /dev/null
@@ -0,0 +1,12 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script src="../../resources/regress-pre.js"></script>
+<script src="script-tests/arguments-out-of-bounds.js"></script>
+<script src="../../resources/regress-post.js"></script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/js/regress/exit-length-on-plain-object-expected.txt b/LayoutTests/js/regress/exit-length-on-plain-object-expected.txt
new file mode 100644 (file)
index 0000000..d59a0bc
--- /dev/null
@@ -0,0 +1,10 @@
+JSRegress/exit-length-on-plain-object
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS no exception thrown
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/regress/exit-length-on-plain-object.html b/LayoutTests/js/regress/exit-length-on-plain-object.html
new file mode 100644 (file)
index 0000000..648c4fe
--- /dev/null
@@ -0,0 +1,12 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script src="../../resources/regress-pre.js"></script>
+<script src="script-tests/exit-length-on-plain-object.js"></script>
+<script src="../../resources/regress-post.js"></script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/js/regress/script-tests/arguments-out-of-bounds.js b/LayoutTests/js/regress/script-tests/arguments-out-of-bounds.js
new file mode 100644 (file)
index 0000000..2f088f4
--- /dev/null
@@ -0,0 +1,21 @@
+function foo() { return arguments; }
+noInline(foo);
+
+function bar(o) {
+    var tmp = o[0];
+    var result = 0;
+    for (var i = 0; i < 1000; ++i) {
+        if (tmp)
+            result += tmp * i;
+    }
+    return result;
+}
+noInline(bar);
+
+var result = 0;
+var o = foo();
+for (var i = 0; i < 10000; ++i)
+    result += bar(o);
+
+if (result !== 0)
+    throw "Error: bad result: " + result;
diff --git a/LayoutTests/js/regress/script-tests/exit-length-on-plain-object.js b/LayoutTests/js/regress/script-tests/exit-length-on-plain-object.js
new file mode 100644 (file)
index 0000000..f451dcb
--- /dev/null
@@ -0,0 +1,31 @@
+function foo(o) {
+    var result;
+    for (var i = 0; i < 2; ++i) {
+        var subResult = o.length;
+        if (i == 0) {
+            result = subResult;
+            if (subResult !== void 0)
+                break;
+        }
+        o = [1];
+    }
+    if (result === void 0) {
+        for (var i = 0; i < 10000; ++i) { }
+    }
+    return result;
+}
+
+noInline(foo);
+
+for (var j = 0; j < 10; ++j) {
+    for (var i = 0; i < 10000; ++i) {
+        var a = [1];
+        a.length = 99999999;
+        a.f = 42;
+        foo(a);
+    }
+    
+    var result = foo({});
+    if (result !== void 0)
+        throw "Error: bad result: " + result;
+}
diff --git a/LayoutTests/js/regress/script-tests/string-out-of-bounds.js b/LayoutTests/js/regress/script-tests/string-out-of-bounds.js
new file mode 100644 (file)
index 0000000..d0aa714
--- /dev/null
@@ -0,0 +1,17 @@
+function bar(o) {
+    var tmp = o[0];
+    var result = 0;
+    for (var i = 0; i < 1000; ++i) {
+        if (tmp)
+            result += tmp * i;
+    }
+    return result;
+}
+noInline(bar);
+
+var result = 0;
+for (var i = 0; i < 10000; ++i)
+    result += bar("");
+
+if (result !== 0)
+    throw "Error: bad result: " + result;
diff --git a/LayoutTests/js/regress/string-out-of-bounds-expected.txt b/LayoutTests/js/regress/string-out-of-bounds-expected.txt
new file mode 100644 (file)
index 0000000..948d031
--- /dev/null
@@ -0,0 +1,10 @@
+JSRegress/string-out-of-bounds
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS no exception thrown
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/regress/string-out-of-bounds.html b/LayoutTests/js/regress/string-out-of-bounds.html
new file mode 100644 (file)
index 0000000..b2ca124
--- /dev/null
@@ -0,0 +1,12 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script src="../../resources/regress-pre.js"></script>
+<script src="script-tests/string-out-of-bounds.js"></script>
+<script src="../../resources/regress-post.js"></script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
index 536e229..b4f90b4 100644 (file)
@@ -1,3 +1,35 @@
+2015-02-26  Filip Pizlo  <fpizlo@apple.com>
+
+        Various array access corner cases should take OSR exit feedback
+        https://bugs.webkit.org/show_bug.cgi?id=142056
+
+        Reviewed by Geoffrey Garen.
+        
+        Two major changes here:
+        
+        - Don't keep converting GetById into GetArrayLength if we exited due to any kind of array
+          type check.
+        
+        - Use a generic form of GetByVal/PutByVal if we exited due to any kind of exotic checks,
+          like the Arguments safety checks. We use the "ExoticObjectMode" for out-of-bounds on
+          arguments for now, since it's a convenient way of forcing out-of-bounds to be handled by
+          the Generic array mode.
+
+        * bytecode/ExitKind.cpp:
+        (JSC::exitKindToString):
+        * bytecode/ExitKind.h:
+        * dfg/DFGArrayMode.cpp:
+        (JSC::DFG::ArrayMode::refine):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileGetByValOnArguments):
+        (JSC::DFG::SpeculativeJIT::compileGetArgumentsLength):
+        * tests/stress/array-length-array-storage-plain-object.js: Added.
+        (foo):
+        * tests/stress/array-length-plain-object.js: Added.
+        (foo):
+
 2015-02-25  Filip Pizlo  <fpizlo@apple.com>
 
         DFG SSA stack accesses shouldn't speak of VariableAccessDatas
index a3f8150..1577b57 100644 (file)
@@ -64,6 +64,8 @@ const char* exitKindToString(ExitKind kind)
         return "InadequateCoverage";
     case ArgumentsEscaped:
         return "ArgumentsEscaped";
+    case ExoticObjectMode:
+        return "ExoticObjectMode";
     case NotStringObject:
         return "NotStringObject";
     case VarargsOverflow:
index 855a867..90ac08a 100644 (file)
@@ -44,6 +44,7 @@ enum ExitKind : uint8_t {
     OutOfBounds, // We had an out-of-bounds access to an array.
     InadequateCoverage, // We exited because we ended up in code that didn't have profiling coverage.
     ArgumentsEscaped, // We exited because arguments escaped but we didn't expect them to.
+    ExoticObjectMode, // We exited because some exotic object that we were accessing was in an exotic mode (like Arguments with slow arguments).
     NotStringObject, // We exited because we shouldn't have attempted to optimize string object access.
     VarargsOverflow, // We exited because a varargs call passed more arguments than we expected.
     Uncountable, // We exited for none of the above reasons, and we should not count it. Most uses of this should be viewed as a FIXME.
index fa1e5e6..fcc9b2a 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012, 2013, 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-2015 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -147,6 +147,10 @@ ArrayMode ArrayMode::refine(
     if (!isInt32Speculation(index))
         return ArrayMode(Array::Generic);
     
+    // If we had exited because of an exotic object behavior, then don't try to specialize.
+    if (graph.hasExitSite(node->origin.semantic, ExoticObjectMode))
+        return ArrayMode(Array::Generic);
+    
     // Note: our profiling currently doesn't give us good information in case we have
     // an unlikely control flow path that sets the base to a non-cell value. Value
     // profiling and prediction propagation will probably tell us that the value is
index a537455..d7809dd 100644 (file)
@@ -919,18 +919,25 @@ private:
         case GetByIdFlush: {
             if (!node->child1()->shouldSpeculateCell())
                 break;
-            StringImpl* impl = m_graph.identifiers()[node->identifierNumber()];
-            if (impl == vm().propertyNames->length.impl()) {
-                attemptToMakeGetArrayLength(node);
-                break;
-            }
-            if (impl == vm().propertyNames->byteLength.impl()) {
-                attemptToMakeGetTypedArrayByteLength(node);
-                break;
-            }
-            if (impl == vm().propertyNames->byteOffset.impl()) {
-                attemptToMakeGetTypedArrayByteOffset(node);
-                break;
+
+            // If we hadn't exited because of BadCache, BadIndexingType, or ExoticObjectMode, then
+            // leave this as a GetById.
+            if (!m_graph.hasExitSite(node->origin.semantic, BadCache)
+                && !m_graph.hasExitSite(node->origin.semantic, BadIndexingType)
+                && !m_graph.hasExitSite(node->origin.semantic, ExoticObjectMode)) {
+                StringImpl* impl = m_graph.identifiers()[node->identifierNumber()];
+                if (impl == vm().propertyNames->length.impl()) {
+                    attemptToMakeGetArrayLength(node);
+                    break;
+                }
+                if (impl == vm().propertyNames->byteLength.impl()) {
+                    attemptToMakeGetTypedArrayByteLength(node);
+                    break;
+                }
+                if (impl == vm().propertyNames->byteOffset.impl()) {
+                    attemptToMakeGetTypedArrayByteOffset(node);
+                    break;
+                }
             }
             fixEdge<CellUse>(node->child1());
             break;
index 92e55cf..061b548 100644 (file)
@@ -4113,12 +4113,12 @@ void SpeculativeJIT::compileGetByValOnArguments(Node* node)
     
     // Two really lame checks.
     speculationCheck(
-        Uncountable, JSValueSource(), 0,
+        ExoticObjectMode, JSValueSource(), 0,
         m_jit.branch32(
             MacroAssembler::AboveOrEqual, propertyReg,
             MacroAssembler::Address(baseReg, Arguments::offsetOfNumArguments())));
     speculationCheck(
-        Uncountable, JSValueSource(), 0,
+        ExoticObjectMode, JSValueSource(), 0,
         m_jit.branchTestPtr(
             MacroAssembler::NonZero,
             MacroAssembler::Address(
@@ -4168,7 +4168,7 @@ void SpeculativeJIT::compileGetArgumentsLength(Node* node)
     ASSERT(ArrayMode(Array::Arguments).alreadyChecked(m_jit.graph(), node, m_state.forNode(node->child1())));
     
     speculationCheck(
-        Uncountable, JSValueSource(), 0,
+        ExoticObjectMode, JSValueSource(), 0,
         m_jit.branchTest8(
             MacroAssembler::NonZero,
             MacroAssembler::Address(baseReg, Arguments::offsetOfOverrodeLength())));
diff --git a/Source/JavaScriptCore/tests/stress/array-length-array-storage-plain-object.js b/Source/JavaScriptCore/tests/stress/array-length-array-storage-plain-object.js
new file mode 100644 (file)
index 0000000..e424933
--- /dev/null
@@ -0,0 +1,16 @@
+function foo(o) {
+    return o.length;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i) {
+    var a = [1];
+    a.length = 99999999;
+    a.f = 42;
+    foo(a);
+}
+
+var result = foo({});
+if (result !== void 0)
+    throw "Error: bad result: " + result;
diff --git a/Source/JavaScriptCore/tests/stress/array-length-plain-object.js b/Source/JavaScriptCore/tests/stress/array-length-plain-object.js
new file mode 100644 (file)
index 0000000..ed85751
--- /dev/null
@@ -0,0 +1,15 @@
+function foo(o) {
+    return o.length;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i) {
+    var a = [1];
+    a.f = 42;
+    foo(a);
+}
+
+var result = foo({});
+if (result !== void 0)
+    throw "Error: bad result: " + result;