<!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>[205304] trunk</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/205304">205304</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2016-09-01 11:55:50 -0700 (Thu, 01 Sep 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>ObjectAllocationSinkingPhase::insertOSRHintsForUpdate() fails to emit updated hints in some cases
https://bugs.webkit.org/show_bug.cgi?id=161492
Reviewed by Mark Lam.
JSTests:
This bug affected function->activation references but not object->object field references,
because object->object field references are !neededForMaterialization(). So, the object
test always passed but the activation/function test used to always fail. It passes now.
* stress/materialize-activation-referenced-from-phantom-function.js: Added.
(bar):
(inc):
(dec):
(foo):
(test):
* stress/materialize-object-referenced-from-phantom-object.js: Added.
(bar):
(foo):
(test):
Source/JavaScriptCore:
If you materialize a sunken object that is referenced from another sunken object, then you
have to emit a PutHint to tell OSR that the latter object now refers to a materialized
object rather than to the old sunken one.
The ObjectAllocationSinkingPhase totally knows how to do this, but for some reason it only
did it when the PromotedLocationDescriptor for the field used for referring to the other
object is !neededForMaterialization(), i.e. it's a NamedPropertyPLoc or a ClosureVarPLoc.
I can sort of imagine why we thought that would be right - neededForMaterialization() means
it's a special meta-data field initialized on construction. But just because it's immutable
and special doesn't mean that materialization can't change its physical representation.
Removing the requirement that it's !neededForMaterialization() fixes the test and doesn't
regress anything.
* dfg/DFGObjectAllocationSinkingPhase.cpp:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkJSTestsChangeLog">trunk/JSTests/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGObjectAllocationSinkingPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkJSTestsstressmaterializeactivationreferencedfromphantomfunctionjs">trunk/JSTests/stress/materialize-activation-referenced-from-phantom-function.js</a></li>
<li><a href="#trunkJSTestsstressmaterializeobjectreferencedfromphantomobjectjs">trunk/JSTests/stress/materialize-object-referenced-from-phantom-object.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkJSTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/JSTests/ChangeLog (205303 => 205304)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/ChangeLog 2016-09-01 18:44:50 UTC (rev 205303)
+++ trunk/JSTests/ChangeLog 2016-09-01 18:55:50 UTC (rev 205304)
</span><span class="lines">@@ -1,3 +1,25 @@
</span><ins>+2016-09-01 Filip Pizlo <fpizlo@apple.com>
+
+ ObjectAllocationSinkingPhase::insertOSRHintsForUpdate() fails to emit updated hints in some cases
+ https://bugs.webkit.org/show_bug.cgi?id=161492
+
+ Reviewed by Mark Lam.
+
+ This bug affected function->activation references but not object->object field references,
+ because object->object field references are !neededForMaterialization(). So, the object
+ test always passed but the activation/function test used to always fail. It passes now.
+
+ * stress/materialize-activation-referenced-from-phantom-function.js: Added.
+ (bar):
+ (inc):
+ (dec):
+ (foo):
+ (test):
+ * stress/materialize-object-referenced-from-phantom-object.js: Added.
+ (bar):
+ (foo):
+ (test):
+
</ins><span class="cx"> 2016-08-31 Yusuke Suzuki <utatane.tea@gmail.com>
</span><span class="cx">
</span><span class="cx"> stress/random-53bit.js.ftl-no-cjit-no-inline-validate sometimes fails
</span></span></pre></div>
<a id="trunkJSTestsstressmaterializeactivationreferencedfromphantomfunctionjs"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/materialize-activation-referenced-from-phantom-function.js (0 => 205304)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/materialize-activation-referenced-from-phantom-function.js (rev 0)
+++ trunk/JSTests/stress/materialize-activation-referenced-from-phantom-function.js 2016-09-01 18:55:50 UTC (rev 205304)
</span><span class="lines">@@ -0,0 +1,42 @@
</span><ins>+function bar()
+{
+}
+
+noInline(bar);
+
+function foo(p, x)
+{
+ var value = 1;
+ function inc()
+ {
+ return value + 1;
+ }
+ function dec()
+ {
+ return value - 1;
+ }
+
+ if (!p)
+ return 0;
+
+ bar(inc);
+
+ x += 2000000000;
+
+ value = 42;
+ return dec();
+}
+
+noInline(foo);
+
+function test(x)
+{
+ var result = foo(true, x);
+ if (result != 42 - 1)
+ throw "Error: bad result: " + result;
+}
+
+for (var i = 0; i < 100000; ++i)
+ test(0);
+
+test(2000000000);
</ins></span></pre></div>
<a id="trunkJSTestsstressmaterializeobjectreferencedfromphantomobjectjs"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/materialize-object-referenced-from-phantom-object.js (0 => 205304)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/materialize-object-referenced-from-phantom-object.js (rev 0)
+++ trunk/JSTests/stress/materialize-object-referenced-from-phantom-object.js 2016-09-01 18:55:50 UTC (rev 205304)
</span><span class="lines">@@ -0,0 +1,37 @@
</span><ins>+function bar()
+{
+}
+
+noInline(bar);
+
+function foo(p, x)
+{
+ var a = {f: 1};
+ var b = {f: a};
+ var c = {f: a};
+
+ if (!p)
+ return 0;
+
+ bar(b);
+
+ x += 2000000000;
+
+ c.f.f = 42;
+ return b.f.f;
+}
+
+noInline(foo);
+
+function test(x)
+{
+ var result = foo(true, x);
+ if (result != 42)
+ throw "Error: bad result: " + result;
+}
+
+for (var i = 0; i < 100000; ++i)
+ test(0);
+
+test(2000000000);
+
</ins></span></pre></div>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (205303 => 205304)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-09-01 18:44:50 UTC (rev 205303)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-09-01 18:55:50 UTC (rev 205304)
</span><span class="lines">@@ -1,3 +1,25 @@
</span><ins>+2016-09-01 Filip Pizlo <fpizlo@apple.com>
+
+ ObjectAllocationSinkingPhase::insertOSRHintsForUpdate() fails to emit updated hints in some cases
+ https://bugs.webkit.org/show_bug.cgi?id=161492
+
+ Reviewed by Mark Lam.
+
+ If you materialize a sunken object that is referenced from another sunken object, then you
+ have to emit a PutHint to tell OSR that the latter object now refers to a materialized
+ object rather than to the old sunken one.
+
+ The ObjectAllocationSinkingPhase totally knows how to do this, but for some reason it only
+ did it when the PromotedLocationDescriptor for the field used for referring to the other
+ object is !neededForMaterialization(), i.e. it's a NamedPropertyPLoc or a ClosureVarPLoc.
+ I can sort of imagine why we thought that would be right - neededForMaterialization() means
+ it's a special meta-data field initialized on construction. But just because it's immutable
+ and special doesn't mean that materialization can't change its physical representation.
+ Removing the requirement that it's !neededForMaterialization() fixes the test and doesn't
+ regress anything.
+
+ * dfg/DFGObjectAllocationSinkingPhase.cpp:
+
</ins><span class="cx"> 2016-09-01 Chris Dumez <cdumez@apple.com>
</span><span class="cx">
</span><span class="cx"> Unreviewed, rolling out r205297.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGObjectAllocationSinkingPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp (205303 => 205304)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp 2016-09-01 18:44:50 UTC (rev 205303)
+++ trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp 2016-09-01 18:55:50 UTC (rev 205304)
</span><span class="lines">@@ -1242,7 +1242,7 @@
</span><span class="cx">
</span><span class="cx"> for (const auto& field : entry.value.fields()) {
</span><span class="cx"> ASSERT(m_sinkCandidates.contains(entry.key) || !escapees.contains(field.value));
</span><del>- if (escapees.contains(field.value) && !field.key.neededForMaterialization())
</del><ins>+ if (escapees.contains(field.value))
</ins><span class="cx"> hints.append(PromotedHeapLocation(entry.key, field.key));
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="lines">@@ -1692,6 +1692,8 @@
</span><span class="cx"> m_localMapping.set(location, m_bottom);
</span><span class="cx">
</span><span class="cx"> if (m_sinkCandidates.contains(node)) {
</span><ins>+ if (verbose)
+ dataLog("For sink candidate ", node, " found location ", location, "\n");
</ins><span class="cx"> m_insertionSet.insert(
</span><span class="cx"> nodeIndex + 1,
</span><span class="cx"> location.createHint(
</span><span class="lines">@@ -1758,6 +1760,8 @@
</span><span class="cx">
</span><span class="cx"> doLower = true;
</span><span class="cx">
</span><ins>+ if (verbose)
+ dataLog("Creating hint with value ", nodeValue, " before ", node, "\n");
</ins><span class="cx"> m_insertionSet.insert(
</span><span class="cx"> nodeIndex + 1,
</span><span class="cx"> location.createHint(
</span><span class="lines">@@ -1893,6 +1897,13 @@
</span><span class="cx">
</span><span class="cx"> void insertOSRHintsForUpdate(unsigned nodeIndex, NodeOrigin origin, bool& canExit, AvailabilityMap& availability, Node* escapee, Node* materialization)
</span><span class="cx"> {
</span><ins>+ if (verbose) {
+ dataLog("Inserting OSR hints at ", origin, ":\n");
+ dataLog(" Escapee: ", escapee, "\n");
+ dataLog(" Materialization: ", materialization, "\n");
+ dataLog(" Availability: ", availability, "\n");
+ }
+
</ins><span class="cx"> // We need to follow() the value in the heap.
</span><span class="cx"> // Consider the following graph:
</span><span class="cx"> //
</span></span></pre>
</div>
</div>
</body>
</html>