<!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>[190215] 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/190215">190215</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2015-09-24 12:23:58 -0700 (Thu, 24 Sep 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>PolymorphicAccess should remember that it checked an ObjectPropertyCondition with a check on some structure
https://bugs.webkit.org/show_bug.cgi?id=149514
Reviewed by Oliver Hunt.
When we checked an ObjectPropertyCondition using an explicit structure check, we would forget to
note the structure in any weak reference table and we would attempt to regenerate the condition
check even if the condition became invalid.
We need to account for this better and we need to prune AccessCases that have an invalid condition
set. This change does both.
* bytecode/PolymorphicAccess.cpp:
(JSC::AccessGenerationState::addWatchpoint):
(JSC::AccessCase::alternateBase):
(JSC::AccessCase::couldStillSucceed):
(JSC::AccessCase::canReplace):
(JSC::AccessCase::generate):
(JSC::PolymorphicAccess::regenerateWithCases):
(JSC::PolymorphicAccess::visitWeak):
(JSC::PolymorphicAccess::regenerate):
* bytecode/PolymorphicAccess.h:
(JSC::AccessCase::callLinkInfo):
* tests/stress/make-dictionary-repatch.js: Added. This used to crash on a release assert. If we removed the release assert, this would return bad results.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCorebytecodePolymorphicAccesscpp">trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCorebytecodePolymorphicAccessh">trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.h</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoretestsstressmakedictionaryrepatchjs">trunk/Source/JavaScriptCore/tests/stress/make-dictionary-repatch.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (190214 => 190215)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-09-24 19:00:11 UTC (rev 190214)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-09-24 19:23:58 UTC (rev 190215)
</span><span class="lines">@@ -1,3 +1,30 @@
</span><ins>+2015-09-23 Filip Pizlo <fpizlo@apple.com>
+
+ PolymorphicAccess should remember that it checked an ObjectPropertyCondition with a check on some structure
+ https://bugs.webkit.org/show_bug.cgi?id=149514
+
+ Reviewed by Oliver Hunt.
+
+ When we checked an ObjectPropertyCondition using an explicit structure check, we would forget to
+ note the structure in any weak reference table and we would attempt to regenerate the condition
+ check even if the condition became invalid.
+
+ We need to account for this better and we need to prune AccessCases that have an invalid condition
+ set. This change does both.
+
+ * bytecode/PolymorphicAccess.cpp:
+ (JSC::AccessGenerationState::addWatchpoint):
+ (JSC::AccessCase::alternateBase):
+ (JSC::AccessCase::couldStillSucceed):
+ (JSC::AccessCase::canReplace):
+ (JSC::AccessCase::generate):
+ (JSC::PolymorphicAccess::regenerateWithCases):
+ (JSC::PolymorphicAccess::visitWeak):
+ (JSC::PolymorphicAccess::regenerate):
+ * bytecode/PolymorphicAccess.h:
+ (JSC::AccessCase::callLinkInfo):
+ * tests/stress/make-dictionary-repatch.js: Added. This used to crash on a release assert. If we removed the release assert, this would return bad results.
+
</ins><span class="cx"> 2015-09-24 Mark Lam <mark.lam@apple.com>
</span><span class="cx">
</span><span class="cx"> We should only expect a RareCaseProfile to exist if the rare case actually exists.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorebytecodePolymorphicAccesscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp (190214 => 190215)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp 2015-09-24 19:00:11 UTC (rev 190214)
+++ trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp 2015-09-24 19:23:58 UTC (rev 190215)
</span><span class="lines">@@ -61,6 +61,7 @@
</span><span class="cx"> Vector<std::function<void(LinkBuffer&)>> callbacks;
</span><span class="cx"> const Identifier* ident;
</span><span class="cx"> std::unique_ptr<WatchpointsOnStructureStubInfo> watchpoints;
</span><ins>+ Vector<WriteBarrier<JSCell>> weakReferences;
</ins><span class="cx">
</span><span class="cx"> Watchpoint* addWatchpoint(const ObjectPropertyCondition& condition = ObjectPropertyCondition())
</span><span class="cx"> {
</span><span class="lines">@@ -251,6 +252,11 @@
</span><span class="cx"> return conditionSet().slotBaseCondition().object();
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+bool AccessCase::couldStillSucceed() const
+{
+ return m_conditionSet.structuresEnsureValidityAssumingImpurePropertyWatchpoint();
+}
+
</ins><span class="cx"> bool AccessCase::canReplace(const AccessCase& other)
</span><span class="cx"> {
</span><span class="cx"> // We could do a lot better here, but for now we just do something obvious.
</span><span class="lines">@@ -375,6 +381,7 @@
</span><span class="cx">
</span><span class="cx"> CCallHelpers& jit = *state.jit;
</span><span class="cx"> VM& vm = *jit.vm();
</span><ins>+ CodeBlock* codeBlock = jit.codeBlock();
</ins><span class="cx"> StructureStubInfo& stubInfo = *state.stubInfo;
</span><span class="cx"> const Identifier& ident = *state.ident;
</span><span class="cx"> JSValueRegs valueRegs = state.valueRegs;
</span><span class="lines">@@ -398,7 +405,14 @@
</span><span class="cx"> continue;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- RELEASE_ASSERT(condition.structureEnsuresValidityAssumingImpurePropertyWatchpoint(structure));
</del><ins>+ if (!condition.structureEnsuresValidityAssumingImpurePropertyWatchpoint(structure)) {
+ dataLog("This condition is no longer met: ", condition, "\n");
+ RELEASE_ASSERT_NOT_REACHED();
+ }
+
+ // We will emit code that has a weak reference that isn't otherwise listed anywhere.
+ state.weakReferences.append(WriteBarrier<JSCell>(vm, codeBlock->ownerExecutable(), structure));
+
</ins><span class="cx"> jit.move(CCallHelpers::TrustedImmPtr(condition.object()), scratchGPR);
</span><span class="cx"> state.failAndRepatch.append(
</span><span class="cx"> jit.branchStructure(
</span><span class="lines">@@ -977,6 +991,11 @@
</span><span class="cx"> // then adding the new cases.
</span><span class="cx"> ListType newCases;
</span><span class="cx"> for (auto& oldCase : m_list) {
</span><ins>+ // Ignore old cases that cannot possibly succeed anymore.
+ if (!oldCase->couldStillSucceed())
+ continue;
+
+ // Figure out if this is replaced by any new cases.
</ins><span class="cx"> bool found = false;
</span><span class="cx"> for (auto& caseToAdd : casesToAdd) {
</span><span class="cx"> if (caseToAdd->canReplace(*oldCase)) {
</span><span class="lines">@@ -984,8 +1003,10 @@
</span><span class="cx"> break;
</span><span class="cx"> }
</span><span class="cx"> }
</span><del>- if (!found)
- newCases.append(oldCase->clone());
</del><ins>+ if (found)
+ continue;
+
+ newCases.append(oldCase->clone());
</ins><span class="cx"> }
</span><span class="cx"> for (auto& caseToAdd : casesToAdd)
</span><span class="cx"> newCases.append(WTF::move(caseToAdd));
</span><span class="lines">@@ -1022,6 +1043,12 @@
</span><span class="cx"> if (!at(i).visitWeak(vm))
</span><span class="cx"> return false;
</span><span class="cx"> }
</span><ins>+ if (Vector<WriteBarrier<JSCell>>* weakReferences = m_weakReferences.get()) {
+ for (WriteBarrier<JSCell>& weakReference : *weakReferences) {
+ if (!Heap::isMarked(weakReference.get()))
+ return false;
+ }
+ }
</ins><span class="cx"> return true;
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -1145,6 +1172,8 @@
</span><span class="cx">
</span><span class="cx"> m_stubRoutine = createJITStubRoutine(code, vm, codeBlock->ownerExecutable(), doesCalls);
</span><span class="cx"> m_watchpoints = WTF::move(state.watchpoints);
</span><ins>+ if (!state.weakReferences.isEmpty())
+ m_weakReferences = std::make_unique<Vector<WriteBarrier<JSCell>>>(WTF::move(state.weakReferences));
</ins><span class="cx"> if (verbose)
</span><span class="cx"> dataLog("Returning: ", code.code(), "\n");
</span><span class="cx"> return code.code();
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorebytecodePolymorphicAccessh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.h (190214 => 190215)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.h 2015-09-24 19:00:11 UTC (rev 190214)
+++ trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.h 2015-09-24 19:23:58 UTC (rev 190215)
</span><span class="lines">@@ -206,6 +206,9 @@
</span><span class="cx"> return m_rareData->callLinkInfo.get();
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ // Is it still possible for this case to ever be taken?
+ bool couldStillSucceed() const;
+
</ins><span class="cx"> // If this method returns true, then it's a good idea to remove 'other' from the access once 'this'
</span><span class="cx"> // is added. This method assumes that in case of contradictions, 'this' represents a newer, and so
</span><span class="cx"> // more useful, truth. This method can be conservative; it will return false when it doubt.
</span><span class="lines">@@ -301,6 +304,7 @@
</span><span class="cx"> ListType m_list;
</span><span class="cx"> RefPtr<JITStubRoutine> m_stubRoutine;
</span><span class="cx"> std::unique_ptr<WatchpointsOnStructureStubInfo> m_watchpoints;
</span><ins>+ std::unique_ptr<Vector<WriteBarrier<JSCell>>> m_weakReferences;
</ins><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> } // namespace JSC
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoretestsstressmakedictionaryrepatchjs"></a>
<div class="addfile"><h4>Added: trunk/Source/JavaScriptCore/tests/stress/make-dictionary-repatch.js (0 => 190215)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/tests/stress/make-dictionary-repatch.js (rev 0)
+++ trunk/Source/JavaScriptCore/tests/stress/make-dictionary-repatch.js 2015-09-24 19:23:58 UTC (rev 190215)
</span><span class="lines">@@ -0,0 +1,36 @@
</span><ins>+//@ if $jitTests then runMiscNoCJITTest("--useDFGJIT=false", "--useLLInt=false") else skip end
+
+function foo(o) {
+ return o.f;
+}
+
+var p1 = {};
+p1.f = 42;
+
+var crazy = {};
+crazy.f = 1;
+crazy.g = 2;
+
+var p2 = Object.create(p1);
+
+var crazy = Object.create(p1);
+crazy.f = 1;
+crazy.g = 2;
+
+function make() {
+ return Object.create(p2);
+}
+
+for (var i = 0; i < 100; ++i)
+ foo(make());
+
+for (var i = 0; i < 10000; ++i)
+ p2["i" + i] = i;
+p2.f = 43;
+
+for (var i = 0; i < 100; ++i)
+ foo({f:24});
+
+var result = foo(make());
+if (result != 43)
+ throw "Error: bad result: " + result;
</ins></span></pre>
</div>
</div>
</body>
</html>