[oilpan]: Make parking threads for GC timeout in the case parking exceeds 100 MS.

This is to avoid issues where a GC is hanging forever waiting for a thread to reach a safepoint. This should not happen, but in the case it does we would rather abandon the GC and attempt it again later.

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=

Review URL: https://codereview.chromium.org/260723003

git-svn-id: svn://svn.chromium.org/blink/trunk@173646 bbb929c8-8fbe-4397-9dbb-9b2b20218538

third_party/WebKit/Source/platform/heap/Heap.cpp

@@ -301,6 +301,7 @@ public:
     explicit GCScope(ThreadState::StackState stackState)
         : m_state(ThreadState::current())
         , m_safePointScope(stackState)
+        , m_parkedAllThreads(false)
     {
         TRACE_EVENT0("Blink", "Heap::GCScope");
         const char* samplingState = TRACE_EVENT_GET_SAMPLING_STATE();
@@ -314,23 +315,31 @@ public:
         // a row.
         RELEASE_ASSERT(!m_state->isInGC());
         RELEASE_ASSERT(!m_state->isSweepInProgress());
-        ThreadState::stopThreads();
-        m_state->enterGC();
-
+        if (LIKELY(ThreadState::stopThreads())) {
+            m_parkedAllThreads = true;
+            m_state->enterGC();
+        }
         if (m_state->isMainThread())
             TRACE_EVENT_SET_NONCONST_SAMPLING_STATE(samplingState);
     }
 
+    bool allThreadsParked() { return m_parkedAllThreads; }
+
     ~GCScope()
     {
-        m_state->leaveGC();
-        ASSERT(!m_state->isInGC());
-        ThreadState::resumeThreads();
+        // Only cleanup if we parked all threads in which case the GC happened
+        // and we need to resume the other threads.
+        if (LIKELY(m_parkedAllThreads)) {
+            m_state->leaveGC();
+            ASSERT(!m_state->isInGC());
+            ThreadState::resumeThreads();
+        }
     }
 
 private:
     ThreadState* m_state;
     ThreadState::SafePointScope m_safePointScope;
+    bool m_parkedAllThreads; // False if we fail to park all threads
 };
 
 NO_SANITIZE_ADDRESS
@@ -1600,10 +1609,13 @@ void Heap::collectGarbage(ThreadState::StackState stackState)
     state->clearGCRequested();
 
     GCScope gcScope(stackState);
-
+    // Check if we successfully parked the other threads. If not we bail out of the GC.
+    if (!gcScope.allThreadsParked()) {
+        ThreadState::current()->setGCRequested();
+        return;
+    }
     TRACE_EVENT0("Blink", "Heap::collectGarbage");
     TRACE_EVENT_SCOPED_SAMPLING_STATE("Blink", "BlinkGC");
-
 #if ENABLE(GC_TRACING)
     static_cast<MarkingVisitor*>(s_markingVisitor)->objectGraph().clear();
 #endif

third_party/WebKit/Source/platform/heap/HeapTest.cpp

@@ -36,6 +36,7 @@
 #include "platform/heap/HeapTerminatedArrayBuilder.h"
 #include "platform/heap/ThreadState.h"
 #include "platform/heap/Visitor.h"
+#include "public/platform/Platform.h"
 #include "wtf/HashTraits.h"
 #include "wtf/LinkedHashSet.h"
 
@@ -99,28 +100,39 @@ public:
     explicit TestGCScope(ThreadState::StackState state)
         : m_state(ThreadState::current())
         , m_safePointScope(state)
+        , m_parkedAllThreads(false)
     {
         m_state->checkThread();
         EXPECT_FALSE(m_state->isInGC());
-        ThreadState::stopThreads();
-        m_state->enterGC();
+        if (LIKELY(ThreadState::stopThreads())) {
+            m_state->enterGC();
+            m_parkedAllThreads = true;
+        }
     }
 
