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:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@247426 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index f845641..5401804 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/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-07-12 Keith Miller <keith_miller@apple.com>
Add API to get all the dependencies of a given JSScript
diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp
index 7acabce..a01b5a2 100644
--- a/Source/JavaScriptCore/heap/Heap.cpp
+++ b/Source/JavaScriptCore/heap/Heap.cpp
@@ -254,8 +254,11 @@
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;
}
@@ -270,6 +273,11 @@
Thread::registerGCThread(GCThreadType::Main);
}
+ void threadIsStopping(const AbstractLocker&) override
+ {
+ m_heap.m_collectorThreadIsRunning = false;
+ }
+
private:
Heap& m_heap;
};
@@ -1240,9 +1248,6 @@
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);
@@ -2121,7 +2126,7 @@
// 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);
diff --git a/Source/JavaScriptCore/heap/Heap.h b/Source/JavaScriptCore/heap/Heap.h
index 181ce8f..94f1a8b 100644
--- a/Source/JavaScriptCore/heap/Heap.h
+++ b/Source/JavaScriptCore/heap/Heap.h
@@ -714,6 +714,7 @@
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 };
