<!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>[205979] 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/205979">205979</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2016-09-15 10:17:07 -0700 (Thu, 15 Sep 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>There is no good reason for WeakBlock to care about newly allocated objects
https://bugs.webkit.org/show_bug.cgi?id=162006
Reviewed by Geoffrey Garen.
WeakBlock scans itself in two modes:
visit: if a Weak in the block belongs to an unmarked object, ask the Weak to consider whether
it should do things.
reap: if a Weak in a block belongs to an unmarked object, delete the Weak.
Except that "unmarked" has a peculiar meaning: WeakBlock defines it as
!markedOrNewlyAllocated. So, a newly allocated object will never be consulted about anything.
That sounds scary until you realize that newlyAllocated must have been cleared before we even
got here.
So, we were paying the price of checking newlyAllocated for no reason. This switches the code
to using isMarked(). I don't know why the code previously checked newlyAllocated, but I do
trust my reasoning.
* heap/LargeAllocation.h:
(JSC::LargeAllocation::isMarkedDuringWeakVisiting):
(JSC::LargeAllocation::isMarkedOrNewlyAllocatedDuringWeakVisiting): Deleted.
* heap/MarkedBlock.h:
(JSC::MarkedBlock::isMarkedDuringWeakVisiting):
(JSC::MarkedBlock::isMarkedOrNewlyAllocatedDuringWeakVisiting): Deleted.
* heap/WeakBlock.cpp:
(JSC::WeakBlock::specializedVisit):
(JSC::WeakBlock::reap):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapLargeAllocationh">trunk/Source/JavaScriptCore/heap/LargeAllocation.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapMarkedBlockh">trunk/Source/JavaScriptCore/heap/MarkedBlock.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapWeakBlockcpp">trunk/Source/JavaScriptCore/heap/WeakBlock.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (205978 => 205979)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-09-15 17:10:40 UTC (rev 205978)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-09-15 17:17:07 UTC (rev 205979)
</span><span class="lines">@@ -1,3 +1,36 @@
</span><ins>+2016-09-14 Filip Pizlo <fpizlo@apple.com>
+
+ There is no good reason for WeakBlock to care about newly allocated objects
+ https://bugs.webkit.org/show_bug.cgi?id=162006
+
+ Reviewed by Geoffrey Garen.
+
+ WeakBlock scans itself in two modes:
+
+ visit: if a Weak in the block belongs to an unmarked object, ask the Weak to consider whether
+ it should do things.
+
+ reap: if a Weak in a block belongs to an unmarked object, delete the Weak.
+
+ Except that "unmarked" has a peculiar meaning: WeakBlock defines it as
+ !markedOrNewlyAllocated. So, a newly allocated object will never be consulted about anything.
+ That sounds scary until you realize that newlyAllocated must have been cleared before we even
+ got here.
+
+ So, we were paying the price of checking newlyAllocated for no reason. This switches the code
+ to using isMarked(). I don't know why the code previously checked newlyAllocated, but I do
+ trust my reasoning.
+
+ * heap/LargeAllocation.h:
+ (JSC::LargeAllocation::isMarkedDuringWeakVisiting):
+ (JSC::LargeAllocation::isMarkedOrNewlyAllocatedDuringWeakVisiting): Deleted.
+ * heap/MarkedBlock.h:
+ (JSC::MarkedBlock::isMarkedDuringWeakVisiting):
+ (JSC::MarkedBlock::isMarkedOrNewlyAllocatedDuringWeakVisiting): Deleted.
+ * heap/WeakBlock.cpp:
+ (JSC::WeakBlock::specializedVisit):
+ (JSC::WeakBlock::reap):
+
</ins><span class="cx"> 2016-09-15 Commit Queue <commit-queue@webkit.org>
</span><span class="cx">
</span><span class="cx"> Unreviewed, rolling out r205931.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapLargeAllocationh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/LargeAllocation.h (205978 => 205979)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/LargeAllocation.h 2016-09-15 17:10:40 UTC (rev 205978)
+++ trunk/Source/JavaScriptCore/heap/LargeAllocation.h 2016-09-15 17:17:07 UTC (rev 205979)
</span><span class="lines">@@ -72,7 +72,7 @@
</span><span class="cx"> ALWAYS_INLINE bool isMarked() { return m_isMarked.load(std::memory_order_relaxed); }
</span><span class="cx"> bool isMarkedOrNewlyAllocated() { return isMarked() || isNewlyAllocated(); }
</span><span class="cx"> bool isMarkedOrNewlyAllocated(HeapCell*) { return isMarkedOrNewlyAllocated(); }
</span><del>- bool isMarkedOrNewlyAllocatedDuringWeakVisiting(HeapVersion, HeapCell*) { return isMarkedOrNewlyAllocated(); }
</del><ins>+ bool isMarkedDuringWeakVisiting(HeapVersion, HeapCell*) { return isMarked(); }
</ins><span class="cx"> bool isLive() { return isMarkedOrNewlyAllocated(); }
</span><span class="cx">
</span><span class="cx"> bool hasValidCell() const { return m_hasValidCell; }
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapMarkedBlockh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/MarkedBlock.h (205978 => 205979)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/MarkedBlock.h 2016-09-15 17:10:40 UTC (rev 205978)
+++ trunk/Source/JavaScriptCore/heap/MarkedBlock.h 2016-09-15 17:17:07 UTC (rev 205979)
</span><span class="lines">@@ -251,7 +251,8 @@
</span><span class="cx"> bool testAndSetMarked(const void*);
</span><span class="cx">
</span><span class="cx"> bool isMarkedOrNewlyAllocated(const HeapCell*);
</span><del>- bool isMarkedOrNewlyAllocatedDuringWeakVisiting(HeapVersion, const HeapCell*);
</del><ins>+
+ bool isMarkedDuringWeakVisiting(HeapVersion, const HeapCell*);
</ins><span class="cx">
</span><span class="cx"> bool isAtom(const void*);
</span><span class="cx"> void clearMarked(const void*);
</span><span class="lines">@@ -561,11 +562,11 @@
</span><span class="cx"> return isMarked(cell) || (m_handle.m_newlyAllocated && m_handle.isNewlyAllocated(cell));
</span><span class="cx"> }
</span><span class="cx">
</span><del>-inline bool MarkedBlock::isMarkedOrNewlyAllocatedDuringWeakVisiting(HeapVersion heapVersion, const HeapCell* cell)
</del><ins>+inline bool MarkedBlock::isMarkedDuringWeakVisiting(HeapVersion heapVersion, const HeapCell* cell)
</ins><span class="cx"> {
</span><span class="cx"> if (needsFlip(heapVersion))
</span><span class="cx"> return false;
</span><del>- return isMarkedOrNewlyAllocated(cell);
</del><ins>+ return isMarked(cell);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> inline bool MarkedBlock::Handle::isLive(const HeapCell* cell)
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapWeakBlockcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/WeakBlock.cpp (205978 => 205979)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/WeakBlock.cpp 2016-09-15 17:10:40 UTC (rev 205978)
+++ trunk/Source/JavaScriptCore/heap/WeakBlock.cpp 2016-09-15 17:17:07 UTC (rev 205979)
</span><span class="lines">@@ -113,7 +113,7 @@
</span><span class="cx"> continue;
</span><span class="cx">
</span><span class="cx"> const JSValue& jsValue = weakImpl->jsValue();
</span><del>- if (container.isMarkedOrNewlyAllocatedDuringWeakVisiting(version, jsValue.asCell()))
</del><ins>+ if (container.isMarkedDuringWeakVisiting(version, jsValue.asCell()))
</ins><span class="cx"> continue;
</span><span class="cx">
</span><span class="cx"> if (!weakHandleOwner->isReachableFromOpaqueRoots(Handle<Unknown>::wrapSlot(&const_cast<JSValue&>(jsValue)), weakImpl->context(), visitor))
</span><span class="lines">@@ -157,7 +157,7 @@
</span><span class="cx"> if (weakImpl->state() > WeakImpl::Dead)
</span><span class="cx"> continue;
</span><span class="cx">
</span><del>- if (m_container.isMarkedOrNewlyAllocated(weakImpl->jsValue().asCell())) {
</del><ins>+ if (m_container.isMarked(weakImpl->jsValue().asCell())) {
</ins><span class="cx"> ASSERT(weakImpl->state() == WeakImpl::Live);
</span><span class="cx"> continue;
</span><span class="cx"> }
</span></span></pre>
</div>
</div>
</body>
</html>