[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 84 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/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.
     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_padding(0)
+{
+    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());
 
-    // Verify that all heaps are empty now.
-    for (int i = 0; i < NumberOfHeaps; i++)
-        m_heaps[i]->assertEmpty();
+        // 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();
+        }
+
+        // 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();
 
     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());

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

This should be SafePointAwareMutexLocker. Then we can remove the line 440 and
441 above.

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

Done.

         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();
 }
 
 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.
+    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<false>(&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 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);
 #ifdef NDEBUG
     if (cachedPage)
         return cachedPage;
 #endif
 
     for (int i = 0; i < NumberOfHeaps; i++) {
         BaseHeapPage* page = m_heaps[i]->heapPageFromAddress(address);
@@ skipping 206 matching lines @@
                 threadAttachMutex().unlock();
             return gcInfo;
         }
     }
     if (needLockForIteration)
         threadAttachMutex().unlock();
     return 0;
 }
 #endif
 }