Oilpan: simplify away StoppingOtherThreads pseudo GCState.

Source/platform/heap/Heap.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 91 matching lines @@
 
 #if ENABLE(GC_PROFILING)
 static String classOf(const void* object)
 {
     if (const GCInfo* gcInfo = Heap::findGCInfo(reinterpret_cast<Address>(const_cast<void*>(object))))
         return gcInfo->m_className;
     return "unknown";
 }
 #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");
         const char* samplingState = TRACE_EVENT_GET_SAMPLING_STATE();
         if (m_state->isMainThread())
             TRACE_EVENT_SET_SAMPLING_STATE("blink_gc", "BlinkGCWaiting");
 
         m_state->checkThread();
 
         // 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);
     }
 
     bool allThreadsParked() { return m_parkedAllThreads; }
 
     ~GCScope()
     {
         // 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
 };
 
 #if ENABLE(ASSERT)
 NO_SANITIZE_ADDRESS
 void HeapObjectHeader::zapMagic()
 {
     checkHeader();
     m_magic = zappedMagic;
 }
@@ skipping 1705 matching lines @@
         STRINGIFY_REASON(ForcedGC);
 #undef STRINGIFY_REASON
     case NumberOfGCReason: ASSERT_NOT_REACHED();
     }
     return "<Unknown>";
 }
 
 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();
 
     s_lastGCWasConservative = false;
 
     TRACE_EVENT2("blink_gc", "Heap::collectGarbage",
         "lazySweeping", gcType == ThreadState::GCWithoutSweep,
         "gcReason", gcReasonString(reason));
     TRACE_EVENT_SCOPED_SAMPLING_STATE("blink_gc", "BlinkGC");
@@ skipping 325 matching lines @@
 size_t Heap::s_allocatedObjectSize = 0;
 size_t Heap::s_allocatedSpace = 0;
 size_t Heap::s_markedObjectSize = 0;
 // We don't want to use 0 KB for the initial value because it may end up
 // triggering the first GC of some thread too prematurely.
 size_t Heap::s_estimatedLiveObjectSize = 512 * 1024;
 size_t Heap::s_externalObjectSizeAtLastGC = 0;
 double Heap::s_estimatedMarkingTimePerByte = 0.0;
 
 } // namespace blink