Restrictions on oversize CopiedBlock allocations should be relaxed

https://bugs.webkit.org/show_bug.cgi?id=105339

Reviewed by Filip Pizlo.

Currently the DFG has a single branch in the inline allocation path for property/array storage where
it checks to see if the number of bytes requested will fit in the current block. This does not match
what the C++ allocation path does; it checks if the requested number of bytes is oversize, and then
if it's not, it tries to fit it in the current block. The garbage collector assumes that ALL allocations
that are greater than 16KB are in oversize blocks. Therefore, this mismatch can lead to crashes when
the collector tries to perform some operation on a CopiedBlock.

To avoid adding an extra branch to the inline allocation path in the JIT, we should make it so that
oversize blocks are allocated on the same alignment boundaries so that there is a single mask to find
the block header of any CopiedBlock (rather than two, one for normal and one for oversize blocks), and
we should figure out if a block is oversize by some other method than just whatever the JSObject says
it is. One way we could record this info Region of the block, since we allocate a one-off Region for
oversize blocks.

* heap/BlockAllocator.h:
(JSC::Region::isCustomSize):
(Region):
(JSC::Region::createCustomSize):
(JSC::Region::Region):
(JSC::BlockAllocator::deallocateCustomSize):
* heap/CopiedBlock.h:
(CopiedBlock):
(JSC::CopiedBlock::isOversize):
(JSC):
* heap/CopiedSpace.cpp:
(JSC::CopiedSpace::tryAllocateOversize):
(JSC::CopiedSpace::tryReallocate):
(JSC::CopiedSpace::tryReallocateOversize):
* heap/CopiedSpace.h:
(CopiedSpace):
* heap/CopiedSpaceInlines.h:
(JSC::CopiedSpace::contains):
(JSC::CopiedSpace::tryAllocate):
(JSC):
* heap/CopyVisitor.h:
(CopyVisitor):
* heap/CopyVisitorInlines.h:
(JSC::CopyVisitor::checkIfShouldCopy):
(JSC::CopyVisitor::didCopy):
* heap/SlotVisitorInlines.h:
(JSC::SlotVisitor::copyLater):
* runtime/JSObject.cpp:
(JSC::JSObject::copyButterfly):

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

Change-Id: Icebcfe83d82ace7c3e1db6a979306f604459c5ae
Reviewed-by: Jocelyn Turcotte <jocelyn.turcotte@digia.com>

10 files changed, 81 insertions, 27 deletions

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 350874b43..18927d19e 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,54 @@
+2012-12-18  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        Restrictions on oversize CopiedBlock allocations should be relaxed
+        https://bugs.webkit.org/show_bug.cgi?id=105339
+
+        Reviewed by Filip Pizlo.
+
+        Currently the DFG has a single branch in the inline allocation path for property/array storage where 
+        it checks to see if the number of bytes requested will fit in the current block. This does not match 
+        what the C++ allocation path does; it checks if the requested number of bytes is oversize, and then 
+        if it's not, it tries to fit it in the current block. The garbage collector assumes that ALL allocations 
+        that are greater than 16KB are in oversize blocks. Therefore, this mismatch can lead to crashes when 
+        the collector tries to perform some operation on a CopiedBlock.
+
+        To avoid adding an extra branch to the inline allocation path in the JIT, we should make it so that 
+        oversize blocks are allocated on the same alignment boundaries so that there is a single mask to find 
+        the block header of any CopiedBlock (rather than two, one for normal and one for oversize blocks), and 
+        we should figure out if a block is oversize by some other method than just whatever the JSObject says 
+        it is. One way we could record this info Region of the block, since we allocate a one-off Region for 
+        oversize blocks.
+
+        * heap/BlockAllocator.h:
+        (JSC::Region::isCustomSize): 
+        (Region):
+        (JSC::Region::createCustomSize):
+        (JSC::Region::Region):
+        (JSC::BlockAllocator::deallocateCustomSize):
+        * heap/CopiedBlock.h:
+        (CopiedBlock):
+        (JSC::CopiedBlock::isOversize): 
+        (JSC):
+        * heap/CopiedSpace.cpp:
+        (JSC::CopiedSpace::tryAllocateOversize):
+        (JSC::CopiedSpace::tryReallocate):
+        (JSC::CopiedSpace::tryReallocateOversize):
+        * heap/CopiedSpace.h:
+        (CopiedSpace): 
+        * heap/CopiedSpaceInlines.h:
+        (JSC::CopiedSpace::contains):
+        (JSC::CopiedSpace::tryAllocate):
+        (JSC):
+        * heap/CopyVisitor.h:
+        (CopyVisitor):
+        * heap/CopyVisitorInlines.h:
+        (JSC::CopyVisitor::checkIfShouldCopy):
+        (JSC::CopyVisitor::didCopy):
+        * heap/SlotVisitorInlines.h:
+        (JSC::SlotVisitor::copyLater):
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::copyButterfly):
+
 2012-12-17  Mark Hahnenberg  <mhahnenberg@apple.com>
 
         Butterfly::growArrayRight shouldn't be called on null Butterfly objects
diff --git a/Source/JavaScriptCore/heap/BlockAllocator.h b/Source/JavaScriptCore/heap/BlockAllocator.h
index 417f81da0..90210c1fa 100644
--- a/Source/JavaScriptCore/heap/BlockAllocator.h
+++ b/Source/JavaScriptCore/heap/BlockAllocator.h
@@ -68,6 +68,7 @@ public:
     size_t blockSize() const { return m_blockSize; }
     bool isFull() const { return m_blocksInUse == m_totalBlocks; }
     bool isEmpty() const { return !m_blocksInUse; }
+    bool isCustomSize() const { return m_isCustomSize; }
 
     DeadBlock* allocate();
     void deallocate(void*);
@@ -81,6 +82,7 @@ private:
     size_t m_totalBlocks;
     size_t m_blocksInUse;
     size_t m_blockSize;
+    bool m_isCustomSize;
     Region* m_prev;
     Region* m_next;
     DoublyLinkedList<DeadBlock> m_deadBlocks;
@@ -101,7 +103,9 @@ inline Region* Region::createCustomSize(size_t blockSize, size_t blockAlignment)
     PageAllocationAligned allocation = PageAllocationAligned::allocate(blockSize, blockAlignment, OSAllocator::JSGCHeapPages);
     if (!static_cast<bool>(allocation))
         CRASH();
-    return new Region(allocation, blockSize, 1);
+    Region* region = new Region(allocation, blockSize, 1);
+    region->m_isCustomSize = true;
+    return region;
 }
 
 inline Region::Region(PageAllocationAligned& allocation, size_t blockSize, size_t totalBlocks)
@@ -110,6 +114,7 @@ inline Region::Region(PageAllocationAligned& allocation, size_t blockSize, size_
     , m_totalBlocks(totalBlocks)
     , m_blocksInUse(0)
     , m_blockSize(blockSize)
+    , m_isCustomSize(false)
     , m_prev(0)
     , m_next(0)
 {
@@ -300,6 +305,7 @@ template<typename T>
 inline void BlockAllocator::deallocateCustomSize(T* block)
 {
     Region* region = block->region();
+    ASSERT(region->isCustomSize());
     region->deallocate(block);
     delete region;
 }
diff --git a/Source/JavaScriptCore/heap/CopiedBlock.h b/Source/JavaScriptCore/heap/CopiedBlock.h
index 7f585585c..cc60a0103 100644
--- a/Source/JavaScriptCore/heap/CopiedBlock.h
+++ b/Source/JavaScriptCore/heap/CopiedBlock.h
@@ -50,6 +50,8 @@ public:
     void pin();
     bool isPinned();
 
+    bool isOversize();
+
     unsigned liveBytes();
     void reportLiveBytes(JSCell*, unsigned);
     void didSurviveGC();
@@ -168,6 +170,11 @@ inline bool CopiedBlock::isPinned()
     return m_isPinned;
 }
 
+inline bool CopiedBlock::isOversize()
+{
+    return region()->isCustomSize();
+}
+
 inline unsigned CopiedBlock::liveBytes()
 {
     return m_liveBytes;
diff --git a/Source/JavaScriptCore/heap/CopiedSpace.cpp b/Source/JavaScriptCore/heap/CopiedSpace.cpp
index e4141c1d7..b235de1dd 100644
--- a/Source/JavaScriptCore/heap/CopiedSpace.cpp
+++ b/Source/JavaScriptCore/heap/CopiedSpace.cpp
@@ -81,7 +81,7 @@ CheckedBoolean CopiedSpace::tryAllocateOversize(size_t bytes, void** outPtr)
 {
     ASSERT(isOversize(bytes));
     
-    CopiedBlock* block = CopiedBlock::create(m_heap->blockAllocator().allocateCustomSize(sizeof(CopiedBlock) + bytes, WTF::pageSize()));
+    CopiedBlock* block = CopiedBlock::create(m_heap->blockAllocator().allocateCustomSize(sizeof(CopiedBlock) + bytes, CopiedBlock::blockSize));
     m_oversizeBlocks.push(block);
     m_blockFilter.add(reinterpret_cast<Bits>(block));
     m_blockSet.add(block);
@@ -104,7 +104,7 @@ CheckedBoolean CopiedSpace::tryReallocate(void** ptr, size_t oldSize, size_t new
     void* oldPtr = *ptr;
     ASSERT(!m_heap->globalData()->isInitializingObject());
     
-    if (isOversize(oldSize) || isOversize(newSize))
+    if (CopiedSpace::blockFor(oldPtr)->isOversize() || isOversize(newSize))
         return tryReallocateOversize(ptr, oldSize, newSize);
     
     if (m_allocator.tryReallocate(oldPtr, oldSize, newSize))
@@ -135,8 +135,8 @@ CheckedBoolean CopiedSpace::tryReallocateOversize(void** ptr, size_t oldSize, si
 
     memcpy(newPtr, oldPtr, oldSize);
 
-    if (isOversize(oldSize)) {
-        CopiedBlock* oldBlock = oversizeBlockFor(oldPtr);
+    CopiedBlock* oldBlock = CopiedSpace::blockFor(oldPtr);
+    if (oldBlock->isOversize()) {
         m_oversizeBlocks.remove(oldBlock);
         m_blockSet.remove(oldBlock);
         m_heap->blockAllocator().deallocateCustomSize(CopiedBlock::destroy(oldBlock));
diff --git a/Source/JavaScriptCore/heap/CopiedSpace.h b/Source/JavaScriptCore/heap/CopiedSpace.h
index e3727100e..65ca04ef6 100644
--- a/Source/JavaScriptCore/heap/CopiedSpace.h
+++ b/Source/JavaScriptCore/heap/CopiedSpace.h
@@ -82,7 +82,6 @@ public:
 
 private:
     static bool isOversize(size_t);
-    static CopiedBlock* oversizeBlockFor(void* ptr);
 
     JS_EXPORT_PRIVATE CheckedBoolean tryAllocateSlowCase(size_t, void**);
     CheckedBoolean tryAllocateOversize(size_t, void**);
diff --git a/Source/JavaScriptCore/heap/CopiedSpaceInlines.h b/Source/JavaScriptCore/heap/CopiedSpaceInlines.h
index 41f94dd74..6087cf4c2 100644
--- a/Source/JavaScriptCore/heap/CopiedSpaceInlines.h
+++ b/Source/JavaScriptCore/heap/CopiedSpaceInlines.h
@@ -47,9 +47,8 @@ inline bool CopiedSpace::contains(void* ptr, CopiedBlock*& result)
         result = block;
         return true;
     }
-    block = oversizeBlockFor(ptr);
-    result = block;
-    return contains(block);
+    result = 0;
+    return false;
 }
 
 inline void CopiedSpace::pin(CopiedBlock* block)
@@ -153,7 +152,7 @@ inline CheckedBoolean CopiedSpace::tryAllocate(size_t bytes, void** outPtr)
 {
     ASSERT(!m_heap->globalData()->isInitializingObject());
 
-    if (isOversize(bytes) || !m_allocator.tryAllocate(bytes, outPtr))
+    if (!m_allocator.tryAllocate(bytes, outPtr))
         return tryAllocateSlowCase(bytes, outPtr);
     
     ASSERT(*outPtr);
@@ -170,11 +169,6 @@ inline bool CopiedSpace::isPinned(void* ptr)
     return blockFor(ptr)->m_isPinned;
 }
 
-inline CopiedBlock* CopiedSpace::oversizeBlockFor(void* ptr)
-{
-    return reinterpret_cast<CopiedBlock*>(reinterpret_cast<size_t>(ptr) & WTF::pageMask());
-}
-
 inline CopiedBlock* CopiedSpace::blockFor(void* ptr)
 {
     return reinterpret_cast<CopiedBlock*>(reinterpret_cast<size_t>(ptr) & s_blockMask);
diff --git a/Source/JavaScriptCore/heap/CopyVisitor.h b/Source/JavaScriptCore/heap/CopyVisitor.h
index c5f7272a9..da92ba5b5 100644
--- a/Source/JavaScriptCore/heap/CopyVisitor.h
+++ b/Source/JavaScriptCore/heap/CopyVisitor.h
@@ -45,7 +45,7 @@ public:
     // Low-level API for copying, appropriate for cases where the object's heap references
     // are discontiguous or if the object occurs frequently enough that you need to focus on
     // performance. Use this with care as it is easy to shoot yourself in the foot.
-    bool checkIfShouldCopy(void*, size_t);
+    bool checkIfShouldCopy(void*);
     void* allocateNewSpace(size_t);
     void didCopy(void*, size_t);
 
diff --git a/Source/JavaScriptCore/heap/CopyVisitorInlines.h b/Source/JavaScriptCore/heap/CopyVisitorInlines.h
index 1557af93d..4e087b8db 100644
--- a/Source/JavaScriptCore/heap/CopyVisitorInlines.h
+++ b/Source/JavaScriptCore/heap/CopyVisitorInlines.h
@@ -40,14 +40,11 @@ inline void CopyVisitor::visitCell(JSCell* cell)
     JSObject::copyBackingStore(cell, *this);
 }
 
-inline bool CopyVisitor::checkIfShouldCopy(void* oldPtr, size_t bytes)
+inline bool CopyVisitor::checkIfShouldCopy(void* oldPtr)
 {
-    if (CopiedSpace::isOversize(bytes))
+    CopiedBlock* block = CopiedSpace::blockFor(oldPtr);
+    if (block->isOversize() || block->isPinned())
         return false;
-
-    if (CopiedSpace::blockFor(oldPtr)->isPinned())
-        return false;
-
     return true;
 }
 
@@ -92,8 +89,8 @@ inline void CopyVisitor::doneCopying()
 
 inline void CopyVisitor::didCopy(void* ptr, size_t bytes)
 {
-    ASSERT(!CopiedSpace::isOversize(bytes));
     CopiedBlock* block = CopiedSpace::blockFor(ptr);
+    ASSERT(!block->isOversize());
     ASSERT(!block->isPinned());
 
     block->didEvacuateBytes(bytes);
diff --git a/Source/JavaScriptCore/heap/SlotVisitorInlines.h b/Source/JavaScriptCore/heap/SlotVisitorInlines.h
index d76ac552a..3a7f2290c 100644
--- a/Source/JavaScriptCore/heap/SlotVisitorInlines.h
+++ b/Source/JavaScriptCore/heap/SlotVisitorInlines.h
@@ -163,12 +163,12 @@ inline void SlotVisitor::donateAndDrain()
 
 inline void SlotVisitor::copyLater(JSCell* owner, void* ptr, size_t bytes)
 {
-    if (CopiedSpace::isOversize(bytes)) {
-        m_shared.m_copiedSpace->pin(CopiedSpace::oversizeBlockFor(ptr));
+    CopiedBlock* block = CopiedSpace::blockFor(ptr);
+    if (block->isOversize()) {
+        m_shared.m_copiedSpace->pin(block);
         return;
     }
 
-    CopiedBlock* block = CopiedSpace::blockFor(ptr);
     if (block->isPinned())
         return;
 
diff --git a/Source/JavaScriptCore/runtime/JSObject.cpp b/Source/JavaScriptCore/runtime/JSObject.cpp
index 1b3d71cfd..32adefd2f 100644
--- a/Source/JavaScriptCore/runtime/JSObject.cpp
+++ b/Source/JavaScriptCore/runtime/JSObject.cpp
@@ -110,7 +110,7 @@ ALWAYS_INLINE void JSObject::copyButterfly(CopyVisitor& visitor, Butterfly* butt
         indexingPayloadSizeInBytes = 0;
     }
     size_t capacityInBytes = Butterfly::totalSize(preCapacity, propertyCapacity, hasIndexingHeader, indexingPayloadSizeInBytes);
-    if (visitor.checkIfShouldCopy(butterfly->base(preCapacity, propertyCapacity), capacityInBytes)) {
+    if (visitor.checkIfShouldCopy(butterfly->base(preCapacity, propertyCapacity))) {
         Butterfly* newButterfly = Butterfly::createUninitializedDuringCollection(visitor, preCapacity, propertyCapacity, hasIndexingHeader, indexingPayloadSizeInBytes);
         
         // Copy the properties.