2011-01-21 Geoffrey Garen <ggaren@apple.com>
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Jan 2011 04:27:18 +0000 (04:27 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Jan 2011 04:27:18 +0000 (04:27 +0000)
        Reviewed by Maciej Stachowiak.

        Cleaned up some conservative marking code.
        https://bugs.webkit.org/show_bug.cgi?id=52946

        SunSpider reports no change.

        * interpreter/RegisterFile.h: No need for a special marking function,
        since we already expose a start() and end().

        * runtime/Heap.cpp:
        (JSC::Heap::registerFile):
        (JSC::Heap::markRoots):
        * runtime/Heap.h:
        (JSC::Heap::contains): Migrated markConservatively() to the machine stack
        marker class. Now, Heap just provides a contains() function, which the
        machine stack marker uses for checking whether a pointer points into the heap.

        * runtime/MachineStackMarker.cpp:
        (JSC::MachineStackMarker::markCurrentThreadConservativelyInternal):
        (JSC::MachineStackMarker::markOtherThreadConservatively):
        (JSC::isPointerAligned):
        (JSC::MachineStackMarker::markConservatively):
        * runtime/MachineStackMarker.h: Move the conservative marking code here.

        * runtime/MarkStack.h:
        (JSC::ConservativeSet::add):
        (JSC::ConservativeSet::mark): Changed to using a vector instead of hash
        set. Vector seems to be a bit faster, and it generates smaller code.

        * runtime/MarkedSpace.cpp:
        (JSC::MarkedSpace::containsSlowCase):
        * runtime/MarkedSpace.h:
        (JSC::MarkedSpace::isCellAligned):
        (JSC::MarkedSpace::isPossibleCell):
        (JSC::MarkedSpace::contains): Kept the code for determining whether a
        pointer pointed into marked space, and moved the code for marking
        a set of conservative pointers into the machine stack marker.

        * wtf/HashSet.h:
        (WTF::::add): Added two missing inlines that I noticed while testing
        vector vs hash set.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/interpreter/RegisterFile.h
Source/JavaScriptCore/runtime/Heap.cpp
Source/JavaScriptCore/runtime/Heap.h
Source/JavaScriptCore/runtime/MachineStackMarker.cpp
Source/JavaScriptCore/runtime/MachineStackMarker.h
Source/JavaScriptCore/runtime/MarkStack.h
Source/JavaScriptCore/runtime/MarkedSpace.cpp
Source/JavaScriptCore/runtime/MarkedSpace.h
Source/JavaScriptCore/wtf/HashSet.h

index 6115bde30652301955727d4fed09af9b0188e23a..40bf8e81dc553d91e4f8577f4638cb21df085662 100644 (file)
@@ -1,3 +1,48 @@
+2011-01-21  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Maciej Stachowiak.
+
+        Cleaned up some conservative marking code.
+        https://bugs.webkit.org/show_bug.cgi?id=52946
+        
+        SunSpider reports no change.
+
+        * interpreter/RegisterFile.h: No need for a special marking function,
+        since we already expose a start() and end().
+
+        * runtime/Heap.cpp:
+        (JSC::Heap::registerFile):
+        (JSC::Heap::markRoots):
+        * runtime/Heap.h:
+        (JSC::Heap::contains): Migrated markConservatively() to the machine stack
+        marker class. Now, Heap just provides a contains() function, which the
+        machine stack marker uses for checking whether a pointer points into the heap.
+
+        * runtime/MachineStackMarker.cpp:
+        (JSC::MachineStackMarker::markCurrentThreadConservativelyInternal):
+        (JSC::MachineStackMarker::markOtherThreadConservatively):
+        (JSC::isPointerAligned):
+        (JSC::MachineStackMarker::markConservatively):
+        * runtime/MachineStackMarker.h: Move the conservative marking code here.
+
+        * runtime/MarkStack.h:
+        (JSC::ConservativeSet::add):
+        (JSC::ConservativeSet::mark): Changed to using a vector instead of hash
+        set. Vector seems to be a bit faster, and it generates smaller code.
+
+        * runtime/MarkedSpace.cpp:
+        (JSC::MarkedSpace::containsSlowCase):
+        * runtime/MarkedSpace.h:
+        (JSC::MarkedSpace::isCellAligned):
+        (JSC::MarkedSpace::isPossibleCell):
+        (JSC::MarkedSpace::contains): Kept the code for determining whether a
+        pointer pointed into marked space, and moved the code for marking
+        a set of conservative pointers into the machine stack marker.
+
+        * wtf/HashSet.h:
+        (WTF::::add): Added two missing inlines that I noticed while testing
+        vector vs hash set.
+
 2011-01-21  Mark Rowe  <mrowe@apple.com>
 
         Reviewed by Sam Weinig.
index 9dfc432ad7f912bdc604de70923cc60695fddd98..e9c6df1e5c79ca7fdf29d5a84fe1c03522a2836d 100644 (file)
@@ -132,8 +132,6 @@ namespace JSC {
 
         Register* lastGlobal() const { return m_start - m_numGlobals; }
         
-        void markCallFrames(ConservativeSet& conservativeSet, Heap* heap) { heap->markConservatively(conservativeSet, m_start, m_end); }
-
         static size_t committedByteCount();
         static void initializeThreading();
 
index 39663247be17971b63cab20ba2c8a6573b59b72c..89f954bd021411df25470b128257b88c3f5b1f34 100644 (file)
@@ -147,11 +147,6 @@ void* Heap::allocate(size_t s)
     return result;
 }
 
-void Heap::markConservatively(ConservativeSet& conservativeSet, void* start, void* end)
-{
-    m_markedSpace.markConservatively(conservativeSet, start, end);
-}
-
 void Heap::updateWeakGCHandles()
 {
     for (unsigned i = 0; i < m_weakGCHandlePools.size(); ++i)
@@ -237,7 +232,12 @@ void Heap::markTempSortVectors(MarkStack& markStack)
         }
     }
 }
