Revert of Revert "Revert of [oilpan]: Change marking to do precise ro…

…ots first and conservative second. (http… (patchset #2 of https://codereview.chromium.org/413133006/) Reason for revert: This breaks strongification of ephemerons when there are pointers on the stack. See: https://codereview.chromium.org/461413002/ for details. Original issue's description: > Revert "Revert of [oilpan]: Change marking to do precise roots first and conservative second. (https://codereview.chromium.org/405403003/)" > > This reverts commit 765af3a. > > AFAICT the CrossThreadPointerToOrphanedPage test was a bit flaky in the sense that the compiler was free to reuse the slot where the stackPtrValue was located after the RELEASE_ASSERT. However the test assumed the stackPtrValue was on the stack (to mimic a rogue integer value finding a dead object) when doing the conservative GC further down. > After my change to Heap::CollectGarbage it seemed the android compiler no longer kept the value around and hence the test started failing. I have now moved the RELEASE_ASSERT down to after the point where we do the conservative GC which makes the test pass on android again. > > I uploaded the unfixed diff first and then a second diff (moved RELEASE_ASSERT) with the fix. > > [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected] > BUG= > > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178845 NOTRY=true NOTREECHECKS=true Review URL: https://codereview.chromium.org/464253002 git-svn-id: svn://svn.chromium.org/blink/trunk@180160 bbb929c8-8fbe-4397-9dbb-9b2b20218538
yoavweiss · Aug 13, 2014 · 8cfae43 · 8cfae43
1 parent c374ef0
commit 8cfae43
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 65 deletions.
diff --git a/Source/platform/heap/Heap.cpp b/Source/platform/heap/Heap.cpp
@@ -2082,23 +2082,7 @@ void Heap::collectGarbage(ThreadState::StackState stackState)
 
     prepareForGC();
 
-    // 1. trace persistent roots.
-    ThreadState::visitPersistentRoots(s_markingVisitor);
-
-    // 2. trace objects reachable from the persistent roots including ephemerons.
-    processMarkingStack<GlobalMarking>();
-
-    // 3. trace objects reachable from the stack. We do this independent of the
-    // given stackState since other threads might have a different stack state.
-    ThreadState::visitStackRoots(s_markingVisitor);
-
-    // 4. trace objects reachable from the stack "roots" including ephemerons.
-    // Only do the processing if we found a pointer to an object on one of the
-    // thread stacks.
-    if (lastGCWasConservative())
-        processMarkingStack<GlobalMarking>();
-
-    globalWeakProcessingAndCleanup();
+    traceRootsAndPerformGlobalWeakProcessing<GlobalMarking>();
 
     // After a global marking we know that any orphaned page that was not reached
     // cannot be reached in a subsequent GC. This is due to a thread either having
@@ -2138,38 +2122,26 @@ void Heap::collectGarbageForTerminatingThread(ThreadState* state)
         state->enterGC();
         state->prepareForGC();
 
-        // 1. trace the thread local persistent roots. For thread local GCs we
-        // don't trace the stack (ie. no conservative scanning) since this is
-        // only called during thread shutdown where there should be no objects
-        // on the stack.
-        // We also assume that orphaned pages have no objects reachable from
-        // persistent handles on other threads or CrossThreadPersistents. The
-        // only cases where this could happen is if a subsequent conservative
-        // global GC finds a "pointer" on the stack or due to a programming
-        // error where an object has a dangling cross-thread pointer to an
-        // object on this heap.
-        state->visitPersistents(s_markingVisitor);
-
-        // 2. trace objects reachable from the thread's persistent roots
-        // including ephemerons.
-        processMarkingStack<ThreadLocalMarking>();
-
-        globalWeakProcessingAndCleanup();
+        traceRootsAndPerformGlobalWeakProcessing<ThreadLocalMarking>();
 
         state->leaveGC();
     }
     state->performPendingSweep();
 }
 
 template<CallbackInvocationMode Mode>
-void Heap::processMarkingStack()
+void Heap::traceRootsAndPerformGlobalWeakProcessing()
 {
+    if (Mode == ThreadLocalMarking)
+        ThreadState::current()->visitLocalRoots(s_markingVisitor);
+    else
+        ThreadState::visitRoots(s_markingVisitor);
+
     // Ephemeron fixed point loop.
     do {
-        // Iteratively mark all objects that are reachable from the objects
-        // currently pushed onto the marking stack. If Mode is ThreadLocalMarking
-        // don't continue tracing if the trace hits an object on another thread's
-        // heap.
+        // Recursively mark all objects that are reachable from the roots for
+        // this thread. If Mode is ThreadLocalMarking don't continue tracing if
+        // the trace hits an object on another thread's heap.
         while (popAndInvokeTraceCallback<Mode>(s_markingVisitor)) { }
 
         // Mark any strong pointers that have now become reachable in ephemeron
@@ -2178,10 +2150,7 @@ void Heap::processMarkingStack()
 
         // Rerun loop if ephemeron processing queued more objects for tracing.
     } while (!s_markingStack->isEmpty());
-}
 
