Changeset 119364 in webkit

Timestamp:

Jun 3, 2012 2:16:55 PM (12 years ago)

Author:

ggaren@apple.com

Message:

Weak pointer finalization should be lazy
​https://bugs.webkit.org/show_bug.cgi?id=87599

Reviewed by Sam Weinig.

This time for sure!

• heap/Heap.cpp:

(JSC::Heap::collect): Don't sweep eagerly -- we'll sweep lazily instead.

• heap/MarkedBlock.cpp:

(JSC::MarkedBlock::sweep): Sweep our weak set before we sweep our other
destructors -- this is our last chance to run weak set finalizers before
we recycle our memory.

• heap/MarkedBlock.h:

(JSC::MarkedBlock::resetAllocator):

• heap/MarkedSpace.cpp:

(JSC::MarkedSpace::resetAllocators):

• heap/MarkedSpace.h:

(JSC::MarkedSpace::resetAllocators): Don't force allocator reset anymore.
It will happen automatically when a weak set is swept. It's simpler to
have only one canonical way for this to happen, and it wasn't buying
us anything to do it eagerly.

• heap/WeakBlock.cpp:

(JSC::WeakBlock::sweep): Don't short-circuit a sweep unless we know
the sweep would be a no-op. If even one finalizer is pending, we need to
run it, since we won't get another chance.

• heap/WeakSet.cpp:

(JSC::WeakSet::sweep): This loop can be simpler now that
WeakBlock::sweep() does what we mean.

Reset our allocator after a sweep because this is the optimal time to
start trying to recycle old weak pointers.

(JSC::WeakSet::tryFindAllocator): Don't sweep when searching for an
allocator because we've swept already, and forcing a new sweep would be
wasteful.

• heap/WeakSet.h:

(JSC::WeakSet::shrink): Be sure to reset our allocator after a shrink
because the shrink may have removed the block the allocator was going to
allocate out of.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/Heap.cpp (modified) (1 diff)
• heap/MarkedBlock.cpp (modified) (1 diff)
• heap/MarkedBlock.h (modified) (2 diffs)
• heap/MarkedSpace.cpp (modified) (3 diffs)
• heap/MarkedSpace.h (modified) (1 diff)
• heap/WeakBlock.cpp (modified) (1 diff)
• heap/WeakSet.cpp (modified) (2 diffs)
• heap/WeakSet.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2012-06-02  Geoffrey Garen  <ggaren@apple.com>
+
+        Weak pointer finalization should be lazy
+        https://bugs.webkit.org/show_bug.cgi?id=87599
+
+        Reviewed by Sam Weinig.
+
+        This time for sure!
+
+        * heap/Heap.cpp:
+        (JSC::Heap::collect): Don't sweep eagerly -- we'll sweep lazily instead.
+
+        * heap/MarkedBlock.cpp:
+        (JSC::MarkedBlock::sweep): Sweep our weak set before we sweep our other
+        destructors -- this is our last chance to run weak set finalizers before
+        we recycle our memory.
+
+        * heap/MarkedBlock.h:
+        (JSC::MarkedBlock::resetAllocator):
+        * heap/MarkedSpace.cpp:
+        (JSC::MarkedSpace::resetAllocators):
+        * heap/MarkedSpace.h:
+        (JSC::MarkedSpace::resetAllocators): Don't force allocator reset anymore.
+        It will happen automatically when a weak set is swept. It's simpler to
+        have only one canonical way for this to happen, and it wasn't buying
+        us anything to do it eagerly.
+
+        * heap/WeakBlock.cpp:
+        (JSC::WeakBlock::sweep): Don't short-circuit a sweep unless we know
+        the sweep would be a no-op. If even one finalizer is pending, we need to
+        run it, since we won't get another chance.
+
+        * heap/WeakSet.cpp:
+        (JSC::WeakSet::sweep): This loop can be simpler now that
+        WeakBlock::sweep() does what we mean.
+
+        Reset our allocator after a sweep because this is the optimal time to
+        start trying to recycle old weak pointers.
+
+        (JSC::WeakSet::tryFindAllocator): Don't sweep when searching for an
+        allocator because we've swept already, and forcing a new sweep would be
+        wasteful.
+
+        * heap/WeakSet.h:
+        (JSC::WeakSet::shrink): Be sure to reset our allocator after a shrink
+        because the shrink may have removed the block the allocator was going to
+        allocate out of.
+
 2012-06-02  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/heap/Heap.cpp

@@ -689 +689 @@
 
     {
-        GCPHASE(FinalizeWeakHandles);
-        m_objectSpace.sweepWeakSets();
+        GCPHASE(FinalizeSmallString);
         m_globalData->smallStrings.finalizeSmallStrings();
     }

trunk/Source/JavaScriptCore/heap/MarkedBlock.cpp

@@ -115 +115 @@
     HEAP_LOG_BLOCK_STATE_TRANSITION(this);
 
+    m_weakSet.sweep();
+
     if (sweepMode == SweepOnly && !m_cellsNeedDestruction)
         return FreeList();

trunk/Source/JavaScriptCore/heap/MarkedBlock.h

@@ -130 +130 @@
 
         void shrink();
-        void resetAllocator();
 
         void visitWeakSet(HeapRootVisitor&);
@@ -273 +272 @@
     {
         m_weakSet.shrink();
-    }
-
-    inline void MarkedBlock::resetAllocator()
-    {
-        m_weakSet.resetAllocator();
     }
 

trunk/Source/JavaScriptCore/heap/MarkedSpace.cpp

@@ -113 +113 @@
 }
 
-struct ResetAllocator : MarkedBlock::VoidFunctor {
-    void operator()(MarkedBlock* block) { block->resetAllocator(); }
-};
-
 void MarkedSpace::resetAllocators()
 {
@@ -128 +124 @@
         destructorAllocatorFor(cellSize).reset();
     }
-
-    forEachBlock<ResetAllocator>();
 }
 
@@ -141 +135 @@
 {
     forEachBlock<ReapWeakSet>();
-}
-
-void MarkedSpace::sweepWeakSets()
-{
-    forEachBlock<SweepWeakSet>();
 }
 

trunk/Source/JavaScriptCore/heap/MarkedSpace.h

@@ -86 +86 @@
     void visitWeakSets(HeapRootVisitor&);
     void reapWeakSets();
-    void sweepWeakSets();
 
     MarkedBlockSet& blocks() { return m_blocks; }

trunk/Source/JavaScriptCore/heap/WeakBlock.cpp

@@ -70 +70 @@
 void WeakBlock::sweep()
 {
-    if (!m_sweepResult.isNull())
+    // If a block is completely empty, a sweep won't have any effect.
+    if (isEmpty())
         return;
 

trunk/Source/JavaScriptCore/heap/WeakSet.cpp

@@ -43 +43 @@
 void WeakSet::sweep()
 {
-    WeakBlock* next;
-    for (WeakBlock* block = m_blocks.head(); block; block = next) {
-        next = block->next();
+    for (WeakBlock* block = m_blocks.head(); block; block = block->next())
+        block->sweep();
 
-        // If a block is completely empty, a new sweep won't have any effect.
-        if (block->isEmpty())
-            continue;
-
-        block->takeSweepResult(); // Force a new sweep by discarding the last sweep.
-        block->sweep();
-    }
+    resetAllocator();
 }
 
@@ -70 +63 @@
         m_nextAllocator = m_nextAllocator->next();
 
-        block->sweep();
         WeakBlock::SweepResult sweepResult = block->takeSweepResult();
         if (sweepResult.freeList)

trunk/Source/JavaScriptCore/heap/WeakSet.h

@@ -119 +119 @@
             removeAllocator(block);
     }
+
+    resetAllocator();
 }