CopiedSpace::contains doesn't check for oversize blocks
authormhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 May 2012 01:28:03 +0000 (01:28 +0000)
committermhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 May 2012 01:28:03 +0000 (01:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=87180

Reviewed by Geoffrey Garen.

When doing a conservative scan we use CopiedSpace::contains to determine if a particular
address points into the CopiedSpace. Currently contains() only checks if the address
points to a block in to-space, which means that pointers to oversize blocks may not get scanned.

* heap/CopiedSpace.cpp:
(JSC::CopiedSpace::tryAllocateOversize):
(JSC::CopiedSpace::tryReallocateOversize):
(JSC::CopiedSpace::doneFillingBlock):
(JSC::CopiedSpace::doneCopying):
* heap/CopiedSpace.h: Refactored CopiedSpace so that all blocks (oversize and to-space) are
in a single hash set and bloom filter for membership testing.
(CopiedSpace):
* heap/CopiedSpaceInlineMethods.h:
(JSC::CopiedSpace::contains): We check for the normal block first. Since the oversize blocks are
only page aligned, rather than block aligned, we have to re-mask the ptr to check if it's in
CopiedSpace. Also added a helper function of the same name that takes a CopiedBlock* and checks
if it's in CopiedSpace so that check isn't typed out twice.
(JSC):
(JSC::CopiedSpace::startedCopying):
(JSC::CopiedSpace::addNewBlock):

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

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

index d9851f9..f548d8c 100644 (file)
@@ -1,3 +1,31 @@
+2012-05-22  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        CopiedSpace::contains doesn't check for oversize blocks
+        https://bugs.webkit.org/show_bug.cgi?id=87180
+
+        Reviewed by Geoffrey Garen.
+
+        When doing a conservative scan we use CopiedSpace::contains to determine if a particular 
+        address points into the CopiedSpace. Currently contains() only checks if the address 
+        points to a block in to-space, which means that pointers to oversize blocks may not get scanned. 
+
+        * heap/CopiedSpace.cpp:
+        (JSC::CopiedSpace::tryAllocateOversize):
+        (JSC::CopiedSpace::tryReallocateOversize):
+        (JSC::CopiedSpace::doneFillingBlock):
+        (JSC::CopiedSpace::doneCopying):
+        * heap/CopiedSpace.h: Refactored CopiedSpace so that all blocks (oversize and to-space) are 
+        in a single hash set and bloom filter for membership testing.
+        (CopiedSpace):
+        * heap/CopiedSpaceInlineMethods.h:
+        (JSC::CopiedSpace::contains): We check for the normal block first. Since the oversize blocks are
+        only page aligned, rather than block aligned, we have to re-mask the ptr to check if it's in 
+        CopiedSpace. Also added a helper function of the same name that takes a CopiedBlock* and checks
+        if it's in CopiedSpace so that check isn't typed out twice.
+        (JSC):
+        (JSC::CopiedSpace::startedCopying):
+        (JSC::CopiedSpace::addNewBlock):
+
 2012-05-22  Geoffrey Garen  <ggaren@apple.com>
 
         CopiedBlock and MarkedBlock should have proper value semantics (i.e., destructors)
index 4183b38..2906e1d 100644 (file)
@@ -79,7 +79,8 @@ CheckedBoolean CopiedSpace::tryAllocateOversize(size_t bytes, void** outPtr)
 
     CopiedBlock* block = CopiedBlock::create(allocation);
     m_oversizeBlocks.push(block);
-    m_oversizeFilter.add(reinterpret_cast<Bits>(block));
+    m_blockFilter.add(reinterpret_cast<Bits>(block));
+    m_blockSet.add(block);
     
     *outPtr = allocateFromBlock(block, bytes);
 
@@ -135,6 +136,7 @@ CheckedBoolean CopiedSpace::tryReallocateOversize(void** ptr, size_t oldSize, si
     if (isOversize(oldSize)) {
         CopiedBlock* oldBlock = oversizeBlockFor(oldPtr);
         m_oversizeBlocks.remove(oldBlock);
+        m_blockSet.remove(oldBlock);
         CopiedBlock::destroy(oldBlock).deallocate();
     }
     
@@ -156,8 +158,8 @@ void CopiedSpace::doneFillingBlock(CopiedBlock* block)
     {
         MutexLocker locker(m_toSpaceLock);
         m_toSpace->push(block);
-        m_toSpaceSet.add(block);
-        m_toSpaceFilter.add(reinterpret_cast<Bits>(block));
+        m_blockSet.add(block);
+        m_blockFilter.add(reinterpret_cast<Bits>(block));
     }
 
     {
@@ -183,14 +185,14 @@ void CopiedSpace::doneCopying()
         CopiedBlock* block = static_cast<CopiedBlock*>(m_fromSpace->removeHead());
         if (block->m_isPinned) {
             block->m_isPinned = false;
-            // We don't add the block to the toSpaceSet because it was never removed.
-            ASSERT(m_toSpaceSet.contains(block));
-            m_toSpaceFilter.add(reinterpret_cast<Bits>(block));
+            // We don't add the block to the blockSet because it was never removed.
+            ASSERT(m_blockSet.contains(block));
+            m_blockFilter.add(reinterpret_cast<Bits>(block));
             m_toSpace->push(block);
             continue;
         }
 
-        m_toSpaceSet.remove(block);
+        m_blockSet.remove(block);
         m_heap->blockAllocator().deallocate(CopiedBlock::destroy(block));
     }
 
@@ -199,9 +201,12 @@ void CopiedSpace::doneCopying()
         CopiedBlock* next = static_cast<CopiedBlock*>(curr->next());
         if (!curr->m_isPinned) {
             m_oversizeBlocks.remove(curr);
+            m_blockSet.remove(curr);
             CopiedBlock::destroy(curr).deallocate();
-        } else
+        } else {
+            m_blockFilter.add(reinterpret_cast<Bits>(curr));
             curr->m_isPinned = false;
+        }
         curr = next;
     }
 
