Skip to content

Commit 1d03118

Browse files
dhechtdhecht
committed
[JSC] Separate the Heap's shutdown from its finalization
https://bugs.webkit.org/show_bug.cgi?id=325527 rdar://188620913 Reviewed by Yusuke Suzuki. Heap::lastChanceToFinalize() did two jobs: it shut down the heap's collection, including bringing the collector thread down, and then finalized what was left in the heap. Only the second is what its name says, and bringing the thread down is the Collector's. Separating them leaves one place to change once a Collector serves more than one heap, when a heap going away leaves the Collector rather than stopping it. * Heap::shutDown(), called from VM::~VM, does the shutdown and then calls Heap::lastChanceToFinalize(), which now only finalizes and is private. * Collector::stopThread() brings the collector thread down. No change in behavior. * Source/JavaScriptCore/heap/Collector.cpp: (JSC::Collector::stopThread): * Source/JavaScriptCore/heap/Collector.h: * Source/JavaScriptCore/heap/Heap.cpp: (JSC::Heap::shutDown): (JSC::Heap::lastChanceToFinalize): * Source/JavaScriptCore/heap/Heap.h: * Source/JavaScriptCore/runtime/VM.cpp: (JSC::VM::~VM): Canonical link: https://commits.webkit.org/322121@main
1 parent 6999b70 commit 1d03118

5 files changed

Lines changed: 41 additions & 34 deletions

File tree

‎Source/JavaScriptCore/heap/Collector.cpp‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,23 @@ Collector::~Collector()
154154
m_raceMarkStack->clear();
155155
}
156156

