CopiedBlock and MarkedBlock should have proper value semantics (i.e., destructors)
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 May 2012 23:59:51 +0000 (23:59 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 May 2012 23:59:51 +0000 (23:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=87172

Reviewed by Oliver Hunt and Phil Pizlo.

This enables MarkedBlock to own non-trivial sub-objects that require
destruction. It also fixes a FIXME about casting a CopiedBlock to a
MarkedBlock at destroy time.

CopiedBlock and MarkedBlock now accept an allocation chunk at create
time and return it at destroy time. Their client is expected to
allocate, recycle, and destroy these chunks.

* heap/BlockAllocator.cpp:
(JSC::BlockAllocator::releaseFreeBlocks):
(JSC::BlockAllocator::blockFreeingThreadMain): Don't call MarkedBlock::destroy
because we expect that to be called before a block is put on our free
list now. Do manually deallocate our allocation chunk because that's
our job now.

* heap/BlockAllocator.h:
(BlockAllocator):
(JSC::BlockAllocator::allocate): Allocate never fails now. This is a
cleaner abstraction because only one object does all the VM allocation
and deallocation. Caching is an implementation detail.

(JSC::BlockAllocator::deallocate): We take an allocation chunk argument
instead of a block because we now expect the block to have been destroyed
before we recycle its memory. For convenience, we still use the HeapBlock
class as our linked list node. This is OK because HeapBlock is a POD type.

* heap/CopiedBlock.h:
(CopiedBlock):
(JSC::CopiedBlock::create):
(JSC::CopiedBlock::destroy):
(JSC::CopiedBlock::CopiedBlock): Added proper create and destroy functions,
to match MarkedBlock.

* heap/CopiedSpace.cpp:
(JSC::CopiedSpace::tryAllocateOversize):
(JSC::CopiedSpace::tryReallocateOversize):
(JSC::CopiedSpace::doneCopying):
(JSC::CopiedSpace::getFreshBlock):
(JSC::CopiedSpace::freeAllBlocks):
* heap/CopiedSpaceInlineMethods.h:
(JSC::CopiedSpace::recycleBlock): Make sure to call destroy before
returning a block to the BlockAllocator. Otherwise, our destructors
won't run. (If we get this wrong now, we'll get a compile error.)

* heap/HeapBlock.h:
(JSC::HeapBlock::HeapBlock): const!

* heap/MarkedAllocator.cpp:
(JSC::MarkedAllocator::allocateBlock): No need to distinguish between
create and recycle -- MarkedBlock always accepts memory allocated by
its client now.

* heap/MarkedBlock.cpp:
(JSC::MarkedBlock::create): Don't allocate memory -- we assume that we're
passed already-allocated memory, to clarify the responsibility for VM
recycling.

(JSC::MarkedBlock::destroy): Do run our destructor before giving back
our VM -- that is the whole point of this patch.

(JSC::MarkedBlock::MarkedBlock):
* heap/MarkedBlock.h:
(MarkedBlock):
* heap/MarkedSpace.cpp: const!

(JSC::MarkedSpace::freeBlocks): Make sure to call destroy before
returning a block to the BlockAllocator. Otherwise, our destructors
won't run. (If we get this wrong now, we'll get a compile error.)

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/BlockAllocator.cpp
Source/JavaScriptCore/heap/BlockAllocator.h
Source/JavaScriptCore/heap/CopiedBlock.h
Source/JavaScriptCore/heap/CopiedSpace.cpp
Source/JavaScriptCore/heap/CopiedSpaceInlineMethods.h
Source/JavaScriptCore/heap/HeapBlock.h
Source/JavaScriptCore/heap/MarkedAllocator.cpp
Source/JavaScriptCore/heap/MarkedBlock.cpp
Source/JavaScriptCore/heap/MarkedBlock.h
Source/JavaScriptCore/heap/MarkedSpace.cpp

