<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[171190] trunk/Source/JavaScriptCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/171190">171190</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2014-07-17 11:17:43 -0700 (Thu, 17 Jul 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>DFG Flush(SetLocal) store elimination is overzealous for captured variables in the presence of nodes that have no effects but may throw
https://bugs.webkit.org/show_bug.cgi?id=134988
<rdar://problem/17706349>
Reviewed by Oliver Hunt.
Luckily, we also don't need this optimization to be super powerful: the only place
where it really matters is for getting rid of the redundancy between op_enter and
op_init_lazy_reg, and in that case, there is a small set of possible nodes between the
two things. This change updates the store eliminator to know about only that small,
obviously safe, set of nodes over which we can store-eliminate.
This shouldn't have any performance impact in the DFG because this optimization kicks
in relatively rarely already. And once we tier up into the FTL, we get a much better
store elimination over LLVM IR, so this really shouldn't matter at all.
The tricky part of this patch is that there is a close relative of this optimization,
for uncaptured variables that got flushed. This happens for arguments to inlined calls.
I make this work by splitting it into two different store eliminators.
Note that in the process of crafting the tests, I realized that we were incorrectly
DCEing NewArrayWithSize. That's not cool, since that can throw an exception for
negative array sizes. If we ever did want to DCE this node, we'd need to lower the node
to a check node followed by the actual allocation.
* dfg/DFGCSEPhase.cpp:
(JSC::DFG::CSEPhase::uncapturedSetLocalStoreElimination):
(JSC::DFG::CSEPhase::capturedSetLocalStoreElimination):
(JSC::DFG::CSEPhase::setLocalStoreElimination):
(JSC::DFG::CSEPhase::performNodeCSE):
(JSC::DFG::CSEPhase::SetLocalStoreEliminationResult::SetLocalStoreEliminationResult): Deleted.
* dfg/DFGNodeType.h:
* tests/stress/capture-escape-and-throw.js: Added.
(foo.f):
(foo):
* tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js: Added.
(foo):
(bar):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGCSEPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGNodeTypeh">trunk/Source/JavaScriptCore/dfg/DFGNodeType.h</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoretestsstresscaptureescapeandthrowjs">trunk/Source/JavaScriptCore/tests/stress/capture-escape-and-throw.js</a></li>
<li><a href="#trunkSourceJavaScriptCoretestsstressnewarraywithsizethrowexceptionandtearoffargumentsjs">trunk/Source/JavaScriptCore/tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (171189 => 171190)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2014-07-17 17:43:31 UTC (rev 171189)
+++ trunk/Source/JavaScriptCore/ChangeLog 2014-07-17 18:17:43 UTC (rev 171190)
</span><span class="lines">@@ -1,3 +1,44 @@
</span><ins>+2014-07-16 Filip Pizlo <fpizlo@apple.com>
+
+ DFG Flush(SetLocal) store elimination is overzealous for captured variables in the presence of nodes that have no effects but may throw
+ https://bugs.webkit.org/show_bug.cgi?id=134988
+ <rdar://problem/17706349>
+
+ Reviewed by Oliver Hunt.
+
+ Luckily, we also don't need this optimization to be super powerful: the only place
+ where it really matters is for getting rid of the redundancy between op_enter and
+ op_init_lazy_reg, and in that case, there is a small set of possible nodes between the
+ two things. This change updates the store eliminator to know about only that small,
+ obviously safe, set of nodes over which we can store-eliminate.
+
+ This shouldn't have any performance impact in the DFG because this optimization kicks
+ in relatively rarely already. And once we tier up into the FTL, we get a much better
+ store elimination over LLVM IR, so this really shouldn't matter at all.
+
+ The tricky part of this patch is that there is a close relative of this optimization,
+ for uncaptured variables that got flushed. This happens for arguments to inlined calls.
+ I make this work by splitting it into two different store eliminators.
+
+ Note that in the process of crafting the tests, I realized that we were incorrectly
+ DCEing NewArrayWithSize. That's not cool, since that can throw an exception for
+ negative array sizes. If we ever did want to DCE this node, we'd need to lower the node
+ to a check node followed by the actual allocation.
+
+ * dfg/DFGCSEPhase.cpp:
+ (JSC::DFG::CSEPhase::uncapturedSetLocalStoreElimination):
+ (JSC::DFG::CSEPhase::capturedSetLocalStoreElimination):
+ (JSC::DFG::CSEPhase::setLocalStoreElimination):
+ (JSC::DFG::CSEPhase::performNodeCSE):
+ (JSC::DFG::CSEPhase::SetLocalStoreEliminationResult::SetLocalStoreEliminationResult): Deleted.
+ * dfg/DFGNodeType.h:
+ * tests/stress/capture-escape-and-throw.js: Added.
+ (foo.f):
+ (foo):
+ * tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js: Added.
+ (foo):
+ (bar):
+
</ins><span class="cx"> 2014-07-15 Benjamin Poulain <benjamin@webkit.org>
</span><span class="cx">
</span><span class="cx"> Reduce the overhead of updating the AssemblerBuffer
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGCSEPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp (171189 => 171190)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp 2014-07-17 17:43:31 UTC (rev 171189)
+++ trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp 2014-07-17 18:17:43 UTC (rev 171190)
</span><span class="lines">@@ -928,48 +928,34 @@
</span><span class="cx"> return 0;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- struct SetLocalStoreEliminationResult {
- SetLocalStoreEliminationResult()
- : mayBeAccessed(false)
- , mayExit(false)
- , mayClobberWorld(false)
- {
- }
-
- bool mayBeAccessed;
- bool mayExit;
- bool mayClobberWorld;
- };
- SetLocalStoreEliminationResult setLocalStoreElimination(
- VirtualRegister local, Node* expectedNode)
</del><ins>+ bool uncapturedSetLocalStoreElimination(VirtualRegister local, Node* expectedNode)
</ins><span class="cx"> {
</span><del>- SetLocalStoreEliminationResult result;
</del><span class="cx"> for (unsigned i = m_indexInBlock; i--;) {
</span><span class="cx"> Node* node = m_currentBlock->at(i);
</span><span class="cx"> switch (node->op()) {
</span><span class="cx"> case GetLocal:
</span><span class="cx"> case Flush:
</span><span class="cx"> if (node->local() == local)
</span><del>- result.mayBeAccessed = true;
</del><ins>+ return false;
</ins><span class="cx"> break;
</span><span class="cx">
</span><span class="cx"> case GetLocalUnlinked:
</span><span class="cx"> if (node->unlinkedLocal() == local)
</span><del>- result.mayBeAccessed = true;
</del><ins>+ return false;
</ins><span class="cx"> break;
</span><span class="cx">
</span><span class="cx"> case SetLocal: {
</span><span class="cx"> if (node->local() != local)
</span><span class="cx"> break;
</span><span class="cx"> if (node != expectedNode)
</span><del>- result.mayBeAccessed = true;
- return result;
</del><ins>+ return false;
+ return true;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> case GetClosureVar:
</span><span class="cx"> case PutClosureVar:
</span><span class="cx"> if (static_cast<VirtualRegister>(node->varNumber()) == local)
</span><del>- result.mayBeAccessed = true;
</del><ins>+ return false;
</ins><span class="cx"> break;
</span><span class="cx">
</span><span class="cx"> case GetMyScope:
</span><span class="lines">@@ -977,25 +963,24 @@
</span><span class="cx"> if (node->origin.semantic.inlineCallFrame)
</span><span class="cx"> break;
</span><span class="cx"> if (m_graph.uncheckedActivationRegister() == local)
</span><del>- result.mayBeAccessed = true;
</del><ins>+ return false;
</ins><span class="cx"> break;
</span><span class="cx">
</span><span class="cx"> case CheckArgumentsNotCreated:
</span><span class="cx"> case GetMyArgumentsLength:
</span><span class="cx"> case GetMyArgumentsLengthSafe:
</span><span class="cx"> if (m_graph.uncheckedArgumentsRegisterFor(node->origin.semantic) == local)
</span><del>- result.mayBeAccessed = true;
</del><ins>+ return false;
</ins><span class="cx"> break;
</span><span class="cx">
</span><span class="cx"> case GetMyArgumentByVal:
</span><span class="cx"> case GetMyArgumentByValSafe:
</span><del>- result.mayBeAccessed = true;
- break;
</del><ins>+ return false;
</ins><span class="cx">
</span><span class="cx"> case GetByVal:
</span><span class="cx"> // If this is accessing arguments then it's potentially accessing locals.
</span><span class="cx"> if (node->arrayMode().type() == Array::Arguments)
</span><del>- result.mayBeAccessed = true;
</del><ins>+ return false;
</ins><span class="cx"> break;
</span><span class="cx">
</span><span class="cx"> case CreateArguments:
</span><span class="lines">@@ -1005,21 +990,66 @@
</span><span class="cx"> // are live. We could be clever here and check if the local qualifies as an
</span><span class="cx"> // argument register. But that seems like it would buy us very little since
</span><span class="cx"> // any kind of tear offs are rare to begin with.
</span><del>- result.mayBeAccessed = true;
- break;
</del><ins>+ return false;
</ins><span class="cx">
</span><span class="cx"> default:
</span><span class="cx"> break;
</span><span class="cx"> }
</span><del>- result.mayExit |= node->canExit();
- result.mayClobberWorld |= m_graph.clobbersWorld(node);
</del><ins>+ if (m_graph.clobbersWorld(node))
+ return false;
</ins><span class="cx"> }
</span><span class="cx"> RELEASE_ASSERT_NOT_REACHED();
</span><del>- // Be safe in release mode.
- result.mayBeAccessed = true;
- return result;
</del><ins>+ return false;
</ins><span class="cx"> }
</span><ins>+
+ bool capturedSetLocalStoreElimination(VirtualRegister local, Node* expectedNode)
+ {
+ for (unsigned i = m_indexInBlock; i--;) {
+ Node* node = m_currentBlock->at(i);
+ switch (node->op()) {
+ case GetLocal:
+ case Flush:
+ if (node->local() == local)
+ return false;
+ break;
+
+ case GetLocalUnlinked:
+ if (node->unlinkedLocal() == local)
+ return false;
+ break;
+
+ case SetLocal: {
+ if (node->local() != local)
+ break;
+ if (node != expectedNode)
+ return false;
+ return true;
+ }
+
+ case Phantom:
+ case Check:
+ case HardPhantom:
+ case MovHint:
+ case JSConstant:
+ case DoubleConstant:
+ case Int52Constant:
+ break;
+
+ default:
+ return false;
+ }
+ }
+ RELEASE_ASSERT_NOT_REACHED();
+ return false;
+ }
</ins><span class="cx">
</span><ins>+ bool setLocalStoreElimination(VariableAccessData* variableAccessData, Node* expectedNode)
+ {
+ if (variableAccessData->isCaptured())
+ return capturedSetLocalStoreElimination(variableAccessData->local(), expectedNode);
+ return uncapturedSetLocalStoreElimination(variableAccessData->local(), expectedNode);
+ }
+
</ins><span class="cx"> bool invalidationPointElimination()
</span><span class="cx"> {
</span><span class="cx"> for (unsigned i = m_indexInBlock; i--;) {
</span><span class="lines">@@ -1215,7 +1245,6 @@
</span><span class="cx"> case Flush: {
</span><span class="cx"> ASSERT(m_graph.m_form != SSA);
</span><span class="cx"> VariableAccessData* variableAccessData = node->variableAccessData();
</span><del>- VirtualRegister local = variableAccessData->local();
</del><span class="cx"> if (!node->child1()) {
</span><span class="cx"> // FIXME: It's silly that we punt on flush-eliminating here. We don't really
</span><span class="cx"> // need child1 to figure out what's going on.
</span><span class="lines">@@ -1239,12 +1268,9 @@
</span><span class="cx"> if (replacement->canExit())
</span><span class="cx"> break;
</span><span class="cx"> }
</span><del>- SetLocalStoreEliminationResult result =
- setLocalStoreElimination(local, replacement);
- if (result.mayBeAccessed || result.mayClobberWorld)
</del><ins>+ if (!setLocalStoreElimination(variableAccessData, replacement))
</ins><span class="cx"> break;
</span><span class="cx"> ASSERT(replacement->op() == SetLocal);
</span><del>- // FIXME: Investigate using mayExit as a further optimization.
</del><span class="cx"> node->convertToPhantom();
</span><span class="cx"> Node* dataNode = replacement->child1().node();
</span><span class="cx"> ASSERT(dataNode->hasResult());
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGNodeTypeh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGNodeType.h (171189 => 171190)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGNodeType.h 2014-07-17 17:43:31 UTC (rev 171189)
+++ trunk/Source/JavaScriptCore/dfg/DFGNodeType.h 2014-07-17 18:17:43 UTC (rev 171190)
</span><span class="lines">@@ -233,7 +233,7 @@
</span><span class="cx"> /* Allocations. */\
</span><span class="cx"> macro(NewObject, NodeResultJS) \
</span><span class="cx"> macro(NewArray, NodeResultJS | NodeHasVarArgs) \
</span><del>- macro(NewArrayWithSize, NodeResultJS) \
</del><ins>+ macro(NewArrayWithSize, NodeResultJS | NodeMustGenerate) \
</ins><span class="cx"> macro(NewArrayBuffer, NodeResultJS) \
</span><span class="cx"> macro(NewTypedArray, NodeResultJS | NodeClobbersWorld | NodeMustGenerate) \
</span><span class="cx"> macro(NewRegexp, NodeResultJS) \
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoretestsstresscaptureescapeandthrowjs"></a>
<div class="addfile"><h4>Added: trunk/Source/JavaScriptCore/tests/stress/capture-escape-and-throw.js (0 => 171190)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/tests/stress/capture-escape-and-throw.js (rev 0)
+++ trunk/Source/JavaScriptCore/tests/stress/capture-escape-and-throw.js 2014-07-17 18:17:43 UTC (rev 171190)
</span><span class="lines">@@ -0,0 +1,28 @@
</span><ins>+var f;
+
+function foo(s) {
+ var x = 1;
+ f = function() { return x; };
+ x = 2;
+ new Array(s);
+ x = 3;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i)
+ foo(1);
+
+var didThrow = false;
+try {
+ foo(-1);
+} catch (e) {
+ didThrow = e;
+}
+
+if (("" + didThrow).indexOf("RangeError") != 0)
+ throw "Error: did not throw right exception: " + didThrow;
+
+var result = f();
+if (result != 2)
+ throw "Error: bad result from f(): " + result;
</ins></span></pre></div>
<a id="trunkSourceJavaScriptCoretestsstressnewarraywithsizethrowexceptionandtearoffargumentsjs"></a>
<div class="addfile"><h4>Added: trunk/Source/JavaScriptCore/tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js (0 => 171190)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js (rev 0)
+++ trunk/Source/JavaScriptCore/tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js 2014-07-17 18:17:43 UTC (rev 171190)
</span><span class="lines">@@ -0,0 +1,26 @@
</span><ins>+function foo() {
+ var a = arguments;
+ return new Array(a[0]);
+}
+
+function bar(x) {
+ return foo(x);
+}
+
+noInline(bar);
+
+for (var i = 0; i < 10000; ++i) {
+ var result = bar(42);
+ if (result.length != 42)
+ throw "Error: bad result length: " + result;
+}
+
+var didThrow = false;
+try {
+ bar(-1);
+} catch (e) {
+ didThrow = e;
+}
+
+if (("" + didThrow).indexOf("RangeError") != 0)
+ throw "Error: did not throw right exception: " + didThrow;
</ins></span></pre>
</div>
</div>
</body>
</html>