- Revision
- 184110
- Author
- [email protected]
- Date
- 2015-05-11 11:18:10 -0700 (Mon, 11 May 2015)
Log Message
We shouldn't promote LoadVarargs to a sequence of GetStacks and PutStacks if doing so would exceed the LoadVarargs' limit
https://bugs.webkit.org/show_bug.cgi?id=144851
Reviewed by Michael Saboff.
Source/_javascript_Core:
LoadVarargs loads arguments from some object and puts them on the stack. The region of
stack is controlled by a bunch of meta-data, including InlineCallFrame. InlineCallFrame
shouldn't really be edited after ByteCodeParser, so we cannot convert LoadVarargs to
something that uses more stack than the LoadVarargs wanted to.
This check was missing in the ArgumentsEliminationPhase's LoadVarargs->GetStack+PutStack
promoter. This is an important promotion rule for performance, and in cases where we are
compiling truly hot code, the LoadVarargs limit will be at least as big as the length of
the phantom arguments array that this phase sees. The LoadVarargs limit is based on
profiling and the phantom arguments array is a proof; in most cases the profiling is more
conservative.
But, you could write some crazy code where the statically obvious arguments array value is
bigger than what the profiling would have told you. When this happens, this promotion
effectively removes a bounds check. This either results in us clobbering a bunch of stack,
or it means that we never initialize a region of the stack that a later operation will read
(the uninitialization happens because PutStackSinkingPhase removes PutStacks that appear
unnecessary, and a GetMyArgumentByVal will claim not to use the region of the stack outside
the original LoadVarargs limit).
* dfg/DFGArgumentsEliminationPhase.cpp:
* tests/stress/load-varargs-elimination-bounds-check-barely.js: Added.
(foo):
(bar):
(baz):
* tests/stress/load-varargs-elimination-bounds-check.js: Added.
(foo):
(bar):
(baz):
LayoutTests:
* js/regress/load-varargs-elimination-expected.txt: Added.
* js/regress/load-varargs-elimination.html: Added.
* js/regress/script-tests/load-varargs-elimination.js: Added.
(foo):
(bar):
(baz):
* js/regress/sink-huge-activation-expected.txt: Added.
* js/regress/sink-huge-activation.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (184109 => 184110)
--- trunk/LayoutTests/ChangeLog 2015-05-11 18:02:57 UTC (rev 184109)
+++ trunk/LayoutTests/ChangeLog 2015-05-11 18:18:10 UTC (rev 184110)
@@ -1,3 +1,19 @@
+2015-05-10 Filip Pizlo <[email protected]>
+
+ We shouldn't promote LoadVarargs to a sequence of GetStacks and PutStacks if doing so would exceed the LoadVarargs' limit
+ https://bugs.webkit.org/show_bug.cgi?id=144851
+
+ Reviewed by Michael Saboff.
+
+ * js/regress/load-varargs-elimination-expected.txt: Added.
+ * js/regress/load-varargs-elimination.html: Added.
+ * js/regress/script-tests/load-varargs-elimination.js: Added.
+ (foo):
+ (bar):
+ (baz):
+ * js/regress/sink-huge-activation-expected.txt: Added.
+ * js/regress/sink-huge-activation.html: Added.
+
2015-05-11 Marcos ChavarrÃa Teijeiro <[email protected]>
[GTK] Gardening 11th May.
Added: trunk/LayoutTests/js/regress/load-varargs-elimination-expected.txt (0 => 184110)
--- trunk/LayoutTests/js/regress/load-varargs-elimination-expected.txt (rev 0)
+++ trunk/LayoutTests/js/regress/load-varargs-elimination-expected.txt 2015-05-11 18:18:10 UTC (rev 184110)
@@ -0,0 +1,10 @@
+JSRegress/load-varargs-elimination
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS no exception thrown
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/js/regress/load-varargs-elimination.html (0 => 184110)
--- trunk/LayoutTests/js/regress/load-varargs-elimination.html (rev 0)
+++ trunk/LayoutTests/js/regress/load-varargs-elimination.html 2015-05-11 18:18:10 UTC (rev 184110)
@@ -0,0 +1,12 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+<script src=""
+<script src=""
+</body>
+</html>
Added: trunk/LayoutTests/js/regress/script-tests/load-varargs-elimination.js (0 => 184110)
--- trunk/LayoutTests/js/regress/script-tests/load-varargs-elimination.js (rev 0)
+++ trunk/LayoutTests/js/regress/script-tests/load-varargs-elimination.js 2015-05-11 18:18:10 UTC (rev 184110)
@@ -0,0 +1,23 @@
+function foo() {
+ var result = 0;
+ for (var i = 0; i < arguments.length; ++i)
+ result += arguments[i];
+ return result;
+}
+
+function bar() {
+ return foo.apply(this, arguments);
+}
+
+function baz() {
+ return bar(1, 2, 3, 4);
+}
+
+noInline(baz);
+
+for (var i = 0; i < 1000000; ++i) {
+ var result = baz();
+ if (result != 10)
+ throw "Error: bad result: " + result;
+}
+
Added: trunk/LayoutTests/js/regress/sink-huge-activation-expected.txt (0 => 184110)
--- trunk/LayoutTests/js/regress/sink-huge-activation-expected.txt (rev 0)
+++ trunk/LayoutTests/js/regress/sink-huge-activation-expected.txt 2015-05-11 18:18:10 UTC (rev 184110)
@@ -0,0 +1,10 @@
+JSRegress/sink-huge-activation
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS no exception thrown
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/js/regress/sink-huge-activation.html (0 => 184110)
--- trunk/LayoutTests/js/regress/sink-huge-activation.html (rev 0)
+++ trunk/LayoutTests/js/regress/sink-huge-activation.html 2015-05-11 18:18:10 UTC (rev 184110)
@@ -0,0 +1,12 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+<script src=""
+<script src=""
+</body>
+</html>
Modified: trunk/Source/_javascript_Core/ChangeLog (184109 => 184110)
--- trunk/Source/_javascript_Core/ChangeLog 2015-05-11 18:02:57 UTC (rev 184109)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-05-11 18:18:10 UTC (rev 184110)
@@ -1,3 +1,40 @@
+2015-05-10 Filip Pizlo <[email protected]>
+
+ We shouldn't promote LoadVarargs to a sequence of GetStacks and PutStacks if doing so would exceed the LoadVarargs' limit
+ https://bugs.webkit.org/show_bug.cgi?id=144851
+
+ Reviewed by Michael Saboff.
+
+ LoadVarargs loads arguments from some object and puts them on the stack. The region of
+ stack is controlled by a bunch of meta-data, including InlineCallFrame. InlineCallFrame
+ shouldn't really be edited after ByteCodeParser, so we cannot convert LoadVarargs to
+ something that uses more stack than the LoadVarargs wanted to.
+
+ This check was missing in the ArgumentsEliminationPhase's LoadVarargs->GetStack+PutStack
+ promoter. This is an important promotion rule for performance, and in cases where we are
+ compiling truly hot code, the LoadVarargs limit will be at least as big as the length of
+ the phantom arguments array that this phase sees. The LoadVarargs limit is based on
+ profiling and the phantom arguments array is a proof; in most cases the profiling is more
+ conservative.
+
+ But, you could write some crazy code where the statically obvious arguments array value is
+ bigger than what the profiling would have told you. When this happens, this promotion
+ effectively removes a bounds check. This either results in us clobbering a bunch of stack,
+ or it means that we never initialize a region of the stack that a later operation will read
+ (the uninitialization happens because PutStackSinkingPhase removes PutStacks that appear
+ unnecessary, and a GetMyArgumentByVal will claim not to use the region of the stack outside
+ the original LoadVarargs limit).
+
+ * dfg/DFGArgumentsEliminationPhase.cpp:
+ * tests/stress/load-varargs-elimination-bounds-check-barely.js: Added.
+ (foo):
+ (bar):
+ (baz):
+ * tests/stress/load-varargs-elimination-bounds-check.js: Added.
+ (foo):
+ (bar):
+ (baz):
+
2015-05-11 Andreas Kling <[email protected]>
JSON.stringify shouldn't use generic get() to access Array.length
Modified: trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp (184109 => 184110)
--- trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2015-05-11 18:02:57 UTC (rev 184109)
+++ trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2015-05-11 18:18:10 UTC (rev 184110)
@@ -450,7 +450,9 @@
LoadVarargsData* varargsData = node->loadVarargsData();
InlineCallFrame* inlineCallFrame = candidate->origin.semantic.inlineCallFrame;
- if (inlineCallFrame && !inlineCallFrame->isVarargs()) {
+ if (inlineCallFrame
+ && !inlineCallFrame->isVarargs()
+ && inlineCallFrame->arguments.size() - varargsData->offset <= varargsData->limit) {
Node* argumentCount = insertionSet.insertConstant(
nodeIndex, node->origin,
jsNumber(inlineCallFrame->arguments.size() - varargsData->offset));
Added: trunk/Source/_javascript_Core/tests/stress/load-varargs-elimination-bounds-check-barely.js (0 => 184110)
--- trunk/Source/_javascript_Core/tests/stress/load-varargs-elimination-bounds-check-barely.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/load-varargs-elimination-bounds-check-barely.js 2015-05-11 18:18:10 UTC (rev 184110)
@@ -0,0 +1,35 @@
+function foo() {
+ var result = 0;
+ for (var i = 0; i < arguments.length; ++i)
+ result += arguments[i];
+ return result;
+}
+
+function bar() {
+ return foo.apply(this, arguments);
+}
+
+function baz(p) {
+ if (p)
+ return bar(1, 42);
+ return 0;
+}
+
+noInline(baz);
+
+// Execute baz() once with p set, so that the call has a valid prediction.
+baz(true);
+
+// Warm up profiling in bar and foo. Convince this profiling that bar()'s varargs call will tend to
+// pass a small number of arguments;
+for (var i = 0; i < 1000; ++i)
+ bar(1);
+
+// Now compile baz(), but don't run the bad code yet.
+for (var i = 0; i < 10000; ++i)
+ baz(false);
+
+// Finally, trigger the bug.
+var result = baz(true);
+if (result != 43)
+ throw "Error: bad result: " + result;
Added: trunk/Source/_javascript_Core/tests/stress/load-varargs-elimination-bounds-check.js (0 => 184110)
--- trunk/Source/_javascript_Core/tests/stress/load-varargs-elimination-bounds-check.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/load-varargs-elimination-bounds-check.js 2015-05-11 18:18:10 UTC (rev 184110)
@@ -0,0 +1,35 @@
+function foo() {
+ var result = 0;
+ for (var i = 0; i < arguments.length; ++i)
+ result += arguments[i];
+ return result;
+}
+
+function bar() {
+ return foo.apply(this, arguments);
+}
+
+function baz(p) {
+ if (p)
+ return bar(1, 2, 3, 4);
+ return 0;
+}
+
+noInline(baz);
+
+// Execute baz() once with p set, so that the call has a valid prediction.
+baz(true);
+
+// Warm up profiling in bar and foo. Convince this profiling that bar()'s varargs call will tend to
+// pass a small number of arguments;
+for (var i = 0; i < 1000; ++i)
+ bar(1);
+
+// Now compile baz(), but don't run the bad code yet.
+for (var i = 0; i < 10000; ++i)
+ baz(false);
+
+// Finally, trigger the bug.
+var result = baz(true);
+if (result != 10)
+ throw "Error: bad result: " + result;
