Merge r247426 - Concurrent GC should not rely on current phase to det…

…ermine if it's safe to steal conn

https://bugs.webkit.org/show_bug.cgi?id=199786
<rdar://problem/52505197>

Reviewed by Saam Barati.

In r246507, we fixed a race condition in the concurrent GC where the mutator might steal
the conn from the collector thread while it transitions from the End phase to NotRunning.
However, that fix was not sufficient. In the case that the mutator steals the conn, and the
execution interleaves long enough for the mutator to progress to a different collection phase,
the collector will resume in a phase other than NotRunning, and hence the check added to
NotRunning will not suffice. To fix that, we add a new variable to track whether the collector
thread is running (m_collectorThreadIsRunning) and use it to determine whether it's safe to
steal the conn, rather than relying on m_currentPhase.

* heap/Heap.cpp:
(JSC::Heap::runNotRunningPhase):
(JSC::Heap::requestCollection):
* heap/Heap.h:

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,25 @@
+2019-07-15  Tadeu Zagallo  <tzagallo@apple.com>
+
+        Concurrent GC should not rely on current phase to determine if it's safe to steal conn
+        https://bugs.webkit.org/show_bug.cgi?id=199786
+        <rdar://problem/52505197>
+
+        Reviewed by Saam Barati.
+
+        In r246507, we fixed a race condition in the concurrent GC where the mutator might steal
+        the conn from the collector thread while it transitions from the End phase to NotRunning.
+        However, that fix was not sufficient. In the case that the mutator steals the conn, and the
+        execution interleaves long enough for the mutator to progress to a different collection phase,
+        the collector will resume in a phase other than NotRunning, and hence the check added to
+        NotRunning will not suffice. To fix that, we add a new variable to track whether the collector
+        thread is running (m_collectorThreadIsRunning) and use it to determine whether it's safe to
+        steal the conn, rather than relying on m_currentPhase.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::runNotRunningPhase):
+        (JSC::Heap::requestCollection):
+        * heap/Heap.h:
+
 2019-06-17  Tadeu Zagallo  <tzagallo@apple.com>
 
         Concurrent GC should check the conn before starting a new collection cycle

Source/JavaScriptCore/heap/Heap.cpp

@@ -250,8 +250,11 @@ class Heap::Thread : public AutomaticThread {
             m_heap.notifyThreadStopping(locker);
             return PollResult::Stop;
         }
-        if (m_heap.shouldCollectInCollectorThread(locker))
+        if (m_heap.shouldCollectInCollectorThread(locker)) {
+            m_heap.m_collectorThreadIsRunning = true;
             return PollResult::Work;
+        }
+        m_heap.m_collectorThreadIsRunning = false;
         return PollResult::Wait;
     }
 
@@ -266,6 +269,11 @@ class Heap::Thread : public AutomaticThread {
         WTF::registerGCThread(GCThreadType::Main);
     }
 
+    void threadIsStopping(const AbstractLocker&) override
+    {
+        m_heap.m_collectorThreadIsRunning = false;
+    }
+
 private:
     Heap& m_heap;
 };
@@ -1193,9 +1201,6 @@ NEVER_INLINE bool Heap::runNotRunningPhase(GCConductor conn)
         auto locker = holdLock(*m_threadLock);
         if (m_requests.isEmpty())
             return false;
-        // Check if the mutator has stolen the conn while the collector transitioned from End to NotRunning
-        if (conn == GCConductor::Collector && !!(m_worldState.load() & mutatorHasConnBit))
-            return false;
     }
 
     return changePhase(conn, CollectorPhase::Begin);
@@ -2088,7 +2093,7 @@ Heap::Ticket Heap::requestCollection(GCRequest request)
     // right now. This is an optimization that prevents the collector thread from ever starting in most
     // cases.
     ASSERT(m_lastServedTicket <= m_lastGrantedTicket);
-    if ((m_lastServedTicket == m_lastGrantedTicket) && (m_currentPhase == CollectorPhase::NotRunning)) {
+    if ((m_lastServedTicket == m_lastGrantedTicket) && !m_collectorThreadIsRunning) {
         if (false)
             dataLog("Taking the conn.\n");
         m_worldState.exchangeOr(mutatorHasConnBit);

Source/JavaScriptCore/heap/Heap.h

@@ -713,6 +713,7 @@ class Heap {
     CollectorPhase m_lastPhase { CollectorPhase::NotRunning };
     CollectorPhase m_currentPhase { CollectorPhase::NotRunning };
     CollectorPhase m_nextPhase { CollectorPhase::NotRunning };
+    bool m_collectorThreadIsRunning { false };
     bool m_threadShouldStop { false };
     bool m_threadIsStopping { false };
     bool m_mutatorDidRun { true };