Eden collections should extend the IncrementalSweeper work list, not replace it.
authorakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 May 2015 20:30:42 +0000 (20:30 +0000)
committerakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 May 2015 20:30:42 +0000 (20:30 +0000)
<https://webkit.org/b/145213>
<rdar://problem/21002666>

Reviewed by Geoffrey Garen.

After an eden collection, the garbage collector was adding all MarkedBlocks containing
new objects to the IncrementalSweeper's work list, to make sure they didn't have to
wait until the next full collection before getting swept.

Or at least, that's what it thought it was doing. It turns out that IncrementalSweeper's
internal work list is really just a reference to Heap::m_blockSnapshot. I didn't realize
this when writing the post-eden sweep code, and instead made eden collections cancel
all pending sweeps and *replace* them with the list of blocks with new objects.

This made it so that rapidly occurring eden collections could prevent large numbers of
heap blocks from ever getting swept. This would manifest as accumulation of MarkedBlocks
when a system under heavy load was also allocating short lived objects at a high rate.
Things would eventually get cleaned up when there was a lull and a full collection was
allowed to run its heap sweep to completion.

Fix this by moving all management of the block snapshot to Heap. snapshotMarkedSpace()
now handles eden collections by merging the list of blocks with new objects into the
existing block snapshot.

* heap/Heap.cpp:
(JSC::Heap::snapshotMarkedSpace):
(JSC::Heap::notifyIncrementalSweeper):
* heap/IncrementalSweeper.cpp:
(JSC::IncrementalSweeper::startSweeping):
(JSC::IncrementalSweeper::addBlocksAndContinueSweeping): Deleted.
* heap/IncrementalSweeper.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@184652 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/IncrementalSweeper.cpp
Source/JavaScriptCore/heap/IncrementalSweeper.h

index 50b2109770d0f38159b4d27682e97978ad3ca482..8e0b1f0d87fb462ed4ce8b54234d246412c82669 100644 (file)
@@ -1,3 +1,38 @@
+2015-05-20  Andreas Kling  <akling@apple.com>
+
+        Eden collections should extend the IncrementalSweeper work list, not replace it.
+        <https://webkit.org/b/145213>
+        <rdar://problem/21002666>
+
+        Reviewed by Geoffrey Garen.
+
+        After an eden collection, the garbage collector was adding all MarkedBlocks containing
+        new objects to the IncrementalSweeper's work list, to make sure they didn't have to
+        wait until the next full collection before getting swept.
+
+        Or at least, that's what it thought it was doing. It turns out that IncrementalSweeper's
+        internal work list is really just a reference to Heap::m_blockSnapshot. I didn't realize
+        this when writing the post-eden sweep code, and instead made eden collections cancel
+        all pending sweeps and *replace* them with the list of blocks with new objects.
+
+        This made it so that rapidly occurring eden collections could prevent large numbers of
+        heap blocks from ever getting swept. This would manifest as accumulation of MarkedBlocks
+        when a system under heavy load was also allocating short lived objects at a high rate.
+        Things would eventually get cleaned up when there was a lull and a full collection was
+        allowed to run its heap sweep to completion.
+
+        Fix this by moving all management of the block snapshot to Heap. snapshotMarkedSpace()
+        now handles eden collections by merging the list of blocks with new objects into the
+        existing block snapshot.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::snapshotMarkedSpace):
+        (JSC::Heap::notifyIncrementalSweeper):
+        * heap/IncrementalSweeper.cpp:
+        (JSC::IncrementalSweeper::startSweeping):
+        (JSC::IncrementalSweeper::addBlocksAndContinueSweeping): Deleted.
+        * heap/IncrementalSweeper.h:
+
 2015-05-20  Youenn Fablet  <youenn.fablet@crf.canon.fr>
 
         AudioContext resume/close/suspend should reject promises with a DOM exception in lieu of throwing exceptions
