[oilpan]: Make thread shutdown more robust.

Source/platform/heap/ThreadState.cpp

Index: Source/platform/heap/ThreadState.cpp
diff --git a/Source/platform/heap/ThreadState.cpp b/Source/platform/heap/ThreadState.cpp
index 4464337e0a7ab72205a1541375f3a0e3c127b320..d91991aa9120b41b2e5052ef61a30ca9cefe6148 100644
--- a/Source/platform/heap/ThreadState.cpp
+++ b/Source/platform/heap/ThreadState.cpp
@@ -103,6 +103,19 @@ static Mutex& threadAttachMutex()
     return mutex;
 }
 
+// The threadShutdownMutex is used to synchronize thread shutdown
+// since the thread local GC, as of now, cannot run in parallel
+// with other thread local GCs since it is using the global marking
+// stack. It can also not run in parallel with a global GC, but
+// that is honored by not entering a safepoint while doing the
+// thread local GC, meaning a request for a global GC would time
+// out.

[haraken, 2014/07/09 05:17:48]

Probably it might be a good idea to add NoSafePointScope to ensure that we don't
enter any safe point scope while doing the thread-local GC. We can introduce
NoSafePointScope in a follow-up.

[wibling-chromium, 2014/07/09 10:32:31]

We already have an ASSERT ensuring that we cannot enter a safepoint while
m_sweepInProgress == true, see SafePointBarrier::enterSafepoint. I think that is
the only part of the ThreadLocal GC where we have a risk.

+static Mutex& threadShutdownMutex()
+{
+    AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
+    return mutex;
+}
+
 static double lockingTimeout()
 {
     // Wait time for parking all threads is at most 100 MS.
@@ -261,6 +274,17 @@ private:
     ThreadCondition m_resume;
 };
 
+
+BaseHeapPage::BaseHeapPage(PageMemory* storage, const GCInfo* gcInfo, ThreadState* state)
+    : m_storage(storage)
+    , m_gcInfo(gcInfo)
+    , m_threadState(state)
+    , m_shuttingDown(false)
+    , m_traced(false)
+{
+    ASSERT(isPageHeaderAddress(reinterpret_cast<Address>(this)));
+}
+
 ThreadState::ThreadState()
     : m_thread(currentThread())
     , m_persistents(adoptPtr(new PersistentAnchor()))
@@ -288,9 +312,9 @@ ThreadState::ThreadState()
     m_statsAfterLastGC.clear();
     // First allocate the general heap, second iterate through to
     // allocate the type specific heaps
-    m_heaps[GeneralHeap] = new ThreadHeap<FinalizedHeapObjectHeader>(this);
+    m_heaps[GeneralHeap] = new ThreadHeap<FinalizedHeapObjectHeader>(this, GeneralHeap);
     for (int i = GeneralHeap + 1; i < NumberOfHeaps; i++)
-        m_heaps[i] = new ThreadHeap<HeapObjectHeader>(this);
+        m_heaps[i] = new ThreadHeap<HeapObjectHeader>(this, i);
 
     CallbackStack::init(&m_weakCallbackStack);
 }
@@ -376,14 +400,32 @@ void ThreadState::cleanup()
     for (size_t i = 0; i < m_cleanupTasks.size(); i++)
         m_cleanupTasks[i]->preCleanup();
 
-    // After this GC we expect heap to be empty because
-    // preCleanup tasks should have cleared all persistent
-    // handles that were externally owned.
-    Heap::collectAllGarbage();
+    {
+        // We enter a safepoint while waiting for the thread shutdown mutex.
+        SafePointAwareMutexLocker locker(threadShutdownMutex(), NoHeapPointersOnStack);
+
+        // Set flags on all heap pages of this thread to ensure we don't trace
+        // pages on other threads.
+        setupHeapsForShutdown();
+
+        // Do thread local GC's as long as the count of thread local Persistents
+        // changes and is above zero.
+        PersistentAnchor* anchor = static_cast<PersistentAnchor*>(m_persistents.get());
+        int oldCount = 0;
+        int currentCount = anchor->numberOfPersistents();
+        while (currentCount > 0 && currentCount != oldCount) {
+            Heap::collectGarbageForTerminatingThread(this);
+            oldCount = currentCount;
+            currentCount = anchor->numberOfPersistents();
+        }
+        // We should not have any persistents left when getting to this point,
+        // if we have it is probably a bug so adding a debug ASSERT to catch this.
+        ASSERT(currentCount == 0);

[haraken, 2014/07/09 05:17:48]

ASSERT(!currentCount);

[wibling-chromium, 2014/07/09 10:32:31]

Done.

 
-    // Verify that all heaps are empty now.
-    for (int i = 0; i < NumberOfHeaps; i++)
-        m_heaps[i]->assertEmpty();
+        // Do a final GC to finalize any objects pointed to by persistents
+        // collected in the last round of GC above.

[haraken, 2014/07/09 05:17:48]

Why do we need the final GC? Isn't it already guaranteed that the last round of
the above GCs did not collect any persistent?

[wibling-chromium, 2014/07/09 10:32:31]

We need it since when the count of persistents reach zero we still need to sweep
and finalize the objects the persistents previously pointed to. We also need it
in the case where there are no persistents to ensure we at least sweep and
finalize the heap once.

I will change it to:

int oldCount = -1;
int currentCount = anchor->numberOfPersistents();
while (currentCount != oldCount) {
    Heap::CollectGarbageForTerminatingThread(this);
    oldCount = currentCount;
    currentCount = anchor->numberOfPersistents();
}

which should give the same guarantees without having an extra GC after the loop.

+        Heap::collectGarbageForTerminatingThread(this);
+    }
 
     for (size_t i = 0; i < m_cleanupTasks.size(); i++)
         m_cleanupTasks[i]->postCleanup();
@@ -396,18 +438,18 @@ void ThreadState::detach()
     ThreadState* state = current();
     state->cleanup();
 
-    // Enter a safe point before trying to acquire threadAttachMutex
-    // to avoid dead lock if another thread is preparing for GC, has acquired
-    // threadAttachMutex and waiting for other threads to pause or reach a
-    // safepoint.
-    if (!state->isAtSafePoint())
-        state->enterSafePointWithoutPointers();
-
     {
-        MutexLocker locker(threadAttachMutex());
-        state->leaveSafePoint();
+        // Enter a safe point while trying to acquire threadAttachMutex
+        // to avoid dead lock if another thread is preparing for GC, has acquired
+        // threadAttachMutex and waiting for other threads to pause or reach a
+        // safepoint.
+        SafePointAwareMutexLocker locker(threadAttachMutex(), NoHeapPointersOnStack);
         ASSERT(attachedThreads().contains(state));
         attachedThreads().remove(state);
+        // Deleting the thread state also destroys the thread's heaps at which
+        // point all the thread's pages are added to the orphanedPagePool.

[Mads Ager (chromium), 2014/07/11 05:48:06]

This comment says that the pages are added to the orphanedPagePool at this
point. Do we set the orphaned bit at this point as well? If we do, I think that
is too late. We could get a normal GC at the SafePointAwareMutexLocker just
above. The pages for this thread are now in an inconsistent state for tracing
and therefore we need to make sure that the orphaned bit is set on all the pages
at the end of cleanup and before we can get a normal GC so that normal GC will
not perform tracing in the pages.

+        // Subsequently they are moved to the freePagePool once they are no
+        // longer traced by a global GC.
         delete state;
     }
     shutdownHeapIfNecessary();
@@ -428,6 +470,14 @@ void ThreadState::visitRoots(Visitor* visitor)
         (*it)->trace(visitor);
 }
 
+void ThreadState::visitLocalRoots(Visitor* visitor)
+{
+    // We assume there are no CrossThreadPersistents to this threads heap. If
+    // there is, it will be handled in the same way as any other cross-thread
+    // pointer pointing to an orphaned page.

[haraken, 2014/07/09 05:17:48]

This comment is a bit misleading. We can read it as if cross-thread pointers
pointing to orphaned pages are allowed. It's not allowed, as far as I
understand.

// We assume that orphaned pages have no objects reachable from persistent
handles on other threads or CrossThreadPersistents.

?

[wibling-chromium, 2014/07/09 10:32:31]

Done. Rephrased it a bit.

+    m_persistents->trace(visitor);
+}
+
 NO_SANITIZE_ADDRESS
 void ThreadState::visitAsanFakeStackForPointer(Visitor* visitor, Address ptr)
 {
@@ -550,7 +600,7 @@ void ThreadState::pushWeakObjectPointerCallback(void* object, WeakPointerCallbac
 
 bool ThreadState::popAndInvokeWeakPointerCallback(Visitor* visitor)
 {
-    return m_weakCallbackStack->popAndInvokeCallback(&m_weakCallbackStack, visitor);
+    return m_weakCallbackStack->popAndInvokeCallback<GlobalGC>(&m_weakCallbackStack, visitor);
 }
 
 PersistentNode* ThreadState::globalRoots()
@@ -688,15 +738,26 @@ void ThreadState::prepareForGC()
     for (int i = 0; i < NumberOfHeaps; i++) {
         BaseHeap* heap = m_heaps[i];
         heap->makeConsistentForGC();
-        // If there are parked threads with outstanding sweep requests, clear their mark bits.
-        // This happens if a thread did not have time to wake up and sweep,
-        // before the next GC arrived.
+        // If a new GC is requested before this thread got around to sweep, ie. due to the
+        // thread doing a long running operation, we clear the mark bits and mark any of
+        // the dead objects as dead. The latter is used to ensure the next GC marking does
+        // not trace already dead objects. If we trace a dead object we could end up tracing
+        // into garbage or the middle of another object via the newly conservatively found
+        // object.
         if (sweepRequested())
-            heap->clearMarks();
+            heap->clearLiveAndMarkDead();
     }
     setSweepRequested();
 }
 
+void ThreadState::setupHeapsForShutdown()
+{
+    for (int i = 0; i < NumberOfHeaps; i++) {
+        BaseHeap* heap = m_heaps[i];
+        heap->prepareHeapForShutdown();
+    }
+}
+
 BaseHeapPage* ThreadState::heapPageFromAddress(Address address)
 {
     BaseHeapPage* cachedPage = heapContainsCache()->lookup(address);