+    bool allThreadsParked() { return m_parkedAllThreads; }
+
     ~TestGCScope()
     {
-        m_state->leaveGC();
-        EXPECT_FALSE(m_state->isInGC());
-        ThreadState::resumeThreads();
+        // Only cleanup if we parked all threads in which case the GC happened
+        // and we need to resume the other threads.
+        if (LIKELY(m_parkedAllThreads)) {
+            m_state->leaveGC();
+            EXPECT_FALSE(m_state->isInGC());
+            ThreadState::resumeThreads();
+        }
     }
 
 private:
     ThreadState* m_state;
     ThreadState::SafePointScope m_safePointScope;
+    bool m_parkedAllThreads; // False if we fail to park all threads
 };
 
 static void getHeapStats(HeapStats* stats)
 {
     TestGCScope scope(ThreadState::NoHeapPointersOnStack);
+    EXPECT_TRUE(scope.allThreadsParked());
     Heap::getStats(stats);
 }
 
@@ -350,7 +362,7 @@ protected:
         for (int i = 0; i < numberOfThreads; i++)
             createThread(&threadFunc, tester, "testing thread");
         while (tester->m_threadsToFinish) {
-            ThreadState::current()->safePoint(ThreadState::NoHeapPointersOnStack);
+            ThreadState::SafePointScope scope(ThreadState::NoHeapPointersOnStack);
             yield();
         }
         delete tester;
@@ -408,8 +420,8 @@ protected:
                     wrapper = IntWrapper::create(0x0bbac0de);
                     if (!(i % 10)) {
                         globalPersistent = adoptPtr(new GlobalIntWrapperPersistent(IntWrapper::create(0x0ed0cabb)));
-                        ThreadState::current()->safePoint(ThreadState::NoHeapPointersOnStack);
                     }
+                    ThreadState::SafePointScope scope(ThreadState::NoHeapPointersOnStack);
                     yield();
                 }
 
@@ -423,6 +435,7 @@ protected:
                 EXPECT_EQ(wrapper->value(), 0x0bbac0de);
                 EXPECT_EQ((*globalPersistent)->value(), 0x0ed0cabb);
             }
+            ThreadState::SafePointScope scope(ThreadState::NoHeapPointersOnStack);
             yield();
         }
         ThreadState::detach();
@@ -452,9 +465,7 @@ private:
                 for (int i = 0; i < numberOfAllocations; i++) {
                     weakMap->add(static_cast<unsigned>(i), IntWrapper::create(0));
                     weakMap2.add(static_cast<unsigned>(i), IntWrapper::create(0));
-                    if (!(i % 10)) {
-                        ThreadState::current()->safePoint(ThreadState::NoHeapPointersOnStack);
-                    }
+                    ThreadState::SafePointScope scope(ThreadState::NoHeapPointersOnStack);
                     yield();
                 }
 
@@ -468,6 +479,7 @@ private:
                 EXPECT_TRUE(weakMap->isEmpty());
                 EXPECT_TRUE(weakMap2.isEmpty());
             }
+            ThreadState::SafePointScope scope(ThreadState::NoHeapPointersOnStack);
             yield();
         }
         ThreadState::detach();
@@ -3273,6 +3285,7 @@ TEST(HeapTest, CheckAndMarkPointer)
     // checkAndMarkPointer tests.
     {
         TestGCScope scope(ThreadState::HeapPointersOnStack);
+        EXPECT_TRUE(scope.allThreadsParked()); // Fail the test if we could not park all threads.
         Heap::makeConsistentForGC();
         for (size_t i = 0; i < objectAddresses.size(); i++) {
             EXPECT_TRUE(Heap::checkAndMarkPointer(&visitor, objectAddresses[i]));
@@ -3291,6 +3304,7 @@ TEST(HeapTest, CheckAndMarkPointer)
     clearOutOldGarbage(&initialHeapStats);
     {
         TestGCScope scope(ThreadState::HeapPointersOnStack);
+        EXPECT_TRUE(scope.allThreadsParked());
         Heap::makeConsistentForGC();
         for (size_t i = 0; i < objectAddresses.size(); i++) {
             EXPECT_FALSE(Heap::checkAndMarkPointer(&visitor, objectAddresses[i]));
@@ -3880,4 +3894,64 @@ TEST(HeapTest, MultipleMixins)
     EXPECT_EQ(3, IntWrapper::s_destructorCalls);
 }
 
+class GCParkingThreadTester {
+public:
+    static void test()
+    {
+        createThread(&sleeperMainFunc, 0, "SleepingThread");
+
+        // Wait for the sleeper to run.
+        while (!s_sleeperRunning) {
+            yield();
+        }
+
+        {
+            // Expect the first attempt to park the sleeping thread to fail
+            TestGCScope scope(ThreadState::NoHeapPointersOnStack);
+            EXPECT_FALSE(scope.allThreadsParked());
+        }
+
+        s_sleeperDone = true;
+
+        // Wait for the sleeper to finish.
+        while (s_sleeperRunning) {
+            // We enter the safepoint here since the sleeper thread will detach
+            // causing it to GC.
+            ThreadState::current()->safePoint(ThreadState::NoHeapPointersOnStack);
+            yield();
+        }
+        {
+            // Since the sleeper thread has detached this is the only thread.
+            TestGCScope scope(ThreadState::NoHeapPointersOnStack);
+            EXPECT_TRUE(scope.allThreadsParked());
+        }
+    }
+
+private:
+    static void sleeperMainFunc(void* data)
+    {
+        ThreadState::attach();
+        s_sleeperRunning = true;
+
+        // Simulate a long running op that is not entering a safepoint.
+        while (!s_sleeperDone) {
+            yield();
+        }
+
+        ThreadState::detach();
+        s_sleeperRunning = false;
+    }
+
+    static volatile bool s_sleeperRunning;
+    static volatile bool s_sleeperDone;
+};
+
+volatile bool GCParkingThreadTester::s_sleeperRunning = false;
+volatile bool GCParkingThreadTester::s_sleeperDone = false;
+
+TEST(HeapTest, GCParkingTimeout)
+{
+    GCParkingThreadTester::test();
+}
+
 } // WebCore namespace

third_party/WebKit/Source/platform/heap/RunAllTests.cpp

@@ -36,11 +36,12 @@
 #include "wtf/MainThread.h"
 #include "wtf/WTF.h"
 #include <base/test/test_suite.h>
+#include <base/time/time.h>
 #include <string.h>
 
 static double CurrentTime()
 {
-    return 0.0;
+    return base::Time::Now().ToDoubleT();
 }
 
 static void AlwaysZeroNumberSource(unsigned char* buf, size_t len)

third_party/WebKit/Source/platform/heap/ThreadState.cpp

@@ -97,6 +97,13 @@ static Mutex& threadAttachMutex()
     return mutex;
 }
 
+static double lockingTimeout()
+{
+    // Wait time for parking all threads is at most 500 MS.
+    return 0.100;
+}
+
+
 typedef void (*PushAllRegistersCallback)(SafePointBarrier*, ThreadState*, intptr_t*);
 extern "C" void pushAllRegisters(SafePointBarrier*, ThreadState*, PushAllRegistersCallback);
 
@@ -106,7 +113,7 @@ public:
     ~SafePointBarrier() { }
 
     // Request other attached threads that are not at safe points to park themselves on safepoints.
-    void parkOthers()
+    bool parkOthers()
     {
         ASSERT(ThreadState::current()->isAtSafePoint());
 
@@ -129,16 +136,30 @@ public:
                 interruptors[i]->requestInterrupt();
         }
 
-        while (acquireLoad(&m_unparkedThreadCount) > 0)
-            m_parked.wait(m_mutex);
+        while (acquireLoad(&m_unparkedThreadCount) > 0) {
+            double expirationTime = currentTime() + lockingTimeout();
+            if (!m_parked.timedWait(m_mutex, expirationTime)) {
+                // One of the other threads did not return to a safepoint within the maximum
+                // time we allow for threads to be parked. Abandon the GC and resume the
+                // currently parked threads.
+                resumeOthers(true);
+                return false;
+            }
+        }
+        return true;
     }
 