-void Heap::globalWeakProcessingAndCleanup()
-{
     // Call weak callbacks on objects that may now be pointing to dead
     // objects and call ephemeronIterationDone callbacks on weak tables
     // to do cleanup (specifically clear the queued bits for weak hash

diff --git a/Source/platform/heap/Heap.h b/Source/platform/heap/Heap.h
@@ -969,8 +969,7 @@ class PLATFORM_EXPORT Heap {
     static void collectGarbage(ThreadState::StackState);
     static void collectGarbageForTerminatingThread(ThreadState*);
     static void collectAllGarbage();
-    template<CallbackInvocationMode Mode> static void processMarkingStack();
-    static void globalWeakProcessingAndCleanup();
+    template<CallbackInvocationMode Mode> static void traceRootsAndPerformGlobalWeakProcessing();
     static void setForcePreciseGCForTesting();
 
     static void prepareForGC();

diff --git a/Source/platform/heap/HeapTest.cpp b/Source/platform/heap/HeapTest.cpp
@@ -4654,6 +4654,8 @@ class CrossThreadPointerTester {
             // shutting it down.
             stackPtrValue = reinterpret_cast<uintptr_t>(cto.get());
         }
+        RELEASE_ASSERT(stackPtrValue);
+
         // At this point it is "programatically" okay to shut down the worker thread
         // since the cto object should be dead. However out stackPtrValue will cause a
         // trace of the object when doing a conservative GC.
@@ -4677,13 +4679,6 @@ class CrossThreadPointerTester {
         EXPECT_EQ(0, CrossThreadObject::s_destructorCalls);
         EXPECT_EQ(1, IntWrapper::s_destructorCalls);
 
-        // This release assert is here to ensure the stackValuePtr is not
-        // optimized away before doing the above conservative GC. If the
-        // EXPECT_EQ(0, CrossThreadObject::s_destructorCalls) call above
-        // starts failing it means we have to find a better way to ensure
-        // the stackPtrValue is not optimized away.
-        RELEASE_ASSERT(stackPtrValue);
-
         // Do a GC with no pointers on the stack to see the cto being collected.
         Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
         EXPECT_EQ(1, CrossThreadObject::s_destructorCalls);

diff --git a/Source/platform/heap/ThreadState.cpp b/Source/platform/heap/ThreadState.cpp
@@ -447,7 +447,7 @@ void ThreadState::detach()
     shutdownHeapIfNecessary();
 }
 
-void ThreadState::visitPersistentRoots(Visitor* visitor)
+void ThreadState::visitRoots(Visitor* visitor)
 {
     {
         // All threads are at safepoints so this is not strictly necessary.
@@ -459,14 +459,17 @@ void ThreadState::visitPersistentRoots(Visitor* visitor)
 
     AttachedThreadStateSet& threads = attachedThreads();
     for (AttachedThreadStateSet::iterator it = threads.begin(), end = threads.end(); it != end; ++it)
-        (*it)->visitPersistents(visitor);
+        (*it)->trace(visitor);
 }
 
-void ThreadState::visitStackRoots(Visitor* visitor)
+void ThreadState::visitLocalRoots(Visitor* visitor)
 {
-    AttachedThreadStateSet& threads = attachedThreads();
-    for (AttachedThreadStateSet::iterator it = threads.begin(), end = threads.end(); it != end; ++it)
-        (*it)->visitStack(visitor);
+    // We assume that orphaned pages have no objects reachable from persistent
+    // handles on other threads or CrossThreadPersistents. The only cases where
+    // this could happen is if a global conservative GC finds a "pointer" on
+    // the stack or due to a programming error where an object has a dangling
+    // cross-thread pointer to an object on this heap.
+    m_persistents->trace(visitor);
 }
 
 NO_SANITIZE_ADDRESS
@@ -500,9 +503,6 @@ void ThreadState::visitAsanFakeStackForPointer(Visitor* visitor, Address ptr)
 NO_SANITIZE_ADDRESS
 void ThreadState::visitStack(Visitor* visitor)
 {
-    if (m_stackState == NoHeapPointersOnStack)
-        return;
-
     Address* start = reinterpret_cast<Address*>(m_startOfStack);
     // If there is a safepoint scope marker we should stop the stack
     // scanning there to not touch active parts of the stack. Anything
@@ -547,6 +547,13 @@ void ThreadState::visitPersistents(Visitor* visitor)
     m_persistents->trace(visitor);
 }
 
+void ThreadState::trace(Visitor* visitor)
+{
+    if (m_stackState == HeapPointersOnStack)
+        visitStack(visitor);
+    visitPersistents(visitor);
+}
+
 bool ThreadState::checkAndMarkPointer(Visitor* visitor, Address address)
 {
     // If thread is terminating ignore conservative pointers.

diff --git a/Source/platform/heap/ThreadState.h b/Source/platform/heap/ThreadState.h
@@ -242,11 +242,8 @@ class PLATFORM_EXPORT ThreadState {
     static void attachMainThread();
     static void detachMainThread();
 
-    // Trace all persistent roots, called when marking the managed heap objects.
-    static void visitPersistentRoots(Visitor*);
-
-    // Trace all objects found on the stack, used when doing conservative GCs.
-    static void visitStackRoots(Visitor*);
+    // Trace all GC roots, called when marking the managed heap objects.
+    static void visitRoots(Visitor*);
 
     // Associate ThreadState object with the current thread. After this
     // call thread can start using the garbage collected heap infrastructure.
@@ -512,6 +509,7 @@ class PLATFORM_EXPORT ThreadState {
     HeapStats& statsAfterLastGC() { return m_statsAfterLastGC; }
 
     void setupHeapsForTermination();
+    void visitLocalRoots(Visitor*);
 
 private:
     explicit ThreadState();
@@ -564,6 +562,8 @@ class PLATFORM_EXPORT ThreadState {
     // and lazily construct ThreadState in it using placement new.
     static uint8_t s_mainThreadStateStorage[];
 
+    void trace(Visitor*);
+
     ThreadIdentifier m_thread;
     OwnPtr<PersistentNode> m_persistents;
     StackState m_stackState;