GC copy phase spends needless cycles zero-filling blocks
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 18 Jun 2012 04:35:21 +0000 (04:35 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 18 Jun 2012 04:35:21 +0000 (04:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=89128

Reviewed by Gavin Barraclough.

We only need to zero-fill when we're allocating memory that might not
get fully initialized before GC.

* heap/CopiedBlock.h:
(JSC::CopiedBlock::createNoZeroFill):
(JSC::CopiedBlock::create): Added a way to create without zero-filling.
This is our optimization.

(JSC::CopiedBlock::zeroFillToEnd):
(JSC::CopiedBlock::CopiedBlock): Split zero-filling out from creation,
so we can sometimes create without zero-filling.

* heap/CopiedSpace.cpp:
(JSC::CopiedSpace::init):
(JSC::CopiedSpace::tryAllocateSlowCase):
(JSC::CopiedSpace::doneCopying): Renamed addNewBlock to allocateBlock()
to clarify that the new block is always newly-allocated.

(JSC::CopiedSpace::doneFillingBlock): Make sure to zero-fill to the end
of a block that might be used in the future for allocation. (Most of the
time, this is a no-op, since we've already filled the block completely.)

(JSC::CopiedSpace::getFreshBlock): Removed this function because the
abstraction of "allocation must succeed" is no longer useful.

* heap/CopiedSpace.h: Updated declarations to match.

* heap/CopiedSpaceInlineMethods.h:
(JSC::CopiedSpace::allocateBlockForCopyingPhase): New function, which
knows that it can skip zero-filling.

Added tighter scoping to our lock, to improve parallelism.

(JSC::CopiedSpace::allocateBlock): Folded getFreshBlock functionality
into this function, for simplicity.

* heap/MarkStack.cpp:
(JSC::SlotVisitor::startCopying):
(JSC::SlotVisitor::allocateNewSpace): Use our new zero-fill-free helper
function for great good.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/CopiedBlock.h
Source/JavaScriptCore/heap/CopiedSpace.cpp
Source/JavaScriptCore/heap/CopiedSpace.h
Source/JavaScriptCore/heap/CopiedSpaceInlineMethods.h
Source/JavaScriptCore/heap/MarkStack.cpp

index e56f5be..8834b92 100644 (file)
@@ -1,3 +1,51 @@
+2012-06-17  Geoffrey Garen  <ggaren@apple.com>
+
+        GC copy phase spends needless cycles zero-filling blocks
+        https://bugs.webkit.org/show_bug.cgi?id=89128
+
+        Reviewed by Gavin Barraclough.
+
+        We only need to zero-fill when we're allocating memory that might not
+        get fully initialized before GC.
+
+        * heap/CopiedBlock.h:
+        (JSC::CopiedBlock::createNoZeroFill):
+        (JSC::CopiedBlock::create): Added a way to create without zero-filling.
+        This is our optimization.
+
+        (JSC::CopiedBlock::zeroFillToEnd):
+        (JSC::CopiedBlock::CopiedBlock): Split zero-filling out from creation,
+        so we can sometimes create without zero-filling.
+
+        * heap/CopiedSpace.cpp:
+        (JSC::CopiedSpace::init):
+        (JSC::CopiedSpace::tryAllocateSlowCase):
+        (JSC::CopiedSpace::doneCopying): Renamed addNewBlock to allocateBlock()
+        to clarify that the new block is always newly-allocated.
+
+        (JSC::CopiedSpace::doneFillingBlock): Make sure to zero-fill to the end
+        of a block that might be used in the future for allocation. (Most of the
+        time, this is a no-op, since we've already filled the block completely.)
+
+        (JSC::CopiedSpace::getFreshBlock): Removed this function because the
+        abstraction of "allocation must succeed" is no longer useful.
+
+        * heap/CopiedSpace.h: Updated declarations to match.
+
+        * heap/CopiedSpaceInlineMethods.h:
+        (JSC::CopiedSpace::allocateBlockForCopyingPhase): New function, which
+        knows that it can skip zero-filling.
+
+        Added tighter scoping to our lock, to improve parallelism.
+
+        (JSC::CopiedSpace::allocateBlock): Folded getFreshBlock functionality
+        into this function, for simplicity.
+
+        * heap/MarkStack.cpp:
+        (JSC::SlotVisitor::startCopying):
+        (JSC::SlotVisitor::allocateNewSpace): Use our new zero-fill-free helper
+        function for great good.
+
 2012-06-17  Filip Pizlo  <fpizlo@apple.com>
 
         DFG should attempt to use structure watchpoints for all inlined get_by_id's and put_by_id's
index b408aa4..5ed5800 100644 (file)
@@ -39,6 +39,7 @@ class CopiedBlock : public HeapBlock {
     friend class CopiedAllocator;
 public:
     static CopiedBlock* create(const PageAllocationAligned&);
+    static CopiedBlock* createNoZeroFill(const PageAllocationAligned&);
     static PageAllocationAligned destroy(CopiedBlock*);
 
     char* payload();
@@ -47,16 +48,37 @@ public:
 
 private:
     CopiedBlock(const PageAllocationAligned&);
+    void zeroFillToEnd(); // Can be called at any time to zero-fill to the end of the block.
 
     void* m_offset;
     uintptr_t m_isPinned;
 };
 
-inline CopiedBlock* CopiedBlock::create(const PageAllocationAligned& allocation)
+inline CopiedBlock* CopiedBlock::createNoZeroFill(const PageAllocationAligned& allocation)
 {
     return new(NotNull, allocation.base()) CopiedBlock(allocation);
 }
 
+inline CopiedBlock* CopiedBlock::create(const PageAllocationAligned& allocation)
+{
+    CopiedBlock* block = createNoZeroFill(allocation);
+    block->zeroFillToEnd();
+    return block;
+}
+
+inline void CopiedBlock::zeroFillToEnd()
+{
+#if USE(JSVALUE64)
+    char* offset = static_cast<char*>(m_offset);
+    memset(static_cast<void*>(offset), 0, static_cast<size_t>((reinterpret_cast<char*>(this) + m_allocation.size()) - offset));
+#else
+    JSValue emptyValue;
+    JSValue* limit = reinterpret_cast_ptr<JSValue*>(reinterpret_cast<char*>(this) + m_allocation.size());
+    for (JSValue* currentValue = reinterpret_cast<JSValue*>(m_offset); currentValue < limit; currentValue++)
+        *currentValue = emptyValue;
+#endif
+}
+
 inline PageAllocationAligned CopiedBlock::destroy(CopiedBlock* block)
 {
     PageAllocationAligned allocation;
@@ -72,15 +94,6 @@ inline CopiedBlock::CopiedBlock(const PageAllocationAligned& allocation)
     , 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()
index ec51061..631e829 100644 (file)
@@ -58,8 +58,7 @@ void CopiedSpace::init()
     m_toSpace = &m_blocks1;
     m_fromSpace = &m_blocks2;
     
-    if (!addNewBlock())
-        CRASH();
+    allocateBlock();
 }   
 
 CheckedBoolean CopiedSpace::tryAllocateSlowCase(size_t bytes, void** outPtr)
@@ -69,10 +68,8 @@ CheckedBoolean CopiedSpace::tryAllocateSlowCase(size_t bytes, void** outPtr)
     
     m_heap->didAllocate(m_allocator.currentCapacity());
 
-    if (!addNewBlock()) {
-        *outPtr = 0;
-        return false;
-    }
+    allocateBlock();
+
     *outPtr = m_allocator.allocate(bytes);
     ASSERT(*outPtr);
     return true;
@@ -168,6 +165,8 @@ void CopiedSpace::doneFillingBlock(CopiedBlock* block)
         return;
     }
 
+    block->zeroFillToEnd();
+
     {
         SpinLockHolder locker(&m_toSpaceLock);
         m_toSpace->push(block);
@@ -223,35 +222,12 @@ void CopiedSpace::doneCopying()
         curr = next;
     }
 
-    if (!m_toSpace->head()) {
-        if (!addNewBlock())
-            CRASH();
-    } else
+    if (!m_toSpace->head())
+        allocateBlock();
+    else
         m_allocator.resetCurrentBlock(static_cast<CopiedBlock*>(m_toSpace->head()));
 }
 
-CheckedBoolean CopiedSpace::getFreshBlock(AllocationEffort allocationEffort, CopiedBlock** outBlock)
-{
-    CopiedBlock* block = 0;
-    if (allocationEffort == AllocationMustSucceed)
-        block = CopiedBlock::create(m_heap->blockAllocator().allocate());
-    else {
-        ASSERT(allocationEffort == AllocationCanFail);
-        if (m_heap->shouldCollect())
-            m_heap->collect(Heap::DoNotSweep);
-        
-        if (!getFreshBlock(AllocationMustSucceed, &block)) {
-            *outBlock = 0;
-            ASSERT_NOT_REACHED();
-            return false;
-        }
-    }
-    ASSERT(block);
-    ASSERT(is8ByteAligned(block->m_offset));
-    *outBlock = block;
-    return true;
-}
-
 size_t CopiedSpace::size()
 {
     size_t calculatedSize = 0;
index 383ccd0..530e989 100644 (file)
@@ -77,22 +77,20 @@ public:
     static CopiedBlock* blockFor(void*);
 
 private:
-    CheckedBoolean tryAllocateSlowCase(size_t, void**);
-    CheckedBoolean addNewBlock();
-    CheckedBoolean allocateNewBlock(CopiedBlock**);
-    
     static void* allocateFromBlock(CopiedBlock*, size_t);
+    static bool isOversize(size_t);
+    static bool fitsInBlock(CopiedBlock*, size_t);
+    static CopiedBlock* oversizeBlockFor(void* ptr);
+
+    CheckedBoolean tryAllocateSlowCase(size_t, void**);
     CheckedBoolean tryAllocateOversize(size_t, void**);
     CheckedBoolean tryReallocateOversize(void**, size_t, size_t);
     
-    static bool isOversize(size_t);
-    
-    CheckedBoolean borrowBlock(CopiedBlock**);
-    CheckedBoolean getFreshBlock(AllocationEffort, CopiedBlock**);
+    void allocateBlock();
+    CopiedBlock* allocateBlockForCopyingPhase();
+
     void doneFillingBlock(CopiedBlock*);
     void recycleBlock(CopiedBlock*);
-    static bool fitsInBlock(CopiedBlock*, size_t);
-    static CopiedBlock* oversizeBlockFor(void* ptr);
 
     Heap* m_heap;
 
index c977625..1366cd8 100644 (file)
@@ -84,46 +84,31 @@ inline void CopiedSpace::recycleBlock(CopiedBlock* block)
     }
 }
 
-inline CheckedBoolean CopiedSpace::borrowBlock(CopiedBlock** outBlock)
+inline CopiedBlock* CopiedSpace::allocateBlockForCopyingPhase()
 {
-    CopiedBlock* block = 0;
-    if (!getFreshBlock(AllocationMustSucceed, &block)) {
-        *outBlock = 0;
-        return false;
-    }
-
     ASSERT(m_inCopyingPhase);
-    MutexLocker locker(m_loanedBlocksLock);
-    m_numberOfLoanedBlocks++;
+    CopiedBlock* block = CopiedBlock::createNoZeroFill(m_heap->blockAllocator().allocate());
+
+    {
+        MutexLocker locker(m_loanedBlocksLock);
+        m_numberOfLoanedBlocks++;
+    }
 
     ASSERT(block->m_offset == block->payload());
-    *outBlock = block;
-    return true;
+    return block;
 }
 
-inline CheckedBoolean CopiedSpace::addNewBlock()
+inline void CopiedSpace::allocateBlock()
 {
-    CopiedBlock* block = 0;
-    if (!getFreshBlock(AllocationCanFail, &block))
-        return false;
+    if (m_heap->shouldCollect())
+        m_heap->collect(Heap::DoNotSweep);
+
+    CopiedBlock* block = CopiedBlock::create(m_heap->blockAllocator().allocate());
         
     m_toSpace->push(block);
     m_blockFilter.add(reinterpret_cast<Bits>(block));
     m_blockSet.add(block);
     m_allocator.resetCurrentBlock(block);
-    return true;
-}
-
-inline CheckedBoolean CopiedSpace::allocateNewBlock(CopiedBlock** outBlock)
-{
-    PageAllocationAligned allocation = PageAllocationAligned::allocate(HeapBlock::s_blockSize, HeapBlock::s_blockSize, OSAllocator::JSGCHeapPages);
-    if (!static_cast<bool>(allocation)) {
-        *outBlock = 0;
-        return false;
-    }
-
-    *outBlock = new (NotNull, allocation.base()) CopiedBlock(allocation);
-    return true;
 }
 
 inline bool CopiedSpace::fitsInBlock(CopiedBlock* block, size_t bytes)
index 9e1e6e6..3eb02c4 100644 (file)
@@ -495,8 +495,7 @@ void MarkStack::mergeOpaqueRoots()
 void SlotVisitor::startCopying()
 {
     ASSERT(!m_copyBlock);
-    if (!m_shared.m_copiedSpace->borrowBlock(&m_copyBlock))
-        CRASH();
+    m_copyBlock = m_shared.m_copiedSpace->allocateBlockForCopyingPhase();
 }    
 
 void* SlotVisitor::allocateNewSpace(void* ptr, size_t bytes)
@@ -517,8 +516,7 @@ void* SlotVisitor::allocateNewSpace(void* ptr, size_t bytes)
         // We don't need to lock across these two calls because the master thread won't 
         // call doneCopying() because this thread is considered active.
         m_shared.m_copiedSpace->doneFillingBlock(m_copyBlock);
-        if (!m_shared.m_copiedSpace->borrowBlock(&m_copyBlock))
-            CRASH();
+        m_copyBlock = m_shared.m_copiedSpace->allocateBlockForCopyingPhase();
     }
     return CopiedSpace::allocateFromBlock(m_copyBlock, bytes);
 }