index 406c4c3..d9851f9 100644 (file)
@@ -1 +1,77 @@
+2012-05-22  Geoffrey Garen  <ggaren@apple.com>
+
+        CopiedBlock and MarkedBlock should have proper value semantics (i.e., destructors)
+        https://bugs.webkit.org/show_bug.cgi?id=87172
+
+        Reviewed by Oliver Hunt and Phil Pizlo.
+
+        This enables MarkedBlock to own non-trivial sub-objects that require
+        destruction. It also fixes a FIXME about casting a CopiedBlock to a
+        MarkedBlock at destroy time.
+
+        CopiedBlock and MarkedBlock now accept an allocation chunk at create
+        time and return it at destroy time. Their client is expected to
+        allocate, recycle, and destroy these chunks.
+
+        * heap/BlockAllocator.cpp:
+        (JSC::BlockAllocator::releaseFreeBlocks):
+        (JSC::BlockAllocator::blockFreeingThreadMain): Don't call MarkedBlock::destroy
+        because we expect that to be called before a block is put on our free
+        list now. Do manually deallocate our allocation chunk because that's
+        our job now.
+
+        * heap/BlockAllocator.h:
+        (BlockAllocator):
+        (JSC::BlockAllocator::allocate): Allocate never fails now. This is a
+        cleaner abstraction because only one object does all the VM allocation
+        and deallocation. Caching is an implementation detail.
+
+        (JSC::BlockAllocator::deallocate): We take an allocation chunk argument
+        instead of a block because we now expect the block to have been destroyed 
+        before we recycle its memory. For convenience, we still use the HeapBlock
+        class as our linked list node. This is OK because HeapBlock is a POD type.
+
+        * heap/CopiedBlock.h:
+        (CopiedBlock):
+        (JSC::CopiedBlock::create):
+        (JSC::CopiedBlock::destroy):
+        (JSC::CopiedBlock::CopiedBlock): Added proper create and destroy functions,
+        to match MarkedBlock.
+
+        * heap/CopiedSpace.cpp:
+        (JSC::CopiedSpace::tryAllocateOversize):
+        (JSC::CopiedSpace::tryReallocateOversize):
+        (JSC::CopiedSpace::doneCopying):
+        (JSC::CopiedSpace::getFreshBlock):
+        (JSC::CopiedSpace::freeAllBlocks):
+        * heap/CopiedSpaceInlineMethods.h:
+        (JSC::CopiedSpace::recycleBlock): Make sure to call destroy before
+        returning a block to the BlockAllocator. Otherwise, our destructors
+        won't run. (If we get this wrong now, we'll get a compile error.)
+
+        * heap/HeapBlock.h:
+        (JSC::HeapBlock::HeapBlock): const!
+
+        * heap/MarkedAllocator.cpp:
+        (JSC::MarkedAllocator::allocateBlock): No need to distinguish between
+        create and recycle -- MarkedBlock always accepts memory allocated by
+        its client now.
+
+        * heap/MarkedBlock.cpp:
+        (JSC::MarkedBlock::create): Don't allocate memory -- we assume that we're
+        passed already-allocated memory, to clarify the responsibility for VM
+        recycling.
+
+        (JSC::MarkedBlock::destroy): Do run our destructor before giving back
+        our VM -- that is the whole point of this patch.
+
+        (JSC::MarkedBlock::MarkedBlock):
+        * heap/MarkedBlock.h:
+        (MarkedBlock):
+        * heap/MarkedSpace.cpp: const!
+
+        (JSC::MarkedSpace::freeBlocks): Make sure to call destroy before
+        returning a block to the BlockAllocator. Otherwise, our destructors
+        won't run. (If we get this wrong now, we'll get a compile error.)
+
 == Rolled over to ChangeLog-2012-05-22 ==
index ce60240..485ec8d 100644 (file)
@@ -26,7 +26,6 @@
 #include "config.h"
 #include "BlockAllocator.h"
 
