Various array access corner cases should take OSR exit feedback
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: http://svn.webkit.org/repository/webkit/trunk@180703 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 585a3c5..ed13f99 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -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
index 0000000..9f64b4d
--- /dev/null
+++ b/LayoutTests/js/regress/arguments-out-of-bounds-expected.txt
@@ -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
index 0000000..52142a4
--- /dev/null
+++ b/LayoutTests/js/regress/arguments-out-of-bounds.html
@@ -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
index 0000000..d59a0bc
--- /dev/null
+++ b/LayoutTests/js/regress/exit-length-on-plain-object-expected.txt
@@ -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
index 0000000..648c4fe
--- /dev/null
+++ b/LayoutTests/js/regress/exit-length-on-plain-object.html
@@ -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
index 0000000..2f088f4
--- /dev/null
+++ b/LayoutTests/js/regress/script-tests/arguments-out-of-bounds.js
@@ -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
index 0000000..f451dcb
--- /dev/null
+++ b/LayoutTests/js/regress/script-tests/exit-length-on-plain-object.js
@@ -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
index 0000000..d0aa714
--- /dev/null
+++ b/LayoutTests/js/regress/script-tests/string-out-of-bounds.js
@@ -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
index 0000000..948d031
--- /dev/null
+++ b/LayoutTests/js/regress/string-out-of-bounds-expected.txt
@@ -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
index 0000000..b2ca124
--- /dev/null
+++ b/LayoutTests/js/regress/string-out-of-bounds.html
@@ -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>
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 536e229..b4f90b4 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -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
diff --git a/Source/JavaScriptCore/bytecode/ExitKind.cpp b/Source/JavaScriptCore/bytecode/ExitKind.cpp
index a3f8150..1577b57 100644
--- a/Source/JavaScriptCore/bytecode/ExitKind.cpp
+++ b/Source/JavaScriptCore/bytecode/ExitKind.cpp
@@ -64,6 +64,8 @@
return "InadequateCoverage";
case ArgumentsEscaped:
return "ArgumentsEscaped";
+ case ExoticObjectMode:
+ return "ExoticObjectMode";
case NotStringObject:
return "NotStringObject";
case VarargsOverflow:
diff --git a/Source/JavaScriptCore/bytecode/ExitKind.h b/Source/JavaScriptCore/bytecode/ExitKind.h
index 855a867..90ac08a 100644
--- a/Source/JavaScriptCore/bytecode/ExitKind.h
+++ b/Source/JavaScriptCore/bytecode/ExitKind.h
@@ -44,6 +44,7 @@
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.
diff --git a/Source/JavaScriptCore/dfg/DFGArrayMode.cpp b/Source/JavaScriptCore/dfg/DFGArrayMode.cpp
index fa1e5e6..fcc9b2a 100644
--- a/Source/JavaScriptCore/dfg/DFGArrayMode.cpp
+++ b/Source/JavaScriptCore/dfg/DFGArrayMode.cpp
@@ -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 @@
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
diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
index a537455..d7809dd 100644
--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
@@ -919,18 +919,25 @@
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;
diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
index 92e55cf..061b548 100644
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
@@ -4113,12 +4113,12 @@
// 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 @@
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
index 0000000..e424933
--- /dev/null
+++ b/Source/JavaScriptCore/tests/stress/array-length-array-storage-plain-object.js
@@ -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
index 0000000..ed85751
--- /dev/null
+++ b/Source/JavaScriptCore/tests/stress/array-length-plain-object.js
@@ -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;
