Title: [131791] trunk/Source/_javascript_Core
- Revision
- 131791
- Author
- [email protected]
- Date
- 2012-10-18 12:44:25 -0700 (Thu, 18 Oct 2012)
Log Message
Race condition between GCThread and main thread during copying phase
https://bugs.webkit.org/show_bug.cgi?id=99641
Reviewed by Filip Pizlo.
When a GCThread returns from copyFromShared(), it then calls doneCopying(), which returns
its borrowed CopiedBlock to the CopiedSpace. This final block allows the CopiedSpace to
continue and finish the cleanup of the copying phase. However, the GCThread can loop back
around, see that m_currentPhase is still "Copy", and try to go through the copying phase again.
This can cause all sorts of issues. To fix this, we should add a cyclic barrier to GCThread::waitForNextPhase().
* heap/GCThread.cpp:
(JSC::GCThread::waitForNextPhase): All GCThreads will wait when they finish one iteration until the main thread
notifies them to move down to the second while loop, where they wait for the next GCPhase to start. They also
decrement the m_numberOfActiveGCThreads counter as they begin to wait for the next phase and increment it as
they enter the next phase. This allows the main thread to wait in endCurrentPhase() until all the threads have
finished the current phase and are waiting on the next phase to begin. Without the counter, there would be
no way to ensure that every thread was available for each GCPhase.
(JSC::GCThread::gcThreadMain): We now use the m_phaseLock to synchronize with the main thread when we're being created.
* heap/GCThreadSharedData.cpp:
(JSC::GCThreadSharedData::GCThreadSharedData): As we create each GCThread, we increment the m_numberOfActiveGCThreads
counter. When we are done creating the threads, we wait until they're all waiting for the next GCPhase. This prevents
us from leaving some GCThreads behind during the first GCPhase, which could hurt us on our very short-running
benchmarks (e.g. SunSpider).
(JSC::GCThreadSharedData::~GCThreadSharedData):
(JSC::GCThreadSharedData::startNextPhase): We atomically swap the two flags, m_gcThreadsShouldWait and m_currentPhase,
so that if the threads finish very quickly, they will wait until the main thread is ready to end the current phase.
(JSC::GCThreadSharedData::endCurrentPhase): Here atomically we swap the two flags again to allow the threads to
advance to waiting on the next GCPhase. We wait until all of the GCThreads have settled into the second wait loop
before allowing the main thread to continue. This prevents us from leaving one of the GCThreads stuck in the first
wait loop if we were to call startNextPhase() before it had time to wake up and move on to the second wait loop.
(JSC):
(JSC::GCThreadSharedData::didStartMarking): We now use startNextPhase() to properly swap the flags.
(JSC::GCThreadSharedData::didFinishMarking): Ditto for endCurrentPhase().
(JSC::GCThreadSharedData::didStartCopying): Ditto.
(JSC::GCThreadSharedData::didFinishCopying): Ditto.
* heap/GCThreadSharedData.h:
(GCThreadSharedData):
* heap/Heap.cpp:
(JSC::Heap::copyBackingStores): No reason to use the extra reference.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (131790 => 131791)
--- trunk/Source/_javascript_Core/ChangeLog 2012-10-18 19:41:04 UTC (rev 131790)
+++ trunk/Source/_javascript_Core/ChangeLog 2012-10-18 19:44:25 UTC (rev 131791)
@@ -1,3 +1,46 @@
+2012-10-18 Mark Hahnenberg <[email protected]>
+
+ Race condition between GCThread and main thread during copying phase
+ https://bugs.webkit.org/show_bug.cgi?id=99641
+
+ Reviewed by Filip Pizlo.
+
+ When a GCThread returns from copyFromShared(), it then calls doneCopying(), which returns
+ its borrowed CopiedBlock to the CopiedSpace. This final block allows the CopiedSpace to
+ continue and finish the cleanup of the copying phase. However, the GCThread can loop back
+ around, see that m_currentPhase is still "Copy", and try to go through the copying phase again.
+ This can cause all sorts of issues. To fix this, we should add a cyclic barrier to GCThread::waitForNextPhase().
+
+ * heap/GCThread.cpp:
+ (JSC::GCThread::waitForNextPhase): All GCThreads will wait when they finish one iteration until the main thread
+ notifies them to move down to the second while loop, where they wait for the next GCPhase to start. They also
+ decrement the m_numberOfActiveGCThreads counter as they begin to wait for the next phase and increment it as
+ they enter the next phase. This allows the main thread to wait in endCurrentPhase() until all the threads have
+ finished the current phase and are waiting on the next phase to begin. Without the counter, there would be
+ no way to ensure that every thread was available for each GCPhase.
+ (JSC::GCThread::gcThreadMain): We now use the m_phaseLock to synchronize with the main thread when we're being created.
+ * heap/GCThreadSharedData.cpp:
+ (JSC::GCThreadSharedData::GCThreadSharedData): As we create each GCThread, we increment the m_numberOfActiveGCThreads
+ counter. When we are done creating the threads, we wait until they're all waiting for the next GCPhase. This prevents
+ us from leaving some GCThreads behind during the first GCPhase, which could hurt us on our very short-running
+ benchmarks (e.g. SunSpider).
+ (JSC::GCThreadSharedData::~GCThreadSharedData):
+ (JSC::GCThreadSharedData::startNextPhase): We atomically swap the two flags, m_gcThreadsShouldWait and m_currentPhase,
+ so that if the threads finish very quickly, they will wait until the main thread is ready to end the current phase.
+ (JSC::GCThreadSharedData::endCurrentPhase): Here atomically we swap the two flags again to allow the threads to
+ advance to waiting on the next GCPhase. We wait until all of the GCThreads have settled into the second wait loop
+ before allowing the main thread to continue. This prevents us from leaving one of the GCThreads stuck in the first
+ wait loop if we were to call startNextPhase() before it had time to wake up and move on to the second wait loop.
+ (JSC):
+ (JSC::GCThreadSharedData::didStartMarking): We now use startNextPhase() to properly swap the flags.
+ (JSC::GCThreadSharedData::didFinishMarking): Ditto for endCurrentPhase().
+ (JSC::GCThreadSharedData::didStartCopying): Ditto.
+ (JSC::GCThreadSharedData::didFinishCopying): Ditto.
+ * heap/GCThreadSharedData.h:
+ (GCThreadSharedData):
+ * heap/Heap.cpp:
+ (JSC::Heap::copyBackingStores): No reason to use the extra reference.
+
2012-10-18 Pablo Flouret <[email protected]>
Implement css3-conditional's @supports rule
Modified: trunk/Source/_javascript_Core/heap/GCThread.cpp (131790 => 131791)
--- trunk/Source/_javascript_Core/heap/GCThread.cpp 2012-10-18 19:41:04 UTC (rev 131790)
+++ trunk/Source/_javascript_Core/heap/GCThread.cpp 2012-10-18 19:44:25 UTC (rev 131791)
@@ -70,8 +70,16 @@
GCPhase GCThread::waitForNextPhase()
{
MutexLocker locker(m_shared.m_phaseLock);
+ while (m_shared.m_gcThreadsShouldWait)
+ m_shared.m_phaseCondition.wait(m_shared.m_phaseLock);
+
+ m_shared.m_numberOfActiveGCThreads--;
+ if (!m_shared.m_numberOfActiveGCThreads)
+ m_shared.m_activityCondition.signal();
+
while (m_shared.m_currentPhase == NoPhase)
m_shared.m_phaseCondition.wait(m_shared.m_phaseLock);
+ m_shared.m_numberOfActiveGCThreads++;
return m_shared.m_currentPhase;
}
@@ -84,7 +92,7 @@
// Wait for the main thread to finish creating and initializing us. The main thread grabs this lock before
// creating this thread. We aren't guaranteed to have a valid threadID until the main thread releases this lock.
{
- MutexLocker locker(m_shared.m_markingLock);
+ MutexLocker locker(m_shared.m_phaseLock);
}
{
ParallelModeEnabler enabler(*m_slotVisitor);
Modified: trunk/Source/_javascript_Core/heap/GCThreadSharedData.cpp (131790 => 131791)
--- trunk/Source/_javascript_Core/heap/GCThreadSharedData.cpp 2012-10-18 19:41:04 UTC (rev 131790)
+++ trunk/Source/_javascript_Core/heap/GCThreadSharedData.cpp 2012-10-18 19:44:25 UTC (rev 131791)
@@ -61,13 +61,16 @@
, m_parallelMarkersShouldExit(false)
, m_blocksToCopy(globalData->heap.m_blockSnapshot)
, m_copyIndex(0)
+ , m_numberOfActiveGCThreads(0)
+ , m_gcThreadsShouldWait(false)
, m_currentPhase(NoPhase)
{
m_copyLock.Init();
#if ENABLE(PARALLEL_GC)
// Grab the lock so the new GC threads can be properly initialized before they start running.
- MutexLocker locker(m_markingLock);
+ MutexLocker locker(m_phaseLock);
for (unsigned i = 1; i < Options::numberOfGCMarkers(); ++i) {
+ m_numberOfActiveGCThreads++;
SlotVisitor* slotVisitor = new SlotVisitor(*this);
CopyVisitor* copyVisitor = new CopyVisitor(*this);
GCThread* newThread = new GCThread(*this, slotVisitor, copyVisitor);
@@ -75,6 +78,10 @@
newThread->initializeThreadID(threadID);
m_gcThreads.append(newThread);
}
+
+ // Wait for all the GCThreads to get to the right place.
+ while (m_numberOfActiveGCThreads)
+ m_activityCondition.wait(m_phaseLock);
#endif
}
@@ -87,6 +94,7 @@
MutexLocker phaseLocker(m_phaseLock);
ASSERT(m_currentPhase == NoPhase);
m_parallelMarkersShouldExit = true;
+ m_gcThreadsShouldWait = false;
m_currentPhase = Exit;
m_phaseCondition.broadcast();
}
@@ -115,24 +123,44 @@
}
}
-void GCThreadSharedData::didStartMarking()
+void GCThreadSharedData::startNextPhase(GCPhase phase)
{
- MutexLocker markingLocker(m_markingLock);
MutexLocker phaseLocker(m_phaseLock);
+ ASSERT(!m_gcThreadsShouldWait);
ASSERT(m_currentPhase == NoPhase);
- m_currentPhase = Mark;
- m_parallelMarkersShouldExit = false;
+ m_gcThreadsShouldWait = true;
+ m_currentPhase = phase;
m_phaseCondition.broadcast();
}
+void GCThreadSharedData::endCurrentPhase()
+{
+ ASSERT(m_gcThreadsShouldWait);
+ MutexLocker locker(m_phaseLock);
+ m_currentPhase = NoPhase;
+ m_gcThreadsShouldWait = false;
+ m_phaseCondition.broadcast();
+ while (m_numberOfActiveGCThreads)
+ m_activityCondition.wait(m_phaseLock);
+}
+
+void GCThreadSharedData::didStartMarking()
+{
+ MutexLocker markingLocker(m_markingLock);
+ m_parallelMarkersShouldExit = false;
+ startNextPhase(Mark);
+}
+
void GCThreadSharedData::didFinishMarking()
{
- MutexLocker markingLocker(m_markingLock);
- MutexLocker phaseLocker(m_phaseLock);
+ {
+ MutexLocker markingLocker(m_markingLock);
+ m_parallelMarkersShouldExit = true;
+ m_markingCondition.broadcast();
+ }
+
ASSERT(m_currentPhase == Mark);
- m_currentPhase = NoPhase;
- m_parallelMarkersShouldExit = true;
- m_markingCondition.broadcast();
+ endCurrentPhase();
}
void GCThreadSharedData::didStartCopying()
@@ -150,18 +178,13 @@
for (size_t i = 0; i < m_gcThreads.size(); i++)
m_gcThreads[i]->copyVisitor()->startCopying();
- MutexLocker locker(m_phaseLock);
- ASSERT(m_currentPhase == NoPhase);
- m_currentPhase = Copy;
- m_phaseCondition.broadcast();
+ startNextPhase(Copy);
}
void GCThreadSharedData::didFinishCopying()
{
- MutexLocker locker(m_phaseLock);
ASSERT(m_currentPhase == Copy);
- m_currentPhase = NoPhase;
- m_phaseCondition.broadcast();
+ endCurrentPhase();
}
} // namespace JSC
Modified: trunk/Source/_javascript_Core/heap/GCThreadSharedData.h (131790 => 131791)
--- trunk/Source/_javascript_Core/heap/GCThreadSharedData.h 2012-10-18 19:41:04 UTC (rev 131790)
+++ trunk/Source/_javascript_Core/heap/GCThreadSharedData.h 2012-10-18 19:44:25 UTC (rev 131791)
@@ -74,6 +74,8 @@
friend class CopyVisitor;
void getNextBlocksToCopy(size_t&, size_t&);
+ void startNextPhase(GCPhase);
+ void endCurrentPhase();
JSGlobalData* m_globalData;
CopiedSpace* m_copiedSpace;
@@ -100,6 +102,9 @@
Mutex m_phaseLock;
ThreadCondition m_phaseCondition;
+ ThreadCondition m_activityCondition;
+ unsigned m_numberOfActiveGCThreads;
+ bool m_gcThreadsShouldWait;
GCPhase m_currentPhase;
ListableHandler<WeakReferenceHarvester>::List m_weakReferenceHarvesters;
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (131790 => 131791)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2012-10-18 19:41:04 UTC (rev 131790)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2012-10-18 19:44:25 UTC (rev 131791)
@@ -612,10 +612,9 @@
m_storageSpace.startedCopying();
if (m_storageSpace.shouldDoCopyPhase()) {
m_sharedData.didStartCopying();
- CopyVisitor& visitor = m_copyVisitor;
- visitor.startCopying();
- visitor.copyFromShared();
- visitor.doneCopying();
+ m_copyVisitor.startCopying();
+ m_copyVisitor.copyFromShared();
+ m_copyVisitor.doneCopying();
// We need to wait for everybody to finish and return their CopiedBlocks
// before signaling that the phase is complete.
m_storageSpace.doneCopying();
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes
