Title: [247426] trunk/Source/_javascript_Core
- Revision
- 247426
- Author
- tzaga...@apple.com
- Date
- 2019-07-15 01:47:22 -0700 (Mon, 15 Jul 2019)
Log Message
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:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (247425 => 247426)
--- trunk/Source/_javascript_Core/ChangeLog 2019-07-14 22:18:03 UTC (rev 247425)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-07-15 08:47:22 UTC (rev 247426)
@@ -1,3 +1,25 @@
+2019-07-15 Tadeu Zagallo <tzaga...@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_mil...@apple.com>
Add API to get all the dependencies of a given JSScript
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (247425 => 247426)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2019-07-14 22:18:03 UTC (rev 247425)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2019-07-15 08:47:22 UTC (rev 247426)
@@ -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);
Modified: trunk/Source/_javascript_Core/heap/Heap.h (247425 => 247426)
--- trunk/Source/_javascript_Core/heap/Heap.h 2019-07-14 22:18:03 UTC (rev 247425)
+++ trunk/Source/_javascript_Core/heap/Heap.h 2019-07-15 08:47:22 UTC (rev 247426)
@@ -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 };
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
