[oilpan]: Make thread shutdown more robust.

Source/platform/heap/ThreadState.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 85 matching lines @@
 uint8_t ThreadState::s_mainThreadStateStorage[sizeof(ThreadState)];
 SafePointBarrier* ThreadState::s_safePointBarrier = 0;
 bool ThreadState::s_inGC = false;
 
 static Mutex& threadAttachMutex()
 {
     AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
     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.
     return 0.100;
 }
 
 
 typedef void (*PushAllRegistersCallback)(SafePointBarrier*, ThreadState*, intptr_t*);
 extern "C" void pushAllRegisters(SafePointBarrier*, ThreadState*, PushAllRegistersCallback);
 
@@ skipping 138 matching lines @@
         barrier->doEnterSafePoint(state, stackEnd);
     }
 
     volatile int m_canResume;
     volatile int m_unparkedThreadCount;
     Mutex m_mutex;
     ThreadCondition m_parked;
     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()))
     , m_startOfStack(reinterpret_cast<intptr_t*>(getStackStart()))
     , m_endOfStack(reinterpret_cast<intptr_t*>(getStackStart()))
     , m_safePointScopeMarker(0)
     , m_atSafePoint(false)
     , m_interruptors()
     , m_gcRequested(false)
     , m_forcePreciseGCForTesting(false)
     , m_sweepRequested(0)
     , m_sweepInProgress(false)
     , m_noAllocationCount(0)
     , m_inGC(false)
     , m_heapContainsCache(adoptPtr(new HeapContainsCache()))
     , m_isCleaningUp(false)
 #if defined(ADDRESS_SANITIZER)
     , m_asanFakeStack(__asan_get_current_fake_stack())
 #endif
 {
     ASSERT(!**s_threadSpecific);
     **s_threadSpecific = this;
 
     m_stats.clear();
     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);
 }
 
 ThreadState::~ThreadState()
 {
     checkThread();
     CallbackStack::shutdown(&m_weakCallbackStack);
     for (int i = GeneralHeap; i < NumberOfHeaps; i++)
         delete m_heaps[i];
@@ skipping 65 matching lines @@
 
 void ThreadState::cleanup()
 {
     // From here on ignore all conservatively discovered
     // pointers into the heap owned by this thread.
     m_isCleaningUp = true;
 
     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);
 
-    // Verify that all heaps are empty now.
-    for (int i = 0; i < NumberOfHeaps; i++)
-        m_heaps[i]->assertEmpty();
+        // 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.

+
+        // 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();
 
     m_cleanupTasks.clear();
 }
 
 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();
 }
 
 void ThreadState::visitRoots(Visitor* visitor)
 {
     {
         // All threads are at safepoints so this is not strictly necessary.
         // However we acquire the mutex to make mutation and traversal of this
         // list symmetrical.
         MutexLocker locker(globalRootsMutex());
         globalRoots()->trace(visitor);
     }
 
     AttachedThreadStateSet& threads = attachedThreads();
     for (AttachedThreadStateSet::iterator it = threads.begin(), end = threads.end(); it != end; ++it)
         (*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)
 {
 #if defined(ADDRESS_SANITIZER)
     Address* start = reinterpret_cast<Address*>(m_startOfStack);
     Address* end = reinterpret_cast<Address*>(m_endOfStack);
     Address* fakeFrameStart = 0;
     Address* fakeFrameEnd = 0;
     Address* maybeFakeFrame = reinterpret_cast<Address*>(ptr);
     Address* realFrameForFakeFrame =
@@ skipping 102 matching lines @@
 #endif
 
 void ThreadState::pushWeakObjectPointerCallback(void* object, WeakPointerCallback callback)
 {
     CallbackStack::Item* slot = m_weakCallbackStack->allocateEntry(&m_weakCallbackStack);
     *slot = CallbackStack::Item(object, callback);
 }
 
 bool ThreadState::popAndInvokeWeakPointerCallback(Visitor* visitor)
 {
-    return m_weakCallbackStack->popAndInvokeCallback(&m_weakCallbackStack, visitor);
+    return m_weakCallbackStack->popAndInvokeCallback<GlobalGC>(&m_weakCallbackStack, visitor);
 }
 
 PersistentNode* ThreadState::globalRoots()
 {
     AtomicallyInitializedStatic(PersistentNode*, anchor = new PersistentAnchor);
     return anchor;
 }
 
 Mutex& ThreadState::globalRootsMutex()
 {
@@ skipping 117 matching lines @@
 {
     for (int i = 0; i < NumberOfHeaps; i++)
         m_heaps[i]->makeConsistentForGC();
 }
 
 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);
 #ifdef NDEBUG
     if (cachedPage)
         return cachedPage;
 #endif
 
     for (int i = 0; i < NumberOfHeaps; i++) {
         BaseHeapPage* page = m_heaps[i]->heapPageFromAddress(address);
@@ skipping 208 matching lines @@
                 threadAttachMutex().unlock();
             return gcInfo;
         }
     }
     if (needLockForIteration)
         threadAttachMutex().unlock();
     return 0;
 }
 #endif
 }