[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 b15a2362232652299e2b378a40b89fa7bff432c5..352344bf308792847501064de36b79ee72449cce 100644
--- a/Source/platform/heap/ThreadState.cpp
+++ b/Source/platform/heap/ThreadState.cpp
@@ -102,6 +102,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/07 16:13:47]

I'm not sure if this assumption is correct. Heap::collectGarbageForThread() at
line 415 can call performPendingSweep() on the thread heap. The sweep can call
any destructors, and the destructor can enter a safe point. Then the thread can
get involved in a global GC while doing the thread-local GC.

Can we make the logic of the mutual exclusion more explicit?

What we have to guarantee is that no two (thread-local or global) GCs must run
at the same time. Can we prepare one mutex shared among thread-local GCs and
global GCs in order to guarantee the fact more straightforwardly?

[wibling-chromium, 2014/07/08 13:39:47]

It should not be the case that we enter a safepoint while sweeping. If we do so
we will have hit the issues discussed in the SafePointAwareMutexLocker change,
https://codereview.chromium.org/367633002. Basically entering a safepoint while
sweeping is not allowed. 
There is also an ASSERT in SafePointBarriers::enterSafePoint which will fire if
we try to enter a safepoint while sweeping. Regarding allocations during sweep,
Mads changed the shouldGC/shouldForceConservativeGC methods to return false if
sweeping to ensure we don't try to GC while sweeping.

+static Mutex& threadShutdownMutex()
+{
+    AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
+    return mutex;
+}
+
 static double lockingTimeout()
 {
     // Wait time for parking all threads is at most 100 MS.
@@ -260,6 +273,16 @@ private:
     ThreadCondition m_resume;
 };
 
+
+BaseHeapPage::BaseHeapPage(PageMemory* storage, const GCInfo* gcInfo, ThreadState* state)
+    : m_storage(storage)
+    , m_gcInfo(gcInfo)
+    , m_threadState(state)
+    , m_padding(0)
+{
+    ASSERT(isPageHeaderAddress(reinterpret_cast<Address>(this)));
+}
+
 ThreadState::ThreadState()
     : m_thread(currentThread())
     , m_persistents(adoptPtr(new PersistentAnchor()))
@@ -287,9 +310,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);
 }
@@ -375,14 +398,29 @@ 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());
+
+        // Set a flag on all this thread's heap pages to ensure we don't trace

[haraken, 2014/07/07 16:13:47]

Set flags on all heap pages of this thread to ensure we don't trace outside of
the heap pages.

[wibling-chromium, 2014/07/08 13:39:47]

Done.

+        // outside this thread's heap pages.
+        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 curCount = anchor->numberOfPersistents();

[haraken, 2014/07/07 16:13:46]

curCount => currentCount

[wibling-chromium, 2014/07/08 13:39:47]

Done.

+        while (curCount > 0 && curCount != oldCount) {

[haraken, 2014/07/07 16:13:47]

I'm not sure if 'curCount == oldCount' is enough to check that we've finished
sweeping pages of the thread heap.

Destructing objects can decrease # of persistents, but it's possible that
destructors can allocate new persistents. Then 'curCount == oldCount' won't mean
that we've finished sweeping pages of the thread heap.

However, this would be a too artificial case, so it wouldn't be worth adding
special logic for this. I think we can just add a RELEASE_ASSERT to verify that
no new persistents are allocated during the thread-local GC. If we hit the
RELEASE_ASSERT, we should restructure the code.

[wibling-chromium, 2014/07/08 13:39:48]

I have added an ASSERT to PersistentBase to check that we are not in the while
doing thread shutdown. I didn't make it a RELEASE_ASSERT since it seems like a
rather large overhead to pay on each Persistent allocation given that I have to
get to the ThreadState.

+            Heap::collectGarbageForThread(this, false);
+            oldCount = curCount;
+            curCount = anchor->numberOfPersistents();
+        }
 
-    // Verify that all heaps are empty now.
-    for (int i = 0; i < NumberOfHeaps; i++)
-        m_heaps[i]->assertEmpty();
+        // Do a final sweep to finalize any objects pointed to by persistents
+        // collected in the last round of GC above.
+        Heap::collectGarbageForThread(this, true);

[Mads Ager (chromium), 2014/07/08 08:24:56]

Add enum instead of bool?

[wibling-chromium, 2014/07/08 13:39:47]

Done.

+    }
 
     for (size_t i = 0; i < m_cleanupTasks.size(); i++)
         m_cleanupTasks[i]->postCleanup();
@@ -407,6 +445,10 @@ void ThreadState::detach()
         state->leaveSafePoint();
         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.
+        // Subsequently they are moved to the memoryPool once they are no
+        // longer traced by a global GC.
         delete state;
     }
     shutdownHeapIfNecessary();
@@ -427,6 +469,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.
+    m_persistents->trace(visitor);
+}
+
 NO_SANITIZE_ADDRESS
 void ThreadState::visitAsanFakeStackForPointer(Visitor* visitor, Address ptr)
 {
@@ -549,7 +599,7 @@ void ThreadState::pushWeakObjectPointerCallback(void* object, WeakPointerCallbac
 
 bool ThreadState::popAndInvokeWeakPointerCallback(Visitor* visitor)
 {
-    return m_weakCallbackStack->popAndInvokeCallback(&m_weakCallbackStack, visitor);
+    return m_weakCallbackStack->popAndInvokeCallback<false>(&m_weakCallbackStack, visitor);
 }
 
 PersistentNode* ThreadState::globalRoots()
@@ -687,15 +737,27 @@ 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 there are parked threads with outstanding sweep requests, clear their mark bits,

[haraken, 2014/07/07 16:13:47]

If a next GC is requested before processing a sweeping request (this can happen
if the thread did not have time to wake up and sweep before the next GC
arrives), clear all the mark bits and ...

[wibling-chromium, 2014/07/08 13:39:47]

Rephrased it a bit.

+        // and mark any of their dead objects as dead. The latter is used to ensure the next
+        // GC marking does not revive already dead objects. If we revived a dead object we

[Mads Ager (chromium), 2014/07/08 08:24:56]

revive -> trace

revived -> traced

[wibling-chromium, 2014/07/08 13:39:48]

Done.

+        // could end up tracing into garbage or the middle of another object via the revived
+        // object.

[haraken, 2014/07/07 16:13:46]

Just help me understand: Is this "revive" problem a new problem that arises when
we introduce the orphaned pages? Or have we already had the problem when a
thread fails to sweep the page before the next GC arrives?

[wibling-chromium, 2014/07/08 13:39:47]

This is an issue we discovered while I was doing this change. Basically if we
have two threads with a cross thread pointer going from one thread heap to the
other thread heap we could end up tracing into garbage or another object if the
"pointing" thread didn't sweep, but the "pointed to" thread did sweep.
The new dead bits should fix this.

+        // The case of a thread missing a sweep happens if it did not have time to wake up
+        // and sweep, before the next GC arrived.
         if (sweepRequested())
-            heap->clearMarks();
+            heap->clearLiveAndMarkDead();
     }
     setSweepRequested();
 }
 
+void ThreadState::setupHeapsForShutdown()
+{
+    for (int i = 0; i < NumberOfHeaps; i++) {
+        BaseHeap* heap = m_heaps[i];
+        heap->setShutdown();

[haraken, 2014/07/07 16:13:46]

setShutdown => setHeapForShutdown

[wibling-chromium, 2014/07/08 13:39:47]

Changed it to prepareHeapForShutdown.

+    }
+}
+
 BaseHeapPage* ThreadState::heapPageFromAddress(Address address)
 {
     BaseHeapPage* cachedPage = heapContainsCache()->lookup(address);