index b7f2f048e492d05083247e0a83e4259acaa853c3..e4309fc0b8e7c2d7bb20f6eadf3d1506346e31a2 100644 (file)
@@ -1212,9 +1212,12 @@ void Heap::snapshotMarkedSpace()
 {
     GCPHASE(SnapshotMarkedSpace);
 
-    if (m_operationInProgress == EdenCollection)
-        m_blockSnapshot = m_objectSpace.blocksWithNewObjects();
-    else {
+    if (m_operationInProgress == EdenCollection) {
+        m_blockSnapshot.appendVector(m_objectSpace.blocksWithNewObjects());
+        // Sort and deduplicate the block snapshot since we might be appending to an unfinished work list.
+        std::sort(m_blockSnapshot.begin(), m_blockSnapshot.end());
+        m_blockSnapshot.shrink(std::unique(m_blockSnapshot.begin(), m_blockSnapshot.end()) - m_blockSnapshot.begin());
+    } else {
         m_blockSnapshot.resizeToFit(m_objectSpace.blocks().set().size());
         MarkedBlockSnapshotFunctor functor(m_blockSnapshot);
         m_objectSpace.forEachBlock(functor);
@@ -1230,13 +1233,13 @@ void Heap::deleteSourceProviderCaches()
 void Heap::notifyIncrementalSweeper()
 {
     GCPHASE(NotifyIncrementalSweeper);
-    if (m_operationInProgress == EdenCollection)
-        m_sweeper->addBlocksAndContinueSweeping(WTF::move(m_blockSnapshot));
-    else {
+
+    if (m_operationInProgress == FullCollection) {
         if (!m_logicallyEmptyWeakBlocks.isEmpty())
             m_indexOfNextLogicallyEmptyWeakBlockToSweep = 0;
-        m_sweeper->startSweeping(WTF::move(m_blockSnapshot));
     }
+
+    m_sweeper->startSweeping();
 }
 
 void Heap::rememberCurrentlyExecutingCodeBlocks()
index 3caf81e8dd5e99361988adc5b38d3cbbfcacbe07..3ff4d8c3a0406ffd6de99bd88cdd473da41b88d1 100644 (file)
@@ -101,18 +101,8 @@ bool IncrementalSweeper::sweepNextBlock()
     return m_vm->heap.sweepNextLogicallyEmptyWeakBlock();
 }
 
-void IncrementalSweeper::startSweeping(Vector<MarkedBlock*>&& blockSnapshot)
+void IncrementalSweeper::startSweeping()
 {
-    m_blocksToSweep = WTF::move(blockSnapshot);
-    scheduleTimer();
-}
-
-void IncrementalSweeper::addBlocksAndContinueSweeping(Vector<MarkedBlock*>&& blockSnapshot)
-{
-    Vector<MarkedBlock*> blocks = WTF::move(blockSnapshot);
-    m_blocksToSweep.appendVector(blocks);
-    std::sort(m_blocksToSweep.begin(), m_blocksToSweep.end());
-    m_blocksToSweep.shrink(std::unique(m_blocksToSweep.begin(), m_blocksToSweep.end()) - m_blocksToSweep.begin());
     scheduleTimer();
 }
 
@@ -134,11 +124,7 @@ void IncrementalSweeper::doWork()
 {
 }
 
-void IncrementalSweeper::startSweeping(Vector<MarkedBlock*>&&)
-{
-}
-
-void IncrementalSweeper::addBlocksAndContinueSweeping(Vector<MarkedBlock*>&&)
+void IncrementalSweeper::startSweeping()
 {
 }
 
index fcea92bd4fb454c92a835a3f8094075d7015771c..f813a36faac8d5336901f83617dca392a58acd6e 100644 (file)
@@ -44,8 +44,7 @@ public:
     explicit IncrementalSweeper(VM*);
 #endif
 
-    void startSweeping(Vector<MarkedBlock*>&&);
-    void addBlocksAndContinueSweeping(Vector<MarkedBlock*>&&);
+    void startSweeping();
 
     JS_EXPORT_PRIVATE virtual void doWork() override;
     bool sweepNextBlock();