157+
void Collector::stopThread()
158+
{
159+
RELEASE_ASSERT(m_requests.isEmpty());
160+
RELEASE_ASSERT(!hasOutstandingRequest());
161+
162+
bool stopped = false;
163+
{
164+
Locker locker { *m_threadLock };
165+
stopped = m_thread->tryStop(locker);
166+
m_threadShouldStop = true;
167+
if (!stopped)
168+
m_threadCondition->notifyOne(locker);
169+
}
170+
if (!stopped)
171+
m_thread->join();
172+
}
173+
157174
void Collector::assertMarkStacksEmpty()
158175
{
159176
bool ok = true;

‎Source/JavaScriptCore/heap/Collector.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,8 @@ class Collector {
105105
void startCollectingContinuously();
106106
void stopCollectingContinuously();
107107

108+
void stopThread();
109+
108110
enum class RunCurrentPhaseResult {
109111
Finished,
110112
Continue,

‎Source/JavaScriptCore/heap/Heap.cpp‎

Lines changed: 19 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -437,72 +437,61 @@ void Heap::dumpHeapStatisticsAtVMDestruction()
437437
}
438438

439439
// The VM is being destroyed and the collector will never run again.
440-
// Run all pending finalizers now because we won't get another chance.
441-
void Heap::lastChanceToFinalize()
440+
void Heap::shutDown()
442441
{
443442
MonotonicTime before;
444443
if (Options::logGC()) [[unlikely]] {
445444
before = MonotonicTime::now();
446-
dataLog("[GC<", RawPointer(this), ">: shutdown ");
445+
dataLog("[GC<", RawPointer(this), ">: shutdown: ");
447446
}
448447

449448
m_isShuttingDown = true;
450449

451450
RELEASE_ASSERT(!vm().entryScope);
452451
RELEASE_ASSERT(m_mutatorState == MutatorState::Running);
453452

453+
// The collect-continuously thread requests a collection whenever the queue is empty, so it has to
454+
// stop before the queue drains.
454455
m_collector->stopCollectingContinuously();
455-
456-
dataLogIf(Options::logGC(), "1");
456+
dataLogIf(Options::logGC(), "stopped collecting continuously, ");
457457

458458
// Prevent new collections from being started. This is probably not even necessary, since we're not
459459
// going to call into anything that starts collections. Still, this makes the algorithm more
460460
// obviously sound.
461461
m_isSafeToCollect = false;
462-
463-
dataLogIf(Options::logGC(), "2");
462+
dataLogIf(Options::logGC(), "disabled collection, ");
464463

465464
bool isCollecting;
466465
{
467466
Locker locker { *m_collector->m_threadLock };
468467
isCollecting = m_collector->hasOutstandingRequest();
469468
}
470469
if (isCollecting) {
471-
dataLogIf(Options::logGC(), "...]\n");
470+
dataLogIf(Options::logGC(), "waiting for the current collection...]\n");
472471

473472
// Wait for the current collection to finish.
474473
waitForCollector(
475474
[&] (const AbstractLocker&) -> bool {
476475
return !m_collector->hasOutstandingRequest();
477476
});
478477

479-
dataLogIf(Options::logGC(), "[GC<", RawPointer(this), ">: shutdown ");
478+
dataLogIf(Options::logGC(), "[GC<", RawPointer(this), ">: shutdown: ");
480479
}
481-
dataLogIf(Options::logGC(), "3");
480+
dataLogIf(Options::logGC(), "no collection pending, stopping the collector thread, ");
482481

483-
RELEASE_ASSERT(m_collector->m_requests.isEmpty());
484-
RELEASE_ASSERT(!m_collector->hasOutstandingRequest());
485-
486-
// Carefully bring the thread down.
487-
bool stopped = false;
488-
{
489-
Locker locker { *m_collector->m_threadLock };
490-
stopped = m_collector->m_thread->tryStop(locker);
491-
m_collector->m_threadShouldStop = true;
492-
if (!stopped)
493-
m_collector->m_threadCondition->notifyOne(locker);
494-
}
495-
496-
dataLogIf(Options::logGC(), "4");
497-
498-
if (!stopped)
499-
m_collector->m_thread->join();
500-
501-
dataLogIf(Options::logGC(), "5 ");
482+
m_collector->stopThread();
483+
dataLogIf(Options::logGC(), "stopped, ");
502484

503485
if (Options::dumpHeapStatisticsAtVMDestruction()) [[unlikely]]
504486
dumpHeapStatisticsAtVMDestruction();
505-
487+
488+
lastChanceToFinalize();
489+
dataLogIf(Options::logGC(), (MonotonicTime::now() - before).milliseconds(), "ms]\n");
490+
}
491+
492+
// Run all pending finalizers now because we won't get another chance.
493+
void Heap::lastChanceToFinalize()
494+
{
506495
m_arrayBuffers.lastChanceToFinalize();
507496
m_objectSpace.lastChanceToFinalize();
508497
releaseDelayedReleasedObjects();
@@ -511,8 +500,6 @@ void Heap::lastChanceToFinalize()
511500
#endif
512501

513502
m_objectSpace.freeMemory();
514-
515-
dataLogIf(Options::logGC(), (MonotonicTime::now() - before).milliseconds(), "ms]\n");
516503
}
517504

518505
void Heap::releaseDelayedReleasedObjects()

‎Source/JavaScriptCore/heap/Heap.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ class Heap {
348348

349349
Heap(VM&, HeapType);
350350
~Heap();
351-
void lastChanceToFinalize();
351+
void shutDown();
352352
void releaseDelayedReleasedObjects();
353353

354354
VM& vm() const;
@@ -796,6 +796,7 @@ class Heap {
796796
void iterateExecutingAndCompilingCodeBlocksWithoutHoldingLocks(Visitor&, const Func&);
797797

798798
void dumpHeapStatisticsAtVMDestruction();
799+
void lastChanceToFinalize();
799800

800801
static bool useGenerationalGC();
801802
bool shouldSweepSynchronously();

‎Source/JavaScriptCore/runtime/VM.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -640,7 +640,7 @@ VM::~VM()
640640
ASSERT(currentThreadIsHoldingAPILock());
641641
m_apiLock->willDestroyVM(this);
642642
smallStrings.setIsInitialized(false);
643-
heap.lastChanceToFinalize();
643+
heap.shutDown();
644644

645645
while (!m_microtaskQueues.isEmpty())
646646
m_microtaskQueues.begin()->remove();

0 commit comments

Comments
 (0)