Oilpan: simplify away StoppingOtherThreads pseudo GCState.

Source/platform/heap/Heap.cpp

Index: Source/platform/heap/Heap.cpp
diff --git a/Source/platform/heap/Heap.cpp b/Source/platform/heap/Heap.cpp
index 430e1121cd22fee3d4651aea19f3667277737c7f..7cb6d50bef15522dade2d2563aaf52e62ae13857 100644
--- a/Source/platform/heap/Heap.cpp
+++ b/Source/platform/heap/Heap.cpp
@@ -109,11 +109,30 @@ static String classOf(const void* object)
 }
 #endif
 
+class GCForbiddenScope {
+public:
+    explicit GCForbiddenScope(ThreadState* state)
+        : m_state(state)
+    {
+        // Prevent nested collectGarbage() invocations.
+        m_state->enterGCForbiddenScope();
+    }
+
+    ~GCForbiddenScope()
+    {
+        m_state->leaveGCForbiddenScope();
+    }
+
+private:
+    ThreadState* m_state;
+};
+
 class GCScope {
 public:
-    explicit GCScope(ThreadState::StackState stackState)
-        : m_state(ThreadState::current())
-        , m_safePointScope(stackState)
+    GCScope(ThreadState* state, ThreadState::StackState stackState)
+        : m_state(state)
+        , m_gcForbiddenScope(state)
+        , m_safePointScope(stackState, state)
         , m_parkedAllThreads(false)
     {
         TRACE_EVENT0("blink_gc", "Heap::GCScope");
@@ -126,9 +145,9 @@ public:
         // FIXME: in an unlikely coincidence that two threads decide
         // to collect garbage at the same time, avoid doing two GCs in
         // a row.
-        if (LIKELY(ThreadState::stopThreads())) {
+        if (LIKELY(ThreadState::stopThreads()))
             m_parkedAllThreads = true;
-        }
+
         if (m_state->isMainThread())
             TRACE_EVENT_SET_NONCONST_SAMPLING_STATE(samplingState);
     }
@@ -139,13 +158,13 @@ public:
     {
         // 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)) {
+        if (LIKELY(m_parkedAllThreads))
             ThreadState::resumeThreads();
-        }
     }
 
 private:
     ThreadState* m_state;
+    GCForbiddenScope m_gcForbiddenScope;
     SafePointScope m_safePointScope;

[haraken, 2015/05/28 15:00:56]

Add a comment and mention that the order between GCForbiddenScope and
SafePointScope matters.

I don't have a strong opinion, but it might be clearer to remove
GCForbiddenScope and write code like:

GCScope() {
  m_state->enterGCForbiddenScope();
  m_state->enterSafePointScope();
  ...
}

~GCScope() {
  ...
  m_state->leaveSafePointScope();
  m_state->leaveGCForbiddenScope();
}

[sof, 2015/05/28 15:08:50]

Will add a comment re: ordering.

[sof, 2015/05/29 18:52:18]

Done.

     bool m_parkedAllThreads; // False if we fail to park all threads
 };
@@ -1872,20 +1891,14 @@ const char* Heap::gcReasonString(GCReason reason)
 void Heap::collectGarbage(ThreadState::StackState stackState, ThreadState::GCType gcType, GCReason reason)
 {
     ThreadState* state = ThreadState::current();
-    RELEASE_ASSERT(!state->isInGC());
+    RELEASE_ASSERT(!state->isGCForbidden());

[haraken, 2015/05/28 15:00:55]

I'd move this check to GCForbiddenScope's constructor. At least, after the
following completeSweep().

[sof, 2015/05/28 15:27:57]

I don't understand the motivation behind the suggested change, could you
explain?

I'll explain mine :) Having it at the top of collectGarbage() catches the
problem right at the source/outset -- you shouldn't be entering collectGarbage()
at all while GCs are forbidden.

[haraken, 2015/05/28 15:28:46]

ah, that makes sense!

[sof, 2015/05/29 18:52:18]

Added a comment next to it.

     state->completeSweep();
-    ThreadState::GCState originalGCState = state->gcState();
-    state->setGCState(ThreadState::StoppingOtherThreads);
 
-    GCScope gcScope(stackState);
+    GCScope gcScope(state, stackState);
     // Check if we successfully parked the other threads.  If not we bail out of
     // the GC.
-    if (!gcScope.allThreadsParked()) {
-        // Restore the original GCState.
-        if (LIKELY(state->gcState() == ThreadState::StoppingOtherThreads))
-            state->setGCState(originalGCState);
+    if (!gcScope.allThreadsParked())
         return;
-    }
 
     if (state->isMainThread())
         ScriptForbiddenScope::enter();