[webkit-changes] [124265] trunk/Source/JavaScriptCore

Tue, 31 Jul 2012 16:05:05 -0700

Title: [124265] trunk/Source/_javascript_Core
Revision
124265
Author
mhahnenb...@apple.com
Date
2012-07-31 16:05:12 -0700 (Tue, 31 Jul 2012)

Log Message

Structures should be swept after all other objects
https://bugs.webkit.org/show_bug.cgi?id=92679

Reviewed by Filip Pizlo.

In order to get rid of ClassInfo from our objects, we need to be able to safely get the 
ClassInfo during the destruction of objects. We'd like to get the ClassInfo out of the 
Structure, but currently it is not safe to do so because the order of destruction of objects 
is not guaranteed to sweep objects before their corresponding Structure. We can fix this by 
sweeping Structures after everything else.

* heap/Heap.cpp:
(JSC::Heap::isSafeToSweepStructures): Add a function that checks if it is safe to sweep Structures.
If the Heap's IncrementalSweeper member is null, that means we're shutting down this VM and it is 
safe to sweep structures since we'll always do Structures last anyways due to the ordering of 
MarkedSpace::forEachBlock.
(JSC):
(JSC::Heap::didStartVMShutdown): Add this intermediate function to the Heap that ~JSGlobalData now
calls rather than calling the two HeapTimer objects individually. This allows the Heap to null out 
these pointers after it has invalidated them to prevent accidental use-after-free in the sweep() 
calls during lastChanceToFinalize().
* heap/Heap.h:
(Heap):
* heap/HeapTimer.h:
(HeapTimer):
* heap/IncrementalSweeper.cpp:
(JSC::IncrementalSweeper::structuresCanBeSwept): Determines if it is currently safe to sweep Structures.
This decision is based on whether we have gotten to the end of the vector of blocks that need sweeping
the first time.
(JSC):
(JSC::IncrementalSweeper::doSweep): We add a second pass over the vector to sweep Structures after we 
make our first pass. We now null out the slots as we sweep them so that we can quickly find the 
Structures during the second pass.
(JSC::IncrementalSweeper::startSweeping): Initialize our new Structure sweeping index.
(JSC::IncrementalSweeper::willFinishSweeping): Callback that is called by MarkedSpace::sweep to notify 
the IncrementalSweeper that we are going to sweep all of the remaining blocks in the Heap so it can 
assume that everything is taken care of in the correct order. Since MarkedSpace::forEachBlock 
iterates over the Structure blocks after all other blocks, the ordering property for sweeping Structures holds.
(JSC::IncrementalSweeper::IncrementalSweeper): Initialize Structure sweeping index.
* heap/IncrementalSweeper.h: Add declarations for new stuff.
(IncrementalSweeper):
* heap/MarkedAllocator.cpp:
(JSC::MarkedAllocator::tryAllocateHelper): We now check if the current block only contains structures and 
if so and it isn't safe to sweep Structures according to the Heap, we just return early instead of doing 
the normal lazy sweep. If this proves to be too much of a waste in the future we can add an extra clause that 
will sweep some number of other blocks in place of the current block to mitigate the cost of the floating 
Structure garbage.
(JSC::MarkedAllocator::addBlock):
* heap/MarkedAllocator.h:
(JSC::MarkedAllocator::zapFreeList): When we zap the free list in the MarkedAllocator, the current block is no 
longer valid to allocate from, so we set the current block to null.
* heap/MarkedBlock.cpp:
(JSC::MarkedBlock::sweepHelper): Added a couple assertions to make sure that we weren't trying to sweep Structures
at an unsafe time.
* heap/MarkedSpace.cpp:
(JSC::MarkedSpace::sweep): Notify the IncrementalSweeper that the MarkedSpace will finish all currently remaining sweeping.
(JSC): 
* heap/MarkedSpace.h:
(JSC):
* runtime/JSGlobalData.cpp:
(JSC::JSGlobalData::~JSGlobalData): Call the new Heap::didStartVMShutdown.

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (124264 => 124265)


--- trunk/Source/_javascript_Core/ChangeLog	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/ChangeLog	2012-07-31 23:05:12 UTC (rev 124265)
@@ -1,3 +1,67 @@
+2012-07-30  Mark Hahnenberg  <mhahnenb...@apple.com>
+
+        Structures should be swept after all other objects
+        https://bugs.webkit.org/show_bug.cgi?id=92679
+
+        Reviewed by Filip Pizlo.
+
+        In order to get rid of ClassInfo from our objects, we need to be able to safely get the 
+        ClassInfo during the destruction of objects. We'd like to get the ClassInfo out of the 
+        Structure, but currently it is not safe to do so because the order of destruction of objects 
+        is not guaranteed to sweep objects before their corresponding Structure. We can fix this by 
+        sweeping Structures after everything else.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::isSafeToSweepStructures): Add a function that checks if it is safe to sweep Structures.
+        If the Heap's IncrementalSweeper member is null, that means we're shutting down this VM and it is 
+        safe to sweep structures since we'll always do Structures last anyways due to the ordering of 
+        MarkedSpace::forEachBlock.
+        (JSC):
+        (JSC::Heap::didStartVMShutdown): Add this intermediate function to the Heap that ~JSGlobalData now
+        calls rather than calling the two HeapTimer objects individually. This allows the Heap to null out 
+        these pointers after it has invalidated them to prevent accidental use-after-free in the sweep() 
+        calls during lastChanceToFinalize().
+        * heap/Heap.h:
+        (Heap):
+        * heap/HeapTimer.h:
+        (HeapTimer):
+        * heap/IncrementalSweeper.cpp:
+        (JSC::IncrementalSweeper::structuresCanBeSwept): Determines if it is currently safe to sweep Structures.
+        This decision is based on whether we have gotten to the end of the vector of blocks that need sweeping
+        the first time.
+        (JSC):
+        (JSC::IncrementalSweeper::doSweep): We add a second pass over the vector to sweep Structures after we 
+        make our first pass. We now null out the slots as we sweep them so that we can quickly find the 
+        Structures during the second pass.
+        (JSC::IncrementalSweeper::startSweeping): Initialize our new Structure sweeping index.
+        (JSC::IncrementalSweeper::willFinishSweeping): Callback that is called by MarkedSpace::sweep to notify 
+        the IncrementalSweeper that we are going to sweep all of the remaining blocks in the Heap so it can 
+        assume that everything is taken care of in the correct order. Since MarkedSpace::forEachBlock 
+        iterates over the Structure blocks after all other blocks, the ordering property for sweeping Structures holds.
+        (JSC::IncrementalSweeper::IncrementalSweeper): Initialize Structure sweeping index.
+        * heap/IncrementalSweeper.h: Add declarations for new stuff.
+        (IncrementalSweeper):
+        * heap/MarkedAllocator.cpp:
+        (JSC::MarkedAllocator::tryAllocateHelper): We now check if the current block only contains structures and 
+        if so and it isn't safe to sweep Structures according to the Heap, we just return early instead of doing 
+        the normal lazy sweep. If this proves to be too much of a waste in the future we can add an extra clause that 
+        will sweep some number of other blocks in place of the current block to mitigate the cost of the floating 
+        Structure garbage.
+        (JSC::MarkedAllocator::addBlock):
+        * heap/MarkedAllocator.h:
+        (JSC::MarkedAllocator::zapFreeList): When we zap the free list in the MarkedAllocator, the current block is no 
+        longer valid to allocate from, so we set the current block to null.
+        * heap/MarkedBlock.cpp:
+        (JSC::MarkedBlock::sweepHelper): Added a couple assertions to make sure that we weren't trying to sweep Structures
+        at an unsafe time.
+        * heap/MarkedSpace.cpp:
+        (JSC::MarkedSpace::sweep): Notify the IncrementalSweeper that the MarkedSpace will finish all currently remaining sweeping.
+        (JSC): 
+        * heap/MarkedSpace.h:
+        (JSC):
+        * runtime/JSGlobalData.cpp:
+        (JSC::JSGlobalData::~JSGlobalData): Call the new Heap::didStartVMShutdown.
+
 2012-07-31  Geoffrey Garen  <gga...@apple.com>
 
         Fix all the other builds I just broke. Maybe fix the Windows build.
@@ -99,6 +163,7 @@
         (MarkedBlock):
         (JSC::MarkedBlock::capacity): Updated for encapsulation.
 
+>>>>>>> .r124259
 2012-07-31  Filip Pizlo  <fpi...@apple.com>
 
         DFG OSR exit profiling has unusual oversights

Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (124264 => 124265)


--- trunk/Source/_javascript_Core/heap/Heap.cpp	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp	2012-07-31 23:05:12 UTC (rev 124265)
@@ -830,4 +830,18 @@
     m_compiledCode.append(executable);
 }
 
+bool Heap::isSafeToSweepStructures()
+{
+    return !m_sweeper || m_sweeper->structuresCanBeSwept();
+}
+
+void Heap::didStartVMShutdown()
+{
+    m_activityCallback->didStartVMShutdown();
+    m_activityCallback = 0;
+    m_sweeper->didStartVMShutdown();
+    m_sweeper = 0;
+    lastChanceToFinalize();
+}
+
 } // namespace JSC

Modified: trunk/Source/_javascript_Core/heap/Heap.h (124264 => 124265)


--- trunk/Source/_javascript_Core/heap/Heap.h	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/heap/Heap.h	2012-07-31 23:05:12 UTC (rev 124265)
@@ -168,6 +168,8 @@
         void didAbandon(size_t);
 
         bool isPagedOut(double deadline);
+        bool isSafeToSweepStructures();
+        void didStartVMShutdown();
 
     private:
         friend class CodeBlock;

Modified: trunk/Source/_javascript_Core/heap/IncrementalSweeper.cpp (124264 => 124265)


--- trunk/Source/_javascript_Core/heap/IncrementalSweeper.cpp	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/heap/IncrementalSweeper.cpp	2012-07-31 23:05:12 UTC (rev 124265)
@@ -51,6 +51,7 @@
 IncrementalSweeper::IncrementalSweeper(Heap* heap, CFRunLoopRef runLoop)
     : HeapTimer(heap->globalData(), runLoop)
     , m_currentBlockToSweepIndex(0)
+    , m_structuresCanBeSwept(false)
 {
 }
 
@@ -73,6 +74,11 @@
 {
     while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) {
         MarkedBlock* block = m_blocksToSweep[m_currentBlockToSweepIndex++];
+        if (block->onlyContainsStructures())
+            m_structuresCanBeSwept = true;
+        else
+            ASSERT(!m_structuresCanBeSwept);
+
         if (!block->needsSweeping())
             continue;
 
@@ -93,15 +99,28 @@
 
 void IncrementalSweeper::startSweeping(const HashSet<MarkedBlock*>& blockSnapshot)
 {
-    WTF::copyToVector(blockSnapshot, m_blocksToSweep);
+    m_blocksToSweep.resize(blockSnapshot.size());
+    CopyFunctor functor(m_blocksToSweep);
+    m_globalData->heap.objectSpace().forEachBlock(functor);
     m_currentBlockToSweepIndex = 0;
+    m_structuresCanBeSwept = false;
     scheduleTimer();
 }
 
+void IncrementalSweeper::willFinishSweeping()
+{
+    m_currentBlockToSweepIndex = 0;
+    m_structuresCanBeSwept = true;
+    m_blocksToSweep.clear();
+    if (m_globalData)
+        cancelTimer();
+}
+
 #else
 
 IncrementalSweeper::IncrementalSweeper(JSGlobalData* globalData)
     : HeapTimer(globalData)
+    , m_structuresCanBeSwept(false)
 {
 }
 
@@ -116,8 +135,19 @@
 
 void IncrementalSweeper::startSweeping(const HashSet<MarkedBlock*>&)
 {
+    m_structuresCanBeSwept = false;
 }
-    
+
+void IncrementalSweeper::willFinishSweeping()
+{
+    m_structuresCanBeSwept = true;
+}
+
 #endif
 
+bool IncrementalSweeper::structuresCanBeSwept()
+{
+    return m_structuresCanBeSwept;
+}
+
 } // namespace JSC

Modified: trunk/Source/_javascript_Core/heap/IncrementalSweeper.h (124264 => 124265)


--- trunk/Source/_javascript_Core/heap/IncrementalSweeper.h	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/heap/IncrementalSweeper.h	2012-07-31 23:05:12 UTC (rev 124265)
@@ -36,12 +36,27 @@
 namespace JSC {
 
 class Heap;
-    
+
+struct CopyFunctor : public MarkedBlock::VoidFunctor {
+    CopyFunctor(Vector<MarkedBlock*>& blocks) 
+        : m_index(0) 
+        , m_blocks(blocks)
+    {
+    }
+
+    void operator()(MarkedBlock* block) { m_blocks[m_index++] = block; }
+
+    size_t m_index;
+    Vector<MarkedBlock*>& m_blocks;
+};
+
 class IncrementalSweeper : public HeapTimer {
 public:
     static IncrementalSweeper* create(Heap*);
     void startSweeping(const HashSet<MarkedBlock*>& blockSnapshot);
     virtual void doWork();
+    bool structuresCanBeSwept();
+    void willFinishSweeping();
 
 private:
 #if USE(CF)
@@ -58,6 +73,7 @@
     IncrementalSweeper(JSGlobalData*);
     
 #endif
+    bool m_structuresCanBeSwept;
 };
 
 } // namespace JSC

Modified: trunk/Source/_javascript_Core/heap/MarkedAllocator.cpp (124264 => 124265)


--- trunk/Source/_javascript_Core/heap/MarkedAllocator.cpp	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/heap/MarkedAllocator.cpp	2012-07-31 23:05:12 UTC (rev 124265)
@@ -3,6 +3,7 @@
 
 #include "GCActivityCallback.h"
 #include "Heap.h"
+#include "IncrementalSweeper.h"
 #include "JSGlobalData.h"
 #include <wtf/CurrentTime.h>
 
@@ -29,6 +30,14 @@
 inline void* MarkedAllocator::tryAllocateHelper()
 {
     if (!m_freeList.head) {
+        if (m_onlyContainsStructures && !m_heap->isSafeToSweepStructures()) {
+            if (m_currentBlock) {
+                m_currentBlock->didConsumeFreeList();
+                m_currentBlock = 0;
+            }
+            return 0;
+        }
+
         for (MarkedBlock*& block = m_blocksToSweep; block; block = block->next()) {
             m_freeList = block->sweep(MarkedBlock::SweepToFreeList);
             if (m_freeList.head) {
@@ -104,7 +113,6 @@
 void MarkedAllocator::addBlock(MarkedBlock* block)
 {
     ASSERT(!m_currentBlock);
-    ASSERT(!m_blocksToSweep);
     ASSERT(!m_freeList.head);
     
     m_blockList.append(block);

Modified: trunk/Source/_javascript_Core/heap/MarkedAllocator.h (124264 => 124265)


--- trunk/Source/_javascript_Core/heap/MarkedAllocator.h	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/heap/MarkedAllocator.h	2012-07-31 23:05:12 UTC (rev 124265)
@@ -101,6 +101,7 @@
     }
     
     m_currentBlock->zapFreeList(m_freeList);
+    m_currentBlock = 0;
     m_freeList = MarkedBlock::FreeList();
 }

Modified: trunk/Source/_javascript_Core/heap/MarkedBlock.cpp (124264 => 124265)


--- trunk/Source/_javascript_Core/heap/MarkedBlock.cpp	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/heap/MarkedBlock.cpp	2012-07-31 23:05:12 UTC (rev 124265)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "MarkedBlock.h"
 
+#include "IncrementalSweeper.h"
 #include "JSCell.h"
 #include "JSObject.h"
 #include "ScopeChain.h"
@@ -131,10 +132,12 @@
         ASSERT_NOT_REACHED();
         return FreeList();
     case Marked:
+        ASSERT(!m_onlyContainsStructures || heap()->isSafeToSweepStructures());
         return sweepMode == SweepToFreeList
             ? specializedSweep<Marked, SweepToFreeList, destructorCallNeeded>()
             : specializedSweep<Marked, SweepOnly, destructorCallNeeded>();
     case Zapped:
+        ASSERT(!m_onlyContainsStructures || heap()->isSafeToSweepStructures());
         return sweepMode == SweepToFreeList
             ? specializedSweep<Zapped, SweepToFreeList, destructorCallNeeded>()
             : specializedSweep<Zapped, SweepOnly, destructorCallNeeded>();

Modified: trunk/Source/_javascript_Core/heap/MarkedSpace.cpp (124264 => 124265)


--- trunk/Source/_javascript_Core/heap/MarkedSpace.cpp	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/heap/MarkedSpace.cpp	2012-07-31 23:05:12 UTC (rev 124265)
@@ -21,6 +21,7 @@
 #include "config.h"
 #include "MarkedSpace.h"
 
+#include "IncrementalSweeper.h"
 #include "JSGlobalObject.h"
 #include "JSLock.h"
 #include "JSObject.h"
@@ -108,6 +109,12 @@
     forEachBlock<LastChanceToFinalize>();
 }
 
+void MarkedSpace::sweep()
+{
+    m_heap->sweeper()->willFinishSweeping();
+    forEachBlock<Sweep>();
+}
+
 void MarkedSpace::resetAllocators()
 {
     for (size_t cellSize = preciseStep; cellSize <= preciseCutoff; cellSize += preciseStep) {

Modified: trunk/Source/_javascript_Core/heap/MarkedSpace.h (124264 => 124265)


--- trunk/Source/_javascript_Core/heap/MarkedSpace.h	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/heap/MarkedSpace.h	2012-07-31 23:05:12 UTC (rev 124265)
@@ -235,11 +235,6 @@
     forEachBlock<ClearMarks>();
 }
 
-inline void MarkedSpace::sweep()
-{
-    forEachBlock<Sweep>();
-}
-
 inline size_t MarkedSpace::objectCount()
 {
     return forEachBlock<MarkCount>();

Modified: trunk/Source/_javascript_Core/runtime/JSGlobalData.cpp (124264 => 124265)


--- trunk/Source/_javascript_Core/runtime/JSGlobalData.cpp	2012-07-31 23:04:48 UTC (rev 124264)
+++ trunk/Source/_javascript_Core/runtime/JSGlobalData.cpp	2012-07-31 23:05:12 UTC (rev 124265)
@@ -223,9 +223,7 @@
 JSGlobalData::~JSGlobalData()
 {
     ASSERT(!m_apiLock.currentThreadIsHoldingLock());
-    heap.activityCallback()->didStartVMShutdown();
-    heap.sweeper()->didStartVMShutdown();
-    heap.lastChanceToFinalize();
+    heap.didStartVMShutdown();
 
     delete interpreter;
 #ifndef NDEBUG

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to