-#include "MarkedBlock.h"
 #include <wtf/CurrentTime.h>
 
 namespace JSC {
@@ -54,14 +53,13 @@ BlockAllocator::~BlockAllocator()
 void BlockAllocator::releaseFreeBlocks()
 {
     while (true) {
-        MarkedBlock* block;
+        HeapBlock* block;
         {
             MutexLocker locker(m_freeBlockLock);
             if (!m_numberOfFreeBlocks)
                 block = 0;
             else {
-                // FIXME: How do we know this is a MarkedBlock? It could be a CopiedBlock.
-                block = static_cast<MarkedBlock*>(m_freeBlocks.removeHead());
+                block = m_freeBlocks.removeHead();
                 ASSERT(block);
                 m_numberOfFreeBlocks--;
             }
@@ -69,8 +67,8 @@ void BlockAllocator::releaseFreeBlocks()
         
         if (!block)
             break;
-        
-        MarkedBlock::destroy(block);
+
+        block->m_allocation.deallocate();
     }
 }
 
@@ -120,14 +118,13 @@ void BlockAllocator::blockFreeingThreadMain()
         size_t desiredNumberOfFreeBlocks = currentNumberOfFreeBlocks / 2;
         
         while (!m_blockFreeingThreadShouldQuit) {
-            MarkedBlock* block;
+            HeapBlock* block;
             {
                 MutexLocker locker(m_freeBlockLock);
                 if (m_numberOfFreeBlocks <= desiredNumberOfFreeBlocks)
                     block = 0;
                 else {
-                    // FIXME: How do we know this is a MarkedBlock? It could be a CopiedBlock.
-                    block = static_cast<MarkedBlock*>(m_freeBlocks.removeHead());
+                    block = m_freeBlocks.removeHead();
                     ASSERT(block);
                     m_numberOfFreeBlocks--;
                 }
@@ -136,7 +133,7 @@ void BlockAllocator::blockFreeingThreadMain()
             if (!block)
                 break;
             
-            MarkedBlock::destroy(block);
+            block->m_allocation.deallocate();
         }
     }
 }
index cc9557f..846bdfa 100644 (file)
 #ifndef BlockAllocator_h
 #define BlockAllocator_h
 
+#include "HeapBlock.h"
 #include <wtf/DoublyLinkedList.h>
 #include <wtf/Forward.h>
+#include <wtf/PageAllocationAligned.h>
 #include <wtf/Threading.h>
 
 namespace JSC {
 
-class HeapBlock;
-
 // Simple allocator to reduce VM cost by holding onto blocks of memory for
 // short periods of time and then freeing them on a secondary thread.
 
@@ -42,8 +42,8 @@ public:
     BlockAllocator();
     ~BlockAllocator();
 
-    HeapBlock* allocate();
-    void deallocate(HeapBlock*);
+    PageAllocationAligned allocate();
+    void deallocate(PageAllocationAligned);
 
 private:
     void waitForRelativeTimeWhileHoldingLock(double relative);
@@ -63,24 +63,28 @@ private:
     ThreadIdentifier m_blockFreeingThread;
 };
 
-inline HeapBlock* BlockAllocator::allocate()
+inline PageAllocationAligned BlockAllocator::allocate()
 {
     MutexLocker locker(m_freeBlockLock);
     m_isCurrentlyAllocating = true;
-    if (!m_numberOfFreeBlocks) {
-        ASSERT(m_freeBlocks.isEmpty());
-        return 0;
+    if (m_numberOfFreeBlocks) {
+        ASSERT(!m_freeBlocks.isEmpty());
+        m_numberOfFreeBlocks--;
+        return m_freeBlocks.removeHead()->m_allocation;
     }
 
-    ASSERT(!m_freeBlocks.isEmpty());
-    m_numberOfFreeBlocks--;
-    return m_freeBlocks.removeHead();
+    ASSERT(m_freeBlocks.isEmpty());
+    PageAllocationAligned allocation = PageAllocationAligned::allocate(HeapBlock::s_blockSize, HeapBlock::s_blockSize, OSAllocator::JSGCHeapPages);
+    if (!static_cast<bool>(allocation))
+        CRASH();
+    return allocation;
 }
 
-inline void BlockAllocator::deallocate(HeapBlock* block)
+inline void BlockAllocator::deallocate(PageAllocationAligned allocation)
 {
     MutexLocker locker(m_freeBlockLock);
-    m_freeBlocks.push(block);
+    HeapBlock* heapBlock = new(NotNull, allocation.base()) HeapBlock(allocation);
+    m_freeBlocks.push(heapBlock);
     m_numberOfFreeBlocks++;
 }
 
index 431b86c..b408aa4 100644 (file)
@@ -38,32 +38,51 @@ class CopiedBlock : public HeapBlock {
     friend class CopiedSpace;
     friend class CopiedAllocator;
 public:
-    CopiedBlock(PageAllocationAligned& allocation)
-        : HeapBlock(allocation)
-        , m_offset(payload())
-        , m_isPinned(false)
-    {
-        ASSERT(is8ByteAligned(static_cast<void*>(m_offset)));
-#if USE(JSVALUE64)
-        char* offset = static_cast<char*>(m_offset);
-        memset(static_cast<void*>(offset), 0, static_cast<size_t>((reinterpret_cast<char*>(this) + allocation.size()) - offset));
-#else
-        JSValue emptyValue;
-        JSValue* limit = reinterpret_cast_ptr<JSValue*>(reinterpret_cast<char*>(this) + allocation.size());
-        for (JSValue* currentValue = reinterpret_cast<JSValue*>(m_offset); currentValue < limit; currentValue++)
-            *currentValue = emptyValue;
-#endif
-    }
+    static CopiedBlock* create(const PageAllocationAligned&);
+    static PageAllocationAligned destroy(CopiedBlock*);
 
     char* payload();
     size_t size();
     size_t capacity();
 
 private:
+    CopiedBlock(const PageAllocationAligned&);
+
     void* m_offset;
     uintptr_t m_isPinned;
 };
 
+inline CopiedBlock* CopiedBlock::create(const PageAllocationAligned& allocation)
+{
+    return new(NotNull, allocation.base()) CopiedBlock(allocation);
+}
+
+inline PageAllocationAligned CopiedBlock::destroy(CopiedBlock* block)
+{
+    PageAllocationAligned allocation;
+    swap(allocation, block->m_allocation);
+
+    block->~CopiedBlock();
+    return allocation;
+}
+
+inline CopiedBlock::CopiedBlock(const PageAllocationAligned& allocation)
+    : HeapBlock(allocation)
+    , m_offset(payload())
+    , m_isPinned(false)
+{
+    ASSERT(is8ByteAligned(static_cast<void*>(m_offset)));
+#if USE(JSVALUE64)
+    char* offset = static_cast<char*>(m_offset);
+    memset(static_cast<void*>(offset), 0, static_cast<size_t>((reinterpret_cast<char*>(this) + allocation.size()) - offset));
+#else
+    JSValue emptyValue;
+    JSValue* limit = reinterpret_cast_ptr<JSValue*>(reinterpret_cast<char*>(this) + allocation.size());
+    for (JSValue* currentValue = reinterpret_cast<JSValue*>(m_offset); currentValue < limit; currentValue++)
+        *currentValue = emptyValue;
+#endif
+}
+
 inline char* CopiedBlock::payload()
 {
     return reinterpret_cast<char*>(this) + ((sizeof(CopiedBlock) + 7) & ~7);
index d52c4e7..4183b38 100644 (file)
@@ -77,7 +77,7 @@ CheckedBoolean CopiedSpace::tryAllocateOversize(size_t bytes, void** outPtr)
         return false;
     }
 
-    CopiedBlock* block = new (NotNull, allocation.base()) CopiedBlock(allocation);
+    CopiedBlock* block = CopiedBlock::create(allocation);
     m_oversizeBlocks.push(block);
     m_oversizeFilter.add(reinterpret_cast<Bits>(block));
     
@@ -135,7 +135,7 @@ CheckedBoolean CopiedSpace::tryReallocateOversize(void** ptr, size_t oldSize, si
     if (isOversize(oldSize)) {
         CopiedBlock* oldBlock = oversizeBlockFor(oldPtr);
         m_oversizeBlocks.remove(oldBlock);
-        oldBlock->m_allocation.deallocate();
+        CopiedBlock::destroy(oldBlock).deallocate();
     }
     
     *ptr = newPtr;
@@ -191,7 +191,7 @@ void CopiedSpace::doneCopying()
         }
 
         m_toSpaceSet.remove(block);
-        m_heap->blockAllocator().deallocate(block);
+        m_heap->blockAllocator().deallocate(CopiedBlock::destroy(block));
     }
 
     CopiedBlock* curr = static_cast<CopiedBlock*>(m_oversizeBlocks.head());
@@ -199,7 +199,7 @@ void CopiedSpace::doneCopying()
         CopiedBlock* next = static_cast<CopiedBlock*>(curr->next());
         if (!curr->m_isPinned) {
             m_oversizeBlocks.remove(curr);
-            curr->m_allocation.deallocate();
+            CopiedBlock::destroy(curr).deallocate();
         } else
             curr->m_isPinned = false;
         curr = next;
@@ -215,15 +215,9 @@ void CopiedSpace::doneCopying()
 CheckedBoolean CopiedSpace::getFreshBlock(AllocationEffort allocationEffort, CopiedBlock** outBlock)
 {
     CopiedBlock* block = 0;
-    if (allocationEffort == AllocationMustSucceed) {
-        if (HeapBlock* heapBlock = m_heap->blockAllocator().allocate())
-            block = new (NotNull, heapBlock) CopiedBlock(heapBlock->m_allocation);
-        else if (!allocateNewBlock(&block)) {
-            *outBlock = 0;
-            ASSERT_NOT_REACHED();
-            return false;
-        }
-    } else {
+    if (allocationEffort == AllocationMustSucceed)
+        block = CopiedBlock::create(m_heap->blockAllocator().allocate());
+    else {
         ASSERT(allocationEffort == AllocationCanFail);
         if (m_heap->shouldCollect())
             m_heap->collect(Heap::DoNotSweep);
@@ -243,13 +237,13 @@ CheckedBoolean CopiedSpace::getFreshBlock(AllocationEffort allocationEffort, Cop
 void CopiedSpace::freeAllBlocks()
 {
     while (!m_toSpace->isEmpty())
-        m_heap->blockAllocator().deallocate(m_toSpace->removeHead());
+        m_heap->blockAllocator().deallocate(CopiedBlock::destroy(static_cast<CopiedBlock*>(m_toSpace->removeHead())));
 
     while (!m_fromSpace->isEmpty())
-        m_heap->blockAllocator().deallocate(m_fromSpace->removeHead());
+        m_heap->blockAllocator().deallocate(CopiedBlock::destroy(static_cast<CopiedBlock*>(m_fromSpace->removeHead())));
 
     while (!m_oversizeBlocks.isEmpty())
-        m_oversizeBlocks.removeHead()->m_allocation.deallocate();
+        CopiedBlock::destroy(static_cast<CopiedBlock*>(m_oversizeBlocks.removeHead())).deallocate();
 }
 
 size_t CopiedSpace::size()
index a8e4565..2d360ae 100644 (file)
@@ -63,7 +63,7 @@ inline void CopiedSpace::startedCopying()
 
 inline void CopiedSpace::recycleBlock(CopiedBlock* block)
 {
-    m_heap->blockAllocator().deallocate(block);
+    m_heap->blockAllocator().deallocate(CopiedBlock::destroy(block));
 
     {
         MutexLocker locker(m_loanedBlocksLock);
index 591520d..3cd3c63 100644 (file)
@@ -36,13 +36,13 @@ enum AllocationEffort { AllocationCanFail, AllocationMustSucceed };
 
 class HeapBlock : public DoublyLinkedListNode<HeapBlock> {
 public:
-    HeapBlock(PageAllocationAligned& allocation)
+    HeapBlock(const PageAllocationAligned& allocation)
         : DoublyLinkedListNode<HeapBlock>()
         , m_prev(0)
         , m_next(0)
         , m_allocation(allocation)
     {
-        ASSERT(allocation);
+        ASSERT(m_allocation);
     }
 
     HeapBlock* m_prev;
index 01f00c3..ac0cf57 100644 (file)
@@ -86,17 +86,11 @@ void* MarkedAllocator::allocateSlowCase()
     ASSERT(result);
     return result;
 }
-    
+
 MarkedBlock* MarkedAllocator::allocateBlock()
 {
-    MarkedBlock* block = static_cast<MarkedBlock*>(m_heap->blockAllocator().allocate());
-    if (block)
-        block = MarkedBlock::recycle(block, m_heap, m_cellSize, m_cellsNeedDestruction);
-    else
-        block = MarkedBlock::create(m_heap, m_cellSize, m_cellsNeedDestruction);
-    
+    MarkedBlock* block = MarkedBlock::create(m_heap->blockAllocator().allocate(), m_heap, m_cellSize, m_cellsNeedDestruction);
     m_markedSpace->didAddBlock(block);
-    
     return block;
 }
 
index 3a58b5a..ad036db 100644 (file)
 
 namespace JSC {
 
-MarkedBlock* MarkedBlock::create(Heap* heap, size_t cellSize, bool cellsNeedDestruction)
+MarkedBlock* MarkedBlock::create(const PageAllocationAligned& allocation, Heap* heap, size_t cellSize, bool cellsNeedDestruction)
 {
-    PageAllocationAligned allocation = PageAllocationAligned::allocate(blockSize, blockSize, OSAllocator::JSGCHeapPages);
-    if (!static_cast<bool>(allocation))
-        CRASH();
     return new (NotNull, allocation.base()) MarkedBlock(allocation, heap, cellSize, cellsNeedDestruction);
 }
 
-MarkedBlock* MarkedBlock::recycle(MarkedBlock* block, Heap* heap, size_t cellSize, bool cellsNeedDestruction)
+PageAllocationAligned MarkedBlock::destroy(MarkedBlock* block)
 {
-    return new (NotNull, block) MarkedBlock(block->m_allocation, heap, cellSize, cellsNeedDestruction);
-}
+    PageAllocationAligned allocation;
+    swap(allocation, block->m_allocation);
 
-void MarkedBlock::destroy(MarkedBlock* block)
-{
-    block->m_allocation.deallocate();
+    block->~MarkedBlock();
+    return allocation;
 }
 
-MarkedBlock::MarkedBlock(PageAllocationAligned& allocation, Heap* heap, size_t cellSize, bool cellsNeedDestruction)
+MarkedBlock::MarkedBlock(const PageAllocationAligned& allocation, Heap* heap, size_t cellSize, bool cellsNeedDestruction)
     : HeapBlock(allocation)
     , m_atomsPerCell((cellSize + atomSize - 1) / atomSize)
     , m_endAtom(atomsPerBlock - m_atomsPerCell + 1)
index aa99ebf..8041294 100644 (file)
@@ -112,9 +112,8 @@ namespace JSC {
             ReturnType m_count;
         };
 
-        static MarkedBlock* create(Heap*, size_t cellSize, bool cellsNeedDestruction);
-        static MarkedBlock* recycle(MarkedBlock*, Heap*, size_t cellSize, bool cellsNeedDestruction);
-        static void destroy(MarkedBlock*);
+        static MarkedBlock* create(const PageAllocationAligned&, Heap*, size_t cellSize, bool cellsNeedDestruction);
+        static PageAllocationAligned destroy(MarkedBlock*);
 
         static bool isAtomAligned(const void*);
         static MarkedBlock* blockFor(const void*);
@@ -187,7 +186,7 @@ namespace JSC {
 
         typedef char Atom[atomSize];
 
-        MarkedBlock(PageAllocationAligned&, Heap*, size_t cellSize, bool cellsNeedDestruction);
+        MarkedBlock(const PageAllocationAligned&, Heap*, size_t cellSize, bool cellsNeedDestruction);
         Atom* atoms();
         size_t atomNumber(const void*);
         void callDestructor(JSCell*);
index 405ed57..8cc3dc7 100644 (file)
@@ -94,7 +94,7 @@ void MarkedSpace::freeBlocks(MarkedBlock* head)
         m_blocks.remove(block);
         block->sweep();
 
-        m_heap->blockAllocator().deallocate(block);
+        m_heap->blockAllocator().deallocate(MarkedBlock::destroy(block));
     }
 }