-    
+
+inline RegisterFile& Heap::registerFile()
+{
+    return m_globalData->interpreter->registerFile();
+}
+
 void Heap::markRoots()
 {
 #ifndef NDEBUG
@@ -258,7 +258,7 @@ void Heap::markRoots()
     // determine whether a reference is valid.
     ConservativeSet conservativeSet;
     m_machineStackMarker.markMachineStackConservatively(conservativeSet);
-    m_globalData->interpreter->registerFile().markCallFrames(conservativeSet, this);
+    m_machineStackMarker.markConservatively(conservativeSet, registerFile().start(), registerFile().end());
 
     // Reset mark bits.
     m_markedSpace.clearMarkBits();
index f7f4deb8c24789e1a6cc00e66484e88ff5eebd95..db05d81782d8e2fbc5244e6199c8a75c84a1c3db 100644 (file)
 
 namespace JSC {
 
-    class JSValue;
-    class UString;
     class GCActivityCallback;
     class JSCell;
     class JSGlobalData;
     class JSValue;
+    class JSValue;
     class LiveObjectIterator;
-    class MarkedArgumentBuffer;
     class MarkStack;
+    class MarkedArgumentBuffer;
+    class RegisterFile;
+    class UString;
     class WeakGCHandlePool;
 
     typedef std::pair<JSValue, UString> ValueStringPair;
@@ -85,11 +86,11 @@ namespace JSC {
         static bool isCellMarked(const JSCell*);
         static bool checkMarkCell(const JSCell*);
         static void markCell(JSCell*);
+        
+        bool contains(void*);
 
         WeakGCHandle* addWeakGCHandle(JSCell*);
 
-        void markConservatively(ConservativeSet&, void* start, void* end);
-
         void pushTempSortVector(WTF::Vector<ValueStringPair>*);
         void popTempSortVector(WTF::Vector<ValueStringPair>*);        
 
@@ -118,6 +119,8 @@ namespace JSC {
         void updateWeakGCHandles();
         WeakGCHandlePool* weakGCHandlePool(size_t index);
 
+        RegisterFile& registerFile();
+
         MarkedSpace m_markedSpace;
         OperationInProgress m_operationInProgress;
 
@@ -152,6 +155,11 @@ namespace JSC {
         MarkedSpace::markCell(cell);
     }
 
+    inline bool Heap::contains(void* p)
+    {
+        return m_markedSpace.contains(p);
+    }
+
     inline void Heap::reportExtraMemoryCost(size_t cost)
     {
         if (cost > minExtraCost) 
index e52f4026c3cf4568bc9a777021f08ad63b800dab..4e3408d85be8d785e6d0f5d89e6d8fb71132bd6a 100644 (file)
@@ -196,7 +196,7 @@ void MachineStackMarker::unregisterThread()
 
 void NEVER_INLINE MachineStackMarker::markCurrentThreadConservativelyInternal(ConservativeSet& conservativeSet)
 {
-    m_heap->markConservatively(conservativeSet, m_heap->globalData()->stack().current(), m_heap->globalData()->stack().origin());
+    markConservatively(conservativeSet, m_heap->globalData()->stack().current(), m_heap->globalData()->stack().origin());
 }
 
 #if COMPILER(GCC)
@@ -358,10 +358,10 @@ void MachineStackMarker::markOtherThreadConservatively(ConservativeSet& conserva
     size_t regSize = getPlatformThreadRegisters(thread->platformThread, regs);
 
     // mark the thread's registers
-    m_heap->markConservatively(conservativeSet, static_cast<void*>(&regs), static_cast<void*>(reinterpret_cast<char*>(&regs) + regSize));
+    markConservatively(conservativeSet, static_cast<void*>(&regs), static_cast<void*>(reinterpret_cast<char*>(&regs) + regSize));
 
     void* stackPointer = otherThreadStackPointer(regs);
-    m_heap->markConservatively(conservativeSet, stackPointer, thread->stackBase);
+    markConservatively(conservativeSet, stackPointer, thread->stackBase);
 
     resumeThread(thread->platformThread);
 }
@@ -397,4 +397,36 @@ void MachineStackMarker::markMachineStackConservatively(ConservativeSet& conserv
 #endif
 }
 
+inline bool isPointerAligned(void* p)
+{
+    return (((intptr_t)(p) & (sizeof(char*) - 1)) == 0);
+}
+
+void MachineStackMarker::markConservatively(ConservativeSet& conservativeSet, void* start, void* end)
+{
+#if OS(WINCE)
+    if (start > end) {
+        void* tmp = start;
+        start = end;
+        end = tmp;
+    }
+#else
+    ASSERT(start <= end);
+#endif
+
+    ASSERT((static_cast<char*>(end) - static_cast<char*>(start)) < 0x1000000);
+    ASSERT(isPointerAligned(start));
+    ASSERT(isPointerAligned(end));
+
+    char** p = static_cast<char**>(start);
+    char** e = static_cast<char**>(end);
+
+    while (p != e) {
+        char* x = *p++;
+        if (!m_heap->contains(x))
+            continue;
+        conservativeSet.add(reinterpret_cast<JSCell*>(x));
+    }
+}
+
 } // namespace JSC
index 8afdb46736d1d5da8cac0032c7139968744786b3..a1f31737edaf076c18e33694c8cd0cc737289ca9 100644 (file)
@@ -41,6 +41,7 @@ namespace JSC {
         ~MachineStackMarker();
 
         void markMachineStackConservatively(ConservativeSet&);
+        void markConservatively(ConservativeSet&, void* start, void* end);
 
 #if ENABLE(JSC_MULTIPLE_THREADS)
         void makeUsableFromMultipleThreads();
index 7946e65508d1d7640423ba11ee590b7ce15bd7d6..58dfa4c72c7ab71d641efca38b7316de9084113a 100644 (file)
@@ -27,7 +27,7 @@
 #define MarkStack_h
 
 #include "JSValue.h"
-#include <wtf/HashSet.h>
+#include <wtf/Vector.h>
 #include <wtf/Noncopyable.h>
 #include <wtf/OSAllocator.h>
 
@@ -190,16 +190,15 @@ namespace JSC {
     
     class ConservativeSet {
     public:
-        void add(JSCell* cell) { m_set.add(cell); }
+        void add(JSCell* cell) { m_vector.append(cell); }
         void mark(MarkStack& markStack)
         {
-            HashSet<JSCell*>::iterator end = m_set.end();
-            for (HashSet<JSCell*>::iterator it = m_set.begin(); it != end; ++it)
-                markStack.append(*it);
+            for (size_t i = 0; i < m_vector.size(); ++i)
+                markStack.append(m_vector[i]);
         }
 
     private:
-        HashSet<JSCell*> m_set;
+        Vector<JSCell*, 64> m_vector;
     };
 }
 
index 036c8f0e90c6dacd3be5f509d80ff85212020b17..c020617a896085261e1ac71b102f6db049796d29 100644 (file)
@@ -200,82 +200,37 @@ void MarkedSpace::shrinkBlocks(size_t neededBlocks)
         m_heap.collectorBlock(i)->marked.set(HeapConstants::cellsPerBlock - 1);
 }
 
-inline bool isPointerAligned(void* p)
+bool MarkedSpace::containsSlowCase(void* x)
 {
-    return (((intptr_t)(p) & (sizeof(char*) - 1)) == 0);
-}
-
-// Cell size needs to be a power of two for isPossibleCell to be valid.
-COMPILE_ASSERT(sizeof(CollectorCell) % 2 == 0, Collector_cell_size_is_power_of_two);
-
-static inline bool isCellAligned(void *p)
-{
-    return (((intptr_t)(p) & CELL_MASK) == 0);
-}
-
-static inline bool isPossibleCell(void* p)
-{
-    return isCellAligned(p) && p;
-}
-
-void MarkedSpace::markConservatively(ConservativeSet& conservativeSet, void* start, void* end)
-{
-#if OS(WINCE)
-    if (start > end) {
-        void* tmp = start;
-        start = end;
-        end = tmp;
-    }
-#else
-    ASSERT(start <= end);
-#endif
-
-    ASSERT((static_cast<char*>(end) - static_cast<char*>(start)) < 0x1000000);
-    ASSERT(isPointerAligned(start));
-    ASSERT(isPointerAligned(end));
-
-    char** p = static_cast<char**>(start);
-    char** e = static_cast<char**>(end);
-
-    while (p != e) {
-        char* x = *p++;
-        if (isPossibleCell(x)) {
-            uintptr_t xAsBits = reinterpret_cast<uintptr_t>(x);
-            xAsBits &= CELL_ALIGN_MASK;
-
-            uintptr_t offset = xAsBits & BLOCK_OFFSET_MASK;
-            const size_t lastCellOffset = sizeof(CollectorCell) * (CELLS_PER_BLOCK - 1);
-            if (offset > lastCellOffset)
-                continue;
-
-            CollectorBlock* blockAddr = reinterpret_cast<CollectorBlock*>(xAsBits - offset);
-            size_t usedBlocks = m_heap.usedBlocks;
-            for (size_t block = 0; block < usedBlocks; block++) {
-                if (m_heap.collectorBlock(block) != blockAddr)
-                    continue;
-
-                // x is a pointer into the heap. Now, verify that the cell it
-                // points to is live. (If the cell is dead, we must not mark it,
-                // since that would revive it in a zombie state.)
-                if (block < m_heap.nextBlock) {
-                    conservativeSet.add(reinterpret_cast<JSCell*>(xAsBits));
-                    break;
-                }
-                
-                size_t cellOffset = offset / CELL_SIZE;
-                
-                if (block == m_heap.nextBlock && cellOffset < m_heap.nextCell) {
-                    conservativeSet.add(reinterpret_cast<JSCell*>(xAsBits));
-                    break;
-                }
-                
-                if (blockAddr->marked.get(cellOffset)) {
-                    conservativeSet.add(reinterpret_cast<JSCell*>(xAsBits));
-                    break;
-                }
-            }
-        }
+    uintptr_t xAsBits = reinterpret_cast<uintptr_t>(x);
+    xAsBits &= CELL_ALIGN_MASK;
+
+    uintptr_t offset = xAsBits & BLOCK_OFFSET_MASK;
+    const size_t lastCellOffset = sizeof(CollectorCell) * (CELLS_PER_BLOCK - 1);
+    if (offset > lastCellOffset)
+        return false;
+
+    CollectorBlock* blockAddr = reinterpret_cast<CollectorBlock*>(xAsBits - offset);
+    size_t usedBlocks = m_heap.usedBlocks;
+    for (size_t block = 0; block < usedBlocks; block++) {
+        if (m_heap.collectorBlock(block) != blockAddr)
+            continue;
+
+        // x is a pointer into the heap. Now, verify that the cell it
+        // points to is live. (If the cell is dead, we must not mark it,
+        // since that would revive it in a zombie state.)
+        if (block < m_heap.nextBlock)
+            return true;
+        
+        size_t cellOffset = offset / CELL_SIZE;
+        
+        if (block == m_heap.nextBlock && cellOffset < m_heap.nextCell)
+            return true;
+        
+        return blockAddr->marked.get(cellOffset);
     }
+    
+    return false;
 }
 
 void MarkedSpace::clearMarkBits()
index af312b574c9b2eb8f9d6a5e12aca55b2b7b97704..63576641fab34c2dbdade0bec916d3a03268ba38 100644 (file)
@@ -85,10 +85,11 @@ namespace JSC {
 
         WeakGCHandle* addWeakGCHandle(JSCell*);
 
-        void markConservatively(ConservativeSet&, void* start, void* end);
+        bool contains(void*);
+        bool containsSlowCase(void*);
+        bool isCellAligned(void*);
+        bool isPossibleCell(void*);
 
-        static bool isNumber(JSCell*);
-        
         LiveObjectIterator primaryHeapBegin();
         LiveObjectIterator primaryHeapEnd();
 
@@ -217,6 +218,27 @@ namespace JSC {
         cellBlock(cell)->marked.set(cellOffset(cell));
     }
 
+    // Cell size needs to be a power of two for isPossibleCell to be valid.
+    COMPILE_ASSERT(sizeof(CollectorCell) % 2 == 0, Collector_cell_size_is_power_of_two);
+
+    inline bool MarkedSpace::isCellAligned(void *p)
+    {
+        return (((intptr_t)(p) & CELL_MASK) == 0);
+    }
+
+    inline bool MarkedSpace::isPossibleCell(void* p)
+    {
+        return isCellAligned(p) && p;
+    }
+
+    inline bool MarkedSpace::contains(void* x)
+    {
+        if (!isPossibleCell(x))
+            return false;
+            
+        return containsSlowCase(x);
+    }
+
 } // namespace JSC
 
 #endif // MarkedSpace_h
index be6b93d4b3f20431645b8ba4d8b5bace46a90ac6..82245f3ecdd8845dbae28e659f5583da95159b83 100644 (file)
@@ -175,14 +175,14 @@ namespace WTF {
     }
 
     template<typename T, typename U, typename V>
-    pair<typename HashSet<T, U, V>::iterator, bool> HashSet<T, U, V>::add(const ValueType& value)
+    inline pair<typename HashSet<T, U, V>::iterator, bool> HashSet<T, U, V>::add(const ValueType& value)
     {
         return m_impl.add(value);
     }
 
     template<typename Value, typename HashFunctions, typename Traits>
     template<typename T, typename HashTranslator>
-    pair<typename HashSet<Value, HashFunctions, Traits>::iterator, bool>
+    inline pair<typename HashSet<Value, HashFunctions, Traits>::iterator, bool>
     HashSet<Value, HashFunctions, Traits>::add(const T& value)
     {
         typedef HashSetTranslatorAdapter<ValueType, ValueTraits, T, HashTranslator> Adapter;