<!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>[190185] 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/190185">190185</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2015-09-23 13:59:51 -0700 (Wed, 23 Sep 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>Parallel copy phase synchronization should be simplified
https://bugs.webkit.org/show_bug.cgi?id=149509
Reviewed by Mark Lam.
Before this change, we didn't wait for the copy phase to finish before starting to do things to
copied space that presumed that copying was done. Copied space would "detect" that nobody was
copying anymore by waiting for all loaned blocks to be returned. But that would succeed if some
thread had not yet started copying. So, we had weird hacks to ensure that a block was loaned
before any threads started. It also meant that we had two separate mechanisms for waiting for
copying threads to finish - one mechanism in the Heap phase logic and another in the
CopiedSpace::doneCopying() method.
We can get rid of a lot of the weirdness by just having a sound shutdown sequence:
1) Threads concur on when there is no more work. We already have this; once
Heap::getNextBlocksToCopy() returns no work in any thread, it will also return no work in
any other thread that asks for work.
2) Main thread waits for the threads to not be copying anymore.
3) Do whatever we need to do after copying finishes.
Currently, we do (3) before (2) and so we have weird problems. This just changes the code to do
(3) after (2), and so we can get rid of the synchronization in doneCopying() and we can safely
call startCopying() inside GCThread. This also means that we don't need to make CopyVisitor a
property of GCThread. Instead, GCThread just instantiates its own CopyVisitor when it needs to.
* heap/CopiedSpace.cpp:
(JSC::CopiedSpace::doneCopying):
* heap/GCThread.cpp:
(JSC::GCThread::GCThread):
(JSC::GCThread::slotVisitor):
(JSC::GCThread::waitForNextPhase):
(JSC::GCThread::gcThreadMain):
(JSC::GCThread::copyVisitor): Deleted.
* heap/GCThread.h:
* heap/Heap.cpp:
(JSC::Heap::Heap):
(JSC::Heap::copyBackingStores):
(JSC::Heap::gatherStackRoots):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapCopiedSpacecpp">trunk/Source/JavaScriptCore/heap/CopiedSpace.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapCopiedSpaceh">trunk/Source/JavaScriptCore/heap/CopiedSpace.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapCopiedSpaceInlinesh">trunk/Source/JavaScriptCore/heap/CopiedSpaceInlines.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapGCThreadcpp">trunk/Source/JavaScriptCore/heap/GCThread.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapGCThreadh">trunk/Source/JavaScriptCore/heap/GCThread.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapHeapcpp">trunk/Source/JavaScriptCore/heap/Heap.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (190184 => 190185)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-09-23 20:58:51 UTC (rev 190184)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-09-23 20:59:51 UTC (rev 190185)
</span><span class="lines">@@ -1,3 +1,45 @@
</span><ins>+2015-09-23 Filip Pizlo <fpizlo@apple.com>
+
+ Parallel copy phase synchronization should be simplified
+ https://bugs.webkit.org/show_bug.cgi?id=149509
+
+ Reviewed by Mark Lam.
+
+ Before this change, we didn't wait for the copy phase to finish before starting to do things to
+ copied space that presumed that copying was done. Copied space would "detect" that nobody was
+ copying anymore by waiting for all loaned blocks to be returned. But that would succeed if some
+ thread had not yet started copying. So, we had weird hacks to ensure that a block was loaned
+ before any threads started. It also meant that we had two separate mechanisms for waiting for
+ copying threads to finish - one mechanism in the Heap phase logic and another in the
+ CopiedSpace::doneCopying() method.
+
+ We can get rid of a lot of the weirdness by just having a sound shutdown sequence:
+
+ 1) Threads concur on when there is no more work. We already have this; once
+ Heap::getNextBlocksToCopy() returns no work in any thread, it will also return no work in
+ any other thread that asks for work.
+ 2) Main thread waits for the threads to not be copying anymore.
+ 3) Do whatever we need to do after copying finishes.
+
+ Currently, we do (3) before (2) and so we have weird problems. This just changes the code to do
+ (3) after (2), and so we can get rid of the synchronization in doneCopying() and we can safely
+ call startCopying() inside GCThread. This also means that we don't need to make CopyVisitor a
+ property of GCThread. Instead, GCThread just instantiates its own CopyVisitor when it needs to.
+
+ * heap/CopiedSpace.cpp:
+ (JSC::CopiedSpace::doneCopying):
+ * heap/GCThread.cpp:
+ (JSC::GCThread::GCThread):
+ (JSC::GCThread::slotVisitor):
+ (JSC::GCThread::waitForNextPhase):
+ (JSC::GCThread::gcThreadMain):
+ (JSC::GCThread::copyVisitor): Deleted.
+ * heap/GCThread.h:
+ * heap/Heap.cpp:
+ (JSC::Heap::Heap):
+ (JSC::Heap::copyBackingStores):
+ (JSC::Heap::gatherStackRoots):
+
</ins><span class="cx"> 2015-09-23 Joseph Pecoraro <pecoraro@apple.com>
</span><span class="cx">
</span><span class="cx"> Remove unimplemented method Heap::showStatistics
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapCopiedSpacecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/CopiedSpace.cpp (190184 => 190185)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/CopiedSpace.cpp 2015-09-23 20:58:51 UTC (rev 190184)
+++ trunk/Source/JavaScriptCore/heap/CopiedSpace.cpp 2015-09-23 20:59:51 UTC (rev 190185)
</span><span class="lines">@@ -202,8 +202,6 @@
</span><span class="cx"> ASSERT(m_numberOfLoanedBlocks > 0);
</span><span class="cx"> ASSERT(m_inCopyingPhase);
</span><span class="cx"> m_numberOfLoanedBlocks--;
</span><del>- if (!m_numberOfLoanedBlocks)
- m_loanedBlocksCondition.notifyOne();
</del><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -230,13 +228,8 @@
</span><span class="cx">
</span><span class="cx"> void CopiedSpace::doneCopying()
</span><span class="cx"> {
</span><del>- {
- LockHolder locker(m_loanedBlocksLock);
- while (m_numberOfLoanedBlocks > 0)
- m_loanedBlocksCondition.wait(m_loanedBlocksLock);
- }
-
- ASSERT(m_inCopyingPhase == m_shouldDoCopyPhase);
</del><ins>+ RELEASE_ASSERT(!m_numberOfLoanedBlocks);
+ RELEASE_ASSERT(m_inCopyingPhase == m_shouldDoCopyPhase);
</ins><span class="cx"> m_inCopyingPhase = false;
</span><span class="cx">
</span><span class="cx"> DoublyLinkedList<CopiedBlock>* toSpace;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapCopiedSpaceh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/CopiedSpace.h (190184 => 190185)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/CopiedSpace.h 2015-09-23 20:58:51 UTC (rev 190184)
+++ trunk/Source/JavaScriptCore/heap/CopiedSpace.h 2015-09-23 20:59:51 UTC (rev 190185)
</span><span class="lines">@@ -139,7 +139,6 @@
</span><span class="cx"> bool m_shouldDoCopyPhase;
</span><span class="cx">
</span><span class="cx"> Lock m_loanedBlocksLock;
</span><del>- Condition m_loanedBlocksCondition;
</del><span class="cx"> size_t m_numberOfLoanedBlocks;
</span><span class="cx">
</span><span class="cx"> size_t m_bytesRemovedFromOldSpaceDueToReallocation;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapCopiedSpaceInlinesh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/CopiedSpaceInlines.h (190184 => 190185)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/CopiedSpaceInlines.h 2015-09-23 20:58:51 UTC (rev 190184)
+++ trunk/Source/JavaScriptCore/heap/CopiedSpaceInlines.h 2015-09-23 20:59:51 UTC (rev 190185)
</span><span class="lines">@@ -117,8 +117,6 @@
</span><span class="cx"> ASSERT(m_numberOfLoanedBlocks > 0);
</span><span class="cx"> ASSERT(m_inCopyingPhase);
</span><span class="cx"> m_numberOfLoanedBlocks--;
</span><del>- if (!m_numberOfLoanedBlocks)
- m_loanedBlocksCondition.notifyOne();
</del><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapGCThreadcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/GCThread.cpp (190184 => 190185)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/GCThread.cpp 2015-09-23 20:58:51 UTC (rev 190184)
+++ trunk/Source/JavaScriptCore/heap/GCThread.cpp 2015-09-23 20:59:51 UTC (rev 190185)
</span><span class="lines">@@ -34,11 +34,10 @@
</span><span class="cx">
</span><span class="cx"> namespace JSC {
</span><span class="cx">
</span><del>-GCThread::GCThread(Heap& heap, std::unique_ptr<SlotVisitor> slotVisitor, std::unique_ptr<CopyVisitor> copyVisitor)
</del><ins>+GCThread::GCThread(Heap& heap, std::unique_ptr<SlotVisitor> slotVisitor)
</ins><span class="cx"> : m_threadID(0)
</span><span class="cx"> , m_heap(heap)
</span><span class="cx"> , m_slotVisitor(WTF::move(slotVisitor))
</span><del>- , m_copyVisitor(WTF::move(copyVisitor))
</del><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -60,12 +59,6 @@
</span><span class="cx"> return m_slotVisitor.get();
</span><span class="cx"> }
</span><span class="cx">
</span><del>-CopyVisitor* GCThread::copyVisitor()
-{
- ASSERT(m_copyVisitor);
- return m_copyVisitor.get();
-}
-
</del><span class="cx"> GCPhase GCThread::waitForNextPhase()
</span><span class="cx"> {
</span><span class="cx"> std::unique_lock<Lock> lock(m_heap.m_phaseMutex);
</span><span class="lines">@@ -102,20 +95,21 @@
</span><span class="cx"> // that all of the various subphases in Heap::markRoots() have been fully finished and there is
</span><span class="cx"> // no more marking work to do and all of the GCThreads are idle, meaning no more work can be generated.
</span><span class="cx"> break;
</span><del>- case Copy:
- // We don't have to call startCopying() because it's called for us on the main thread to avoid a
- // race condition.
- m_copyVisitor->copyFromShared();
</del><ins>+ case Copy: {
+ CopyVisitor copyVisitor(m_heap);
+ copyVisitor.startCopying();
+ copyVisitor.copyFromShared();
</ins><span class="cx"> // We know we're done copying when we return from copyFromShared() because we would
</span><span class="cx"> // only do so if there were no more chunks of copying work left to do. When there is no
</span><span class="cx"> // more copying work to do, the main thread will wait in CopiedSpace::doneCopying() until
</span><span class="cx"> // all of the blocks that the GCThreads borrowed have been returned. doneCopying()
</span><span class="cx"> // returns our borrowed CopiedBlock, allowing the copying phase to finish.
</span><del>- m_copyVisitor->doneCopying();
</del><ins>+ copyVisitor.doneCopying();
</ins><span class="cx">
</span><span class="cx"> WTF::releaseFastMallocFreeMemoryForThisThread();
</span><span class="cx">
</span><span class="cx"> break;
</span><ins>+ }
</ins><span class="cx"> case NoPhase:
</span><span class="cx"> RELEASE_ASSERT_NOT_REACHED();
</span><span class="cx"> break;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapGCThreadh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/GCThread.h (190184 => 190185)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/GCThread.h 2015-09-23 20:58:51 UTC (rev 190184)
+++ trunk/Source/JavaScriptCore/heap/GCThread.h 2015-09-23 20:59:51 UTC (rev 190185)
</span><span class="lines">@@ -31,7 +31,6 @@
</span><span class="cx">
</span><span class="cx"> namespace JSC {
</span><span class="cx">
</span><del>-class CopyVisitor;
</del><span class="cx"> class Heap;
</span><span class="cx"> class SlotVisitor;
</span><span class="cx">
</span><span class="lines">@@ -44,10 +43,9 @@
</span><span class="cx">
</span><span class="cx"> class GCThread {
</span><span class="cx"> public:
</span><del>- GCThread(Heap&, std::unique_ptr<SlotVisitor>, std::unique_ptr<CopyVisitor>);
</del><ins>+ GCThread(Heap&, std::unique_ptr<SlotVisitor>);
</ins><span class="cx">
</span><span class="cx"> SlotVisitor* slotVisitor();
</span><del>- CopyVisitor* copyVisitor();
</del><span class="cx"> ThreadIdentifier threadID();
</span><span class="cx"> void initializeThreadID(ThreadIdentifier);
</span><span class="cx">
</span><span class="lines">@@ -60,7 +58,6 @@
</span><span class="cx"> ThreadIdentifier m_threadID;
</span><span class="cx"> Heap& m_heap;
</span><span class="cx"> std::unique_ptr<SlotVisitor> m_slotVisitor;
</span><del>- std::unique_ptr<CopyVisitor> m_copyVisitor;
</del><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> } // namespace JSC
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapHeapcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/Heap.cpp (190184 => 190185)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/Heap.cpp 2015-09-23 20:58:51 UTC (rev 190184)
+++ trunk/Source/JavaScriptCore/heap/Heap.cpp 2015-09-23 20:59:51 UTC (rev 190185)
</span><span class="lines">@@ -362,7 +362,7 @@
</span><span class="cx"> std::unique_lock<Lock> lock(m_phaseMutex);
</span><span class="cx"> for (unsigned i = 1; i < Options::numberOfGCMarkers(); ++i) {
</span><span class="cx"> m_numberOfActiveGCThreads++;
</span><del>- GCThread* newThread = new GCThread(*this, std::make_unique<SlotVisitor>(*this), std::make_unique<CopyVisitor>(*this));
</del><ins>+ GCThread* newThread = new GCThread(*this, std::make_unique<SlotVisitor>(*this));
</ins><span class="cx"> ThreadIdentifier threadID = createThread(GCThread::gcThreadStartFunc, newThread, "JavaScriptCore::Marking");
</span><span class="cx"> newThread->initializeThreadID(threadID);
</span><span class="cx"> m_gcThreads.append(newThread);
</span><span class="lines">@@ -634,25 +634,17 @@
</span><span class="cx"> m_copyIndex = 0;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- // We do this here so that we avoid a race condition where the main thread can
- // blow through all of the copying work before the GCThreads fully wake up.
- // The GCThreads then request a block from the CopiedSpace when the copying phase
- // has completed, which isn't allowed.
- for (size_t i = 0; i < m_gcThreads.size(); i++)
- m_gcThreads[i]->copyVisitor()->startCopying();
-
</del><span class="cx"> startNextPhase(Copy);
</span><span class="cx">
</span><span class="cx"> m_copyVisitor.startCopying();
</span><span class="cx"> m_copyVisitor.copyFromShared();
</span><span class="cx"> m_copyVisitor.doneCopying();
</span><del>- // We need to wait for everybody to finish and return their CopiedBlocks
- // before signaling that the phase is complete.
- m_storageSpace.doneCopying();
</del><ins>+
</ins><span class="cx"> ASSERT(m_currentPhase == Copy);
</span><span class="cx"> endCurrentPhase();
</span><del>- } else
- m_storageSpace.doneCopying();
</del><ins>+ }
+
+ m_storageSpace.doneCopying();
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void Heap::gatherStackRoots(ConservativeRoots& roots, void* stackOrigin, void* stackTop, MachineThreads::RegisterState& calleeSavedRegisters)
</span></span></pre>
</div>
</div>
</body>
</html>