Refactored heap tear-down to use normal value semantics (i.e., destructors)
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 May 2012 20:47:46 +0000 (20:47 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 May 2012 20:47:46 +0000 (20:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=87302

Reviewed by Oliver Hunt.

This is a step toward incremental DOM finalization.

* heap/CopiedSpace.cpp:
(JSC::CopiedSpace::~CopiedSpace):
* heap/CopiedSpace.h:
(CopiedSpace): Just use our destructor, instead of relying on the heap
to send us a special message at a special time.

* heap/Heap.cpp:
(JSC::Heap::Heap): Use OwnPtr for m_markListSet because this is not Sparta.

(JSC::Heap::~Heap): No need for delete or freeAllBlocks because normal
destructors do this work automatically now.

(JSC::Heap::lastChanceToFinalize): Just call lastChanceToFinalize on our
sub-objects, and assume it does the right thing. This improves encapsulation,
so we can add items requiring finalization to our sub-objects.

* heap/Heap.h: Moved m_blockAllocator to get the right destruction order.

* heap/MarkedSpace.cpp:
(Take):
(JSC):
(JSC::Take::Take):
(JSC::Take::operator()):
(JSC::Take::returnValue): Moved to the top of the file so it can be used
in another function.

(JSC::MarkedSpace::~MarkedSpace): Delete all outstanding memory, like a good
destructor should.

(JSC::MarkedSpace::lastChanceToFinalize): Moved some code here from the heap,
since it pertains to our internal implementation details.

* heap/MarkedSpace.h:
(MarkedSpace):
* heap/WeakBlock.cpp:
(JSC::WeakBlock::lastChanceToFinalize):
* heap/WeakBlock.h:
(WeakBlock):
* heap/WeakSet.cpp:
(JSC::WeakSet::lastChanceToFinalize):
* heap/WeakSet.h:
(WeakSet): Stop using a special freeAllBlocks() callback and just implement
lastChanceToFinalize.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/CopiedSpace.cpp
Source/JavaScriptCore/heap/CopiedSpace.h
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/heap/MarkedSpace.cpp
Source/JavaScriptCore/heap/MarkedSpace.h
Source/JavaScriptCore/heap/WeakBlock.cpp
Source/JavaScriptCore/heap/WeakBlock.h
Source/JavaScriptCore/heap/WeakSet.cpp
Source/JavaScriptCore/heap/WeakSet.h

index 4554d7a..aa5c29b 100644 (file)
@@ -1,3 +1,56 @@
+2012-05-23  Geoffrey Garen  <ggaren@apple.com>
+
+        Refactored heap tear-down to use normal value semantics (i.e., destructors)
+        https://bugs.webkit.org/show_bug.cgi?id=87302
+
+        Reviewed by Oliver Hunt.
+
+        This is a step toward incremental DOM finalization.
+
+        * heap/CopiedSpace.cpp:
+        (JSC::CopiedSpace::~CopiedSpace):
+        * heap/CopiedSpace.h:
+        (CopiedSpace): Just use our destructor, instead of relying on the heap
+        to send us a special message at a special time.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::Heap): Use OwnPtr for m_markListSet because this is not Sparta.
+
+        (JSC::Heap::~Heap): No need for delete or freeAllBlocks because normal
+        destructors do this work automatically now.
+
+        (JSC::Heap::lastChanceToFinalize): Just call lastChanceToFinalize on our
+        sub-objects, and assume it does the right thing. This improves encapsulation,
+        so we can add items requiring finalization to our sub-objects.
+
+        * heap/Heap.h: Moved m_blockAllocator to get the right destruction order.
+
+        * heap/MarkedSpace.cpp:
+        (Take):
+        (JSC):
+        (JSC::Take::Take):
+        (JSC::Take::operator()):
+        (JSC::Take::returnValue): Moved to the top of the file so it can be used
+        in another function.
+
+        (JSC::MarkedSpace::~MarkedSpace): Delete all outstanding memory, like a good
+        destructor should.
+
+        (JSC::MarkedSpace::lastChanceToFinalize): Moved some code here from the heap,
+        since it pertains to our internal implementation details.
+
+        * heap/MarkedSpace.h:
+        (MarkedSpace):
+        * heap/WeakBlock.cpp:
+        (JSC::WeakBlock::lastChanceToFinalize):
+        * heap/WeakBlock.h:
+        (WeakBlock):
+        * heap/WeakSet.cpp:
+        (JSC::WeakSet::lastChanceToFinalize):
+        * heap/WeakSet.h:
+        (WeakSet): Stop using a special freeAllBlocks() callback and just implement
+        lastChanceToFinalize.
+
 2011-05-22  Geoffrey Garen  <ggaren@apple.com>
 
         Encapsulated some calculations for whether portions of the heap are empty
index 2906e1d..7f5a665 100644 (file)
@@ -40,6 +40,18 @@ CopiedSpace::CopiedSpace(Heap* heap)
 {
 }
 
+CopiedSpace::~CopiedSpace()
+{
+    while (!m_toSpace->isEmpty())
+        m_heap->blockAllocator().deallocate(CopiedBlock::destroy(static_cast<CopiedBlock*>(m_toSpace->removeHead())));
+
+    while (!m_fromSpace->isEmpty())
+        m_heap->blockAllocator().deallocate(CopiedBlock::destroy(static_cast<CopiedBlock*>(m_fromSpace->removeHead())));
+
+    while (!m_oversizeBlocks.isEmpty())
+        CopiedBlock::destroy(static_cast<CopiedBlock*>(m_oversizeBlocks.removeHead())).deallocate();
+}
+
 void CopiedSpace::init()
 {
     m_toSpace = &m_blocks1;
@@ -239,18 +251,6 @@ CheckedBoolean CopiedSpace::getFreshBlock(AllocationEffort allocationEffort, Cop
     return true;
 }
 
-void CopiedSpace::freeAllBlocks()
-{
-    while (!m_toSpace->isEmpty())
-        m_heap->blockAllocator().deallocate(CopiedBlock::destroy(static_cast<CopiedBlock*>(m_toSpace->removeHead())));
-
-    while (!m_fromSpace->isEmpty())
-        m_heap->blockAllocator().deallocate(CopiedBlock::destroy(static_cast<CopiedBlock*>(m_fromSpace->removeHead())));
-
-    while (!m_oversizeBlocks.isEmpty())
-        CopiedBlock::destroy(static_cast<CopiedBlock*>(m_oversizeBlocks.removeHead())).deallocate();
-}
-
 size_t CopiedSpace::size()
 {
     size_t calculatedSize = 0;
index 438df45..2701178 100644 (file)
@@ -50,6 +50,7 @@ class CopiedSpace {
     friend class JIT;
 public:
     CopiedSpace(Heap*);
+    ~CopiedSpace();
     void init();
 
     CheckedBoolean tryAllocate(size_t, void**);
@@ -70,7 +71,6 @@ public:
     size_t size();
     size_t capacity();
 
-    void freeAllBlocks();
     bool isPagedOut(double deadline);
 
     static CopiedBlock* blockFor(void*);
index 611c75a..a43af8a 100644 (file)
@@ -244,7 +244,6 @@ Heap::Heap(JSGlobalData* globalData, HeapType heapType)
     , m_operationInProgress(NoOperation)
     , m_objectSpace(this)
     , m_storageSpace(this)
-    , m_markListSet(0)
     , m_activityCallback(DefaultGCActivityCallback::create(this))
     , m_machineThreads(this)
     , m_sharedData(globalData)
@@ -261,13 +260,6 @@ Heap::Heap(JSGlobalData* globalData, HeapType heapType)
 
 Heap::~Heap()
 {
-    delete m_markListSet;
-
-    m_objectSpace.freeAllBlocks();
-    m_storageSpace.freeAllBlocks();
-
-    ASSERT(!size());
-    ASSERT(!capacity());
 }
 
 bool Heap::isPagedOut(double deadline)
@@ -286,11 +278,8 @@ void Heap::lastChanceToFinalize()
     if (size_t size = m_protectedValues.size())
         WTFLogAlways("ERROR: JavaScriptCore heap deallocated while %ld values were still protected", static_cast<unsigned long>(size));
 
-    m_weakSet.finalizeAll();
-    m_objectSpace.canonicalizeCellLivenessData();
-    m_objectSpace.clearMarks();
-    m_objectSpace.sweep();
-    m_globalData->smallStrings.finalizeSmallStrings();
+    m_weakSet.lastChanceToFinalize();
+    m_objectSpace.lastChanceToFinalize();
 
 #if ENABLE(SIMPLE_HEAP_PROFILING)
     m_slotVisitor.m_visitedTypeCounts.dump(WTF::dataFile(), "Visited Type Counts");
index dc681e8..50e1ea8 100644 (file)
@@ -143,7 +143,7 @@ namespace JSC {
         void pushTempSortVector(Vector<ValueStringPair>*);
         void popTempSortVector(Vector<ValueStringPair>*);
     
-        HashSet<MarkedArgumentBuffer*>& markListSet() { if (!m_markListSet) m_markListSet = new HashSet<MarkedArgumentBuffer*>; return *m_markListSet; }
+        HashSet<MarkedArgumentBuffer*>& markListSet() { if (!m_markListSet) m_markListSet = adoptPtr(new HashSet<MarkedArgumentBuffer*>); return *m_markListSet; }
         
         template<typename Functor> typename Functor::ReturnType forEachProtectedCell(Functor&);
         template<typename Functor> typename Functor::ReturnType forEachProtectedCell();
@@ -206,18 +206,17 @@ namespace JSC {
         size_t m_bytesAbandoned;
         
         OperationInProgress m_operationInProgress;
+        BlockAllocator m_blockAllocator;
         MarkedSpace m_objectSpace;
         CopiedSpace m_storageSpace;
 
-        BlockAllocator m_blockAllocator;
-
 #if ENABLE(SIMPLE_HEAP_PROFILING)
         VTableSpectrum m_destroyedTypeCounts;
 #endif
 
         ProtectCountSet m_protectedValues;
         Vector<Vector<ValueStringPair>* > m_tempSortingVectors;
-        HashSet<MarkedArgumentBuffer*>* m_markListSet;
+        OwnPtr<HashSet<MarkedArgumentBuffer*> > m_markListSet;
 
         OwnPtr<GCActivityCallback> m_activityCallback;
         
index 509deb8..8ccaefc 100644 (file)
@@ -30,6 +30,42 @@ namespace JSC {
 
 class Structure;
 
+class Take {
+public:
+    typedef MarkedBlock* ReturnType;
+
+    enum TakeMode { TakeIfEmpty, TakeAll };
+
+    Take(TakeMode, MarkedSpace*);
+    void operator()(MarkedBlock*);
+    ReturnType returnValue();
+    
+private:
+    TakeMode m_takeMode;
+    MarkedSpace* m_markedSpace;
+    DoublyLinkedList<MarkedBlock> m_blocks;
+};
+
+inline Take::Take(TakeMode takeMode, MarkedSpace* newSpace)
+    : m_takeMode(takeMode)
+    , m_markedSpace(newSpace)
+{
+}
+
+inline void Take::operator()(MarkedBlock* block)
+{
+    if (m_takeMode == TakeIfEmpty && !block->isEmpty())
+        return;
+    
+    m_markedSpace->allocatorFor(block).removeBlock(block);
+    m_blocks.append(block);
+}
+
+inline Take::ReturnType Take::returnValue()
+{
+    return m_blocks.head();
+}
+
 MarkedSpace::MarkedSpace(Heap* heap)
     : m_heap(heap)
 {
@@ -44,6 +80,20 @@ MarkedSpace::MarkedSpace(Heap* heap)
     }
 }
 
+MarkedSpace::~MarkedSpace()
+{
+    // We record a temporary list of empties to avoid modifying m_blocks while iterating it.
+    Take take(Take::TakeAll, this);
+    freeBlocks(forEachBlock(take));
+}
+
+void MarkedSpace::lastChanceToFinalize()
+{
+    canonicalizeCellLivenessData();
+    clearMarks();
+    sweep();
+}
+
 void MarkedSpace::resetAllocators()
 {
     for (size_t cellSize = preciseStep; cellSize <= preciseCutoff; cellSize += preciseStep) {
@@ -98,42 +148,6 @@ void MarkedSpace::freeBlocks(MarkedBlock* head)
     }
 }
 
-class Take {
-public:
-    typedef MarkedBlock* ReturnType;
-
-    enum TakeMode { TakeIfEmpty, TakeAll };
-
-    Take(TakeMode, MarkedSpace*);
-    void operator()(MarkedBlock*);
-    ReturnType returnValue();
-    
-private:
-    TakeMode m_takeMode;
-    MarkedSpace* m_markedSpace;
-    DoublyLinkedList<MarkedBlock> m_blocks;
-};
-
-inline Take::Take(TakeMode takeMode, MarkedSpace* newSpace)
-    : m_takeMode(takeMode)
-    , m_markedSpace(newSpace)
-{
-}
-
-inline void Take::operator()(MarkedBlock* block)
-{
-    if (m_takeMode == TakeIfEmpty && !block->isEmpty())
-        return;
-    
-    m_markedSpace->allocatorFor(block).removeBlock(block);
-    m_blocks.append(block);
-}
-
-inline Take::ReturnType Take::returnValue()
-{
-    return m_blocks.head();
-}
-
 void MarkedSpace::shrink()
 {
     // We record a temporary list of empties to avoid modifying m_blocks while iterating it.
@@ -141,13 +155,6 @@ void MarkedSpace::shrink()
     freeBlocks(forEachBlock(takeIfEmpty));
 }
 
-void MarkedSpace::freeAllBlocks()
-{
-    // We record a temporary list of empties to avoid modifying m_blocks while iterating it.
-    Take take(Take::TakeAll, this);
-    freeBlocks(forEachBlock(take));
-}
-
 #if ENABLE(GGC)
 class GatherDirtyCells {
     WTF_MAKE_NONCOPYABLE(GatherDirtyCells);
index 7bd5ca5..31a0564 100644 (file)
@@ -71,6 +71,8 @@ public:
     static const size_t maxCellSize = 2048;
 
     MarkedSpace(Heap*);
+    ~MarkedSpace();
+    void lastChanceToFinalize();
 
     MarkedAllocator& firstAllocator();
     MarkedAllocator& allocatorFor(size_t);
@@ -93,7 +95,6 @@ public:
     template<typename Functor> typename Functor::ReturnType forEachBlock();
     
     void shrink();
-    void freeAllBlocks();
     void freeBlocks(MarkedBlock* head);
 
     void didAddBlock(MarkedBlock*);
index 88ca438..ded1584 100644 (file)
@@ -59,7 +59,7 @@ WeakBlock::WeakBlock(PageAllocation& allocation)
     ASSERT(isEmpty());
 }
 
-void WeakBlock::finalizeAll()
+void WeakBlock::lastChanceToFinalize()
 {
     for (size_t i = 0; i < weakImplCount(); ++i) {
         WeakImpl* weakImpl = &weakImpls()[i];
index ca0b69e..471f49b 100644 (file)
@@ -70,7 +70,7 @@ public:
     void visitLiveWeakImpls(HeapRootVisitor&);
     void visitDeadWeakImpls(HeapRootVisitor&);
 
-    void finalizeAll();
+    void lastChanceToFinalize();
 
 private:
     static FreeCell* asFreeCell(WeakImpl*);
index be17e47..b225517 100644 (file)
@@ -40,10 +40,10 @@ WeakSet::~WeakSet()
     m_blocks.clear();
 }
 
-void WeakSet::finalizeAll()
+void WeakSet::lastChanceToFinalize()
 {
     for (WeakBlock* block = m_blocks.head(); block; block = block->next())
-        block->finalizeAll();
+        block->lastChanceToFinalize();
 }
 
 void WeakSet::visitLiveWeakImpls(HeapRootVisitor& visitor)
index 0a683bd..fb0c5df 100644 (file)
@@ -36,7 +36,7 @@ class WeakImpl;
 class WeakSet {
 public:
     WeakSet(Heap*);
-    void finalizeAll();
+    void lastChanceToFinalize();
     ~WeakSet();
 
     static WeakImpl* allocate(JSValue, WeakHandleOwner* = 0, void* context = 0);