-    void resumeOthers()
+    void resumeOthers(bool barrierLocked = false)
     {
         ThreadState::AttachedThreadStateSet& threads = ThreadState::attachedThreads();
         atomicSubtract(&m_unparkedThreadCount, threads.size());
         releaseStore(&m_canResume, 1);
-        {
+
+        // FIXME: Resumed threads will all contend for m_mutex just to unlock it
+        // later which is a waste of resources.
+        if (UNLIKELY(barrierLocked)) {
+            m_resume.broadcast();
+        } else {
             // FIXME: Resumed threads will all contend for
             // m_mutex just to unlock it later which is a waste of
             // resources.
@@ -160,6 +181,28 @@ public:
         ASSERT(ThreadState::current()->isAtSafePoint());
     }
 
+    void checkAndPark(ThreadState* state)
+    {
+        ASSERT(!state->isSweepInProgress());
+        if (!acquireLoad(&m_canResume)) {
+            pushAllRegisters(this, state, parkAfterPushRegisters);
+            state->performPendingSweep();
+        }
+    }
+
+    void enterSafePoint(ThreadState* state)
+    {
+        ASSERT(!state->isSweepInProgress());
+        pushAllRegisters(this, state, enterSafePointAfterPushRegisters);
+    }
+
+    void leaveSafePoint(ThreadState* state)
+    {
+        if (atomicIncrement(&m_unparkedThreadCount) > 0)
+            checkAndPark(state);
+    }
+
+private:
     void doPark(ThreadState* state, intptr_t* stackEnd)
     {
         state->recordStackEnd(stackEnd);
@@ -171,13 +214,9 @@ public:
         atomicIncrement(&m_unparkedThreadCount);
     }
 
-    void checkAndPark(ThreadState* state)
+    static void parkAfterPushRegisters(SafePointBarrier* barrier, ThreadState* state, intptr_t* stackEnd)
     {
-        ASSERT(!state->isSweepInProgress());
-        if (!acquireLoad(&m_canResume)) {
-            pushAllRegisters(this, state, parkAfterPushRegisters);
-            state->performPendingSweep();
-        }
+        barrier->doPark(state, stackEnd);
     }
 
     void doEnterSafePoint(ThreadState* state, intptr_t* stackEnd)
@@ -198,24 +237,6 @@ public:
         }
     }
 
-    void enterSafePoint(ThreadState* state)
-    {
-        ASSERT(!state->isSweepInProgress());
-        pushAllRegisters(this, state, enterSafePointAfterPushRegisters);
-    }
-
-    void leaveSafePoint(ThreadState* state)
-    {
-        if (atomicIncrement(&m_unparkedThreadCount) > 0)
-            checkAndPark(state);
-    }
-
-private:
-    static void parkAfterPushRegisters(SafePointBarrier* barrier, ThreadState* state, intptr_t* stackEnd)
-    {
-        barrier->doPark(state, stackEnd);
-    }
-
     static void enterSafePointAfterPushRegisters(SafePointBarrier* barrier, ThreadState* state, intptr_t* stackEnd)
     {
         barrier->doEnterSafePoint(state, stackEnd);
@@ -702,9 +723,9 @@ void ThreadState::getStats(HeapStats& stats)
 #endif
 }
 
-void ThreadState::stopThreads()
+bool ThreadState::stopThreads()
 {
-    s_safePointBarrier->parkOthers();
+    return s_safePointBarrier->parkOthers();
 }
 
 void ThreadState::resumeThreads()

third_party/WebKit/Source/platform/heap/ThreadState.h

@@ -359,7 +359,7 @@ public:
     //
 
     // Request all other threads to stop. Must only be called if the current thread is at safepoint.
-    static void stopThreads();
+    static bool stopThreads();
     static void resumeThreads();
 
     // Check if GC is requested by another thread and pause this thread if this is the case.