index d3cc040..438df45 100644 (file)
@@ -64,6 +64,7 @@ public:
     void pin(CopiedBlock*);
     bool isPinned(void*);
 
+    bool contains(CopiedBlock*);
     bool contains(void*, CopiedBlock*&);
 
     size_t size();
@@ -96,9 +97,8 @@ private:
 
     CopiedAllocator m_allocator;
 
-    TinyBloomFilter m_toSpaceFilter;
-    TinyBloomFilter m_oversizeFilter;
-    HashSet<CopiedBlock*> m_toSpaceSet;
+    TinyBloomFilter m_blockFilter;
+    HashSet<CopiedBlock*> m_blockSet;
 
     Mutex m_toSpaceLock;
 
index 2d360ae..c977625 100644 (file)
 
 namespace JSC {
 
+inline bool CopiedSpace::contains(CopiedBlock* block)
+{
+    return !m_blockFilter.ruleOut(reinterpret_cast<Bits>(block)) && m_blockSet.contains(block);
+}
+
 inline bool CopiedSpace::contains(void* ptr, CopiedBlock*& result)
 {
     CopiedBlock* block = blockFor(ptr);
+    if (contains(block)) {
+        result = block;
+        return true;
+    }
+    block = oversizeBlockFor(ptr);
     result = block;
-    return !m_toSpaceFilter.ruleOut(reinterpret_cast<Bits>(block)) && m_toSpaceSet.contains(block);
+    return contains(block);
 }
 
 inline void CopiedSpace::pin(CopiedBlock* block)
@@ -53,7 +63,7 @@ inline void CopiedSpace::startedCopying()
     m_fromSpace = m_toSpace;
     m_toSpace = temp;
 
-    m_toSpaceFilter.reset();
+    m_blockFilter.reset();
     m_allocator.startedCopying();
 
     ASSERT(!m_inCopyingPhase);
@@ -98,8 +108,8 @@ inline CheckedBoolean CopiedSpace::addNewBlock()
         return false;
         
     m_toSpace->push(block);
-    m_toSpaceFilter.add(reinterpret_cast<Bits>(block));
-    m_toSpaceSet.add(block);
+    m_blockFilter.add(reinterpret_cast<Bits>(block));
+    m_blockSet.add(block);
     m_allocator.resetCurrentBlock(block);
     return true;
 }