Skip to content

Commit 5e53548

Browse files
dhechtdhecht
committed
[JSC] Move the marking constraint set to the Collector
https://bugs.webkit.org/show_bug.cgi?id=325513 rdar://188601461 Reviewed by Vassili Bykov. The constraint set is what a collection's fixpoint converges, and once a Collector marks more than one heap its constraints are no longer one heap's. Move it from Heap to Collector. * Collector owns the MarkingConstraintSet and exposes addMarkingConstraint(), which Heap uses to register its core constraints and embedders' constraints. It asserts that no collection is running, since a collection iterates the set. * MarkingConstraintSet drops the Heap it stored and never used. * MarkingConstraintSolver reaches the Collector through its main visitor instead of a Heap. * Leave defaults off Collector::addMarkingConstraint() parameters. There's a limited number of constraints, so I think it's better to make them explicit, especially since the constraints themselves will change somewhat to allow a Collector to mark multiple Heaps. Heap::verifierMark still runs the set directly; how the verifier should run constraints once it covers more than one heap is left for later. No change in behavior. * Source/JavaScriptCore/heap/Collector.cpp: (JSC::Collector::Collector): (JSC::Collector::runBeginPhase): (JSC::Collector::runFixpointPhase): (JSC::Collector::addMarkingConstraint): * Source/JavaScriptCore/heap/Collector.h: * Source/JavaScriptCore/heap/Heap.cpp: (JSC::Heap::Heap): (JSC::Heap::clearConcurrentRetainedDataIfPossible): * Source/JavaScriptCore/heap/Heap.h: * Source/JavaScriptCore/heap/MarkingConstraintSet.cpp: (JSC::MarkingConstraintSet::executeConvergenceImpl): (JSC::MarkingConstraintSet::MarkingConstraintSet): Deleted. * Source/JavaScriptCore/heap/MarkingConstraintSet.h: (JSC::MarkingConstraintSet::add): Deleted. * Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp: (JSC::MarkingConstraintSolver::MarkingConstraintSolver): (JSC::MarkingConstraintSolver::execute): * Source/JavaScriptCore/heap/MarkingConstraintSolver.h: Canonical link: https://commits.webkit.org/322104@main
1 parent cae8ae4 commit 5e53548

8 files changed

Lines changed: 73 additions & 51 deletions

File tree

‎Source/JavaScriptCore/heap/Collector.cpp‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ Collector::Collector(Heap& heap)
120120
, m_raceMarkStack(makeUnique<MarkStackArray>())
121121
, m_collectorSlotVisitor(makeUnique<SlotVisitor>(*this, "C"_s))
122122
, m_helperClient(&heapHelperPool())
123+
, m_constraintSet(makeUnique<MarkingConstraintSet>())
123124
, m_threadLock(Box<Lock>::create())
124125
, m_threadCondition(AutomaticThreadCondition::create())
125126
{
@@ -369,7 +370,7 @@ NEVER_INLINE bool Collector::runBeginPhase(GCConductor conn)
369370

370371
SlotVisitor& visitor = *m_collectorSlotVisitor;
371372

372-
m_heap.m_constraintSet->didStartMarking();
373+
m_constraintSet->didStartMarking();
373374

374375
m_scheduler->beginCollection();
375376
if (Options::logGC()) [[unlikely]]
@@ -427,7 +428,7 @@ NEVER_INLINE bool Collector::runFixpointPhase(GCConductor conn)
427428

428429
// Wondering what this does? Look at Heap::addCoreConstraints(). The DOM and others can also
429430
// add their own using Heap::addMarkingConstraint().
430-
bool converged = m_heap.m_constraintSet->executeConvergence(visitor);
431+
bool converged = m_constraintSet->executeConvergence(visitor);
431432

432433
// FIXME: The visitor.isEmpty() check is most likely not needed.
433434
// https://bugs.webkit.org/show_bug.cgi?id=180310
@@ -794,6 +795,19 @@ void Collector::resumeCompilerThreads()
794795
#endif
795796
}
796797

798+
void Collector::addMarkingConstraint(std::unique_ptr<MarkingConstraint> constraint)
799+
{
800+
ASSERT(!m_heap.m_collectionScope);
801+
m_constraintSet->add(WTF::move(constraint));
802+
}
803+
804+
void Collector::addMarkingConstraint(ASCIICString abbreviatedName, ASCIICString name, MarkingConstraintExecutorPair&& executors,
805+
ConstraintVolatility volatility, ConstraintConcurrency concurrency, ConstraintParallelism parallelism)
806+
{
807+
ASSERT(!m_heap.m_collectionScope);
808+
m_constraintSet->add(WTF::move(abbreviatedName), WTF::move(name), WTF::move(executors), volatility, concurrency, parallelism);
809+
}
810+
797811
size_t Collector::bytesVisited()
798812
{
799813
size_t result = 0;

‎Source/JavaScriptCore/heap/Collector.h‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@
2222
#pragma once
2323

2424
#include <JavaScriptCore/CollectorPhase.h>
25+
#include <JavaScriptCore/ConstraintConcurrency.h>
26+
#include <JavaScriptCore/ConstraintParallelism.h>
27+
#include <JavaScriptCore/ConstraintVolatility.h>
2528
#include <JavaScriptCore/GCConductor.h>
2629
#include <JavaScriptCore/GCRequest.h>
2730
#include <JavaScriptCore/MachineStackMarker.h>
@@ -44,6 +47,9 @@ namespace JSC {
4447

4548
class Heap;
4649
class MarkStackArray;
50+
class MarkingConstraint;
51+
class MarkingConstraintSet;
52+
struct MarkingConstraintExecutorPair;
4753
class MutatorScheduler;
4854
class SlotVisitor;
4955

@@ -71,6 +77,11 @@ class Collector {
7177
runTaskInParallel(createSharedTask<void(SlotVisitor&)>(func));
7278
}
7379

80+
// Only while no collection is running, since a collection iterates the constraints.
81+
void addMarkingConstraint(std::unique_ptr<MarkingConstraint>);
82+
void addMarkingConstraint(ASCIICString abbreviatedName, ASCIICString name, MarkingConstraintExecutorPair&&,
83+
ConstraintVolatility, ConstraintConcurrency, ConstraintParallelism);
84+
7485
private:
7586
class CollectorThread;
7687
friend class CollectorThread;
@@ -156,6 +167,8 @@ class Collector {
156167
Vector<SlotVisitor*> m_availableParallelSlotVisitors WTF_GUARDED_BY_LOCK(m_parallelSlotVisitorLock);
157168
ParallelHelperClient m_helperClient;
158169

170+
std::unique_ptr<MarkingConstraintSet> m_constraintSet;
171+
159172
// The phase machine, stepped once per transition.
160173
CollectorPhase m_lastPhase { CollectorPhase::NotRunning };
161174
CollectorPhase m_currentPhase { CollectorPhase::NotRunning };

‎Source/JavaScriptCore/heap/Heap.cpp‎

Lines changed: 34 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,6 @@ Heap::Heap(VM& vm, HeapType heapType)
296296
, m_collector(makeUnique<Collector>(*this))
297297
, m_mutatorSlotVisitor(makeUnique<SlotVisitor>(*m_collector, "M"_s))
298298
, m_mutatorMarkStack(makeUnique<MarkStackArray>())
299-
, m_constraintSet(makeUnique<MarkingConstraintSet>(*this))
300299
, m_strongSet(vm)
301300
, m_codeBlocks(makeUnique<CodeBlockSet>())
302301
, m_jitStubRoutines(makeUnique<JITStubRoutineSet>())
@@ -2374,7 +2373,7 @@ static UNUSED_FUNCTION void visitSamplingProfiler(VM&, AbstractSlotVisitor&) { }
23742373

23752374
void Heap::addCoreConstraints()
23762375
{
2377-
m_constraintSet->add(
2376+
m_collector->addMarkingConstraint(
23782377
"Cs"_s, "Conservative Scan"_s,
23792378
MAKE_MARKING_CONSTRAINT_EXECUTOR_PAIR(([this, lastVersion = static_cast<uint64_t>(0)] (auto& visitor) mutable {
23802379
bool shouldNotProduceWork = lastVersion == m_collector->m_phaseVersion;
@@ -2422,9 +2421,11 @@ void Heap::addCoreConstraints()
24222421
}
24232422
lastVersion = m_collector->m_phaseVersion;
24242423
})),
2425-
ConstraintVolatility::GreyedByExecution);
2424+
ConstraintVolatility::GreyedByExecution,
2425+
ConstraintConcurrency::Concurrent,
2426+
ConstraintParallelism::Sequential);
24262427

2427-
m_constraintSet->add(
2428+
m_collector->addMarkingConstraint(
24282429
"Msr"_s, "Misc Small Roots"_s,
24292430
MAKE_MARKING_CONSTRAINT_EXECUTOR_PAIR(([this] (auto& visitor) {
24302431
VM& vm = this->vm();
@@ -2464,18 +2465,22 @@ void Heap::addCoreConstraints()
24642465
visitor.appendUnbarriered(vm.m_terminationException);
24652466
}
24662467
})),
2467-
ConstraintVolatility::GreyedByExecution);
2468+
ConstraintVolatility::GreyedByExecution,
2469+
ConstraintConcurrency::Concurrent,
2470+
ConstraintParallelism::Sequential);
24682471

2469-
m_constraintSet->add(
2472+
m_collector->addMarkingConstraint(
24702473
"Sh"_s, "Strong Handles"_s,
24712474
MAKE_MARKING_CONSTRAINT_EXECUTOR_PAIR(([this] (auto& visitor) {
24722475
SetRootMarkReasonScope rootScope(visitor, RootMarkReason::StrongHandles);
24732476
m_strongSet.visitAggregate(visitor);
24742477
vm().visitAggregate(visitor);
24752478
})),
2476-
ConstraintVolatility::GreyedByExecution);
2479+
ConstraintVolatility::GreyedByExecution,
2480+
ConstraintConcurrency::Concurrent,
2481+
ConstraintParallelism::Sequential);
24772482

2478-
m_constraintSet->add(
2483+
m_collector->addMarkingConstraint(
24792484
"D"_s, "Debugger"_s,
24802485
MAKE_MARKING_CONSTRAINT_EXECUTOR_PAIR(([this] (auto& visitor) {
24812486
SetRootMarkReasonScope rootScope(visitor, RootMarkReason::Debugger);
@@ -2490,19 +2495,22 @@ void Heap::addCoreConstraints()
24902495
if (auto* shadowChicken = vm.shadowChicken())
24912496
shadowChicken->visitChildren(visitor);
24922497
})),
2493-
ConstraintVolatility::GreyedByExecution);
2498+
ConstraintVolatility::GreyedByExecution,
2499+
ConstraintConcurrency::Concurrent,
2500+
ConstraintParallelism::Sequential);
24942501

2495-
m_constraintSet->add(
2502+
m_collector->addMarkingConstraint(
24962503
"Ws"_s, "Weak Sets"_s,
24972504
MAKE_MARKING_CONSTRAINT_EXECUTOR_PAIR(([this] (auto& visitor) {
24982505
SetRootMarkReasonScope rootScope(visitor, RootMarkReason::WeakSets);
24992506
RefPtr<SharedTask<void(decltype(visitor)&)>> task = m_objectSpace.forEachWeakInParallel<decltype(visitor)>(visitor);
25002507
visitor.addParallelConstraintTask(WTF::move(task));
25012508
})),
25022509
ConstraintVolatility::GreyedByMarking,
2510+
ConstraintConcurrency::Concurrent,
25032511
ConstraintParallelism::Parallel);
25042512

2505-
m_constraintSet->add(
2513+
m_collector->addMarkingConstraint(
25062514
"O"_s, "Output"_s,
25072515
MAKE_MARKING_CONSTRAINT_EXECUTOR_PAIR(([this] (auto& visitor) {
25082516
auto callOutputConstraint = [] (auto& visitor, HeapCell* heapCell, HeapCell::Kind) {
@@ -2531,10 +2539,11 @@ void Heap::addCoreConstraints()
25312539
}
25322540
})),
25332541
ConstraintVolatility::GreyedByMarking,
2542+
ConstraintConcurrency::Concurrent,
25342543
ConstraintParallelism::Parallel);
25352544

25362545
#if ENABLE(WEBASSEMBLY)
2537-
m_constraintSet->add(
2546+
m_collector->addMarkingConstraint(
25382547
"Pbc"_s, "Pinball Completions"_s,
25392548
MAKE_MARKING_CONSTRAINT_EXECUTOR_PAIR(([this] (auto& visitor) {
25402549
// FIXME: Unlike the "Cs" constraint which is skipped during verification
@@ -2561,12 +2570,13 @@ void Heap::addCoreConstraints()
25612570
visitor.append(conservativeRoots);
25622571
})),
25632572
ConstraintVolatility::GreyedByMarking,
2564-
ConstraintConcurrency::Sequential);
2573+
ConstraintConcurrency::Sequential,
2574+
ConstraintParallelism::Sequential);
25652575
#endif
25662576

25672577
#if ENABLE(JIT)
25682578
if (Options::useJIT()) {
2569-
m_constraintSet->add(
2579+
m_collector->addMarkingConstraint(
25702580
"Jw"_s, "JIT Worklist"_s,
25712581
MAKE_MARKING_CONSTRAINT_EXECUTOR_PAIR(([this] (auto& visitor) {
25722582
SetRootMarkReasonScope rootScope(visitor, RootMarkReason::JITWorkList);
@@ -2584,11 +2594,13 @@ void Heap::addCoreConstraints()
25842594
if (Options::logGC() == GCLogging::Verbose)
25852595
dataLog("JIT Worklists:\n", visitor);
25862596
})),
2587-
ConstraintVolatility::GreyedByMarking);
2597+
ConstraintVolatility::GreyedByMarking,
2598+
ConstraintConcurrency::Concurrent,
2599+
ConstraintParallelism::Sequential);
25882600
}
25892601
#endif
25902602

2591-
m_constraintSet->add(
2603+
m_collector->addMarkingConstraint(
25922604
"Cb"_s, "CodeBlocks"_s,
25932605
MAKE_MARKING_CONSTRAINT_EXECUTOR_PAIR(([this] (auto& visitor) {
25942606
SetRootMarkReasonScope rootScope(visitor, RootMarkReason::CodeBlocks);
@@ -2600,15 +2612,17 @@ void Heap::addCoreConstraints()
26002612
visitor.visitAsConstraint(codeBlock);
26012613
});
26022614
})),
2603-
ConstraintVolatility::SeldomGreyed);
2615+
ConstraintVolatility::SeldomGreyed,
2616+
ConstraintConcurrency::Concurrent,
2617+
ConstraintParallelism::Sequential);
26042618

2605-
m_constraintSet->add(makeUnique<MarkStackMergingConstraint>(*this));
2619+
m_collector->addMarkingConstraint(makeUnique<MarkStackMergingConstraint>(*this));
26062620
}
26072621

26082622
void Heap::addMarkingConstraint(std::unique_ptr<MarkingConstraint> constraint)
26092623
{
26102624
PreventCollectionScope preventCollectionScope(*this);
2611-
m_constraintSet->add(WTF::move(constraint));
2625+
m_collector->addMarkingConstraint(WTF::move(constraint));
26122626
}
26132627

26142628
void Heap::notifyIsSafeToCollect()
@@ -2720,7 +2734,7 @@ void Heap::verifierMark()
27202734
do {
27212735
while (!visitor.isEmpty())
27222736
visitor.drain();
2723-
m_constraintSet->executeAllSynchronously(visitor);
2737+
m_collector->m_constraintSet->executeAllSynchronously(visitor);
27242738
visitor.executeConstraintTasks();
27252739
} while (!visitor.isEmpty());
27262740

‎Source/JavaScriptCore/heap/Heap.h‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,6 @@ class MarkStackMergingConstraint;
9393
class BlockDirectory;
9494
class MarkedVectorBase;
9595
class MarkingConstraint;
96-
class MarkingConstraintSet;
9796
class MutatorScheduler;
9897
class RunningScope;
9998
class SlotVisitor;
@@ -851,7 +850,6 @@ class Heap {
851850
const std::unique_ptr<Collector> m_collector;
852851
std::unique_ptr<SlotVisitor> m_mutatorSlotVisitor;
853852
std::unique_ptr<MarkStackArray> m_mutatorMarkStack;
854-
std::unique_ptr<MarkingConstraintSet> m_constraintSet;
855853
std::unique_ptr<VerifierSlotVisitor> m_verifierSlotVisitor;
856854

857855
StrongSet m_strongSet;

‎Source/JavaScriptCore/heap/MarkingConstraintSet.cpp‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,7 @@ namespace JSC {
3636

3737
WTF_MAKE_TZONE_ALLOCATED_IMPL(MarkingConstraintSet);
3838

39-
MarkingConstraintSet::MarkingConstraintSet(JSC::Heap& heap)
40-
: m_heap(heap)
41-
{
42-
}
39+
MarkingConstraintSet::MarkingConstraintSet() = default;
4340

4441
MarkingConstraintSet::~MarkingConstraintSet() = default;
4542

@@ -97,7 +94,7 @@ bool MarkingConstraintSet::isWavefrontAdvancing(SlotVisitor& visitor)
9794
bool MarkingConstraintSet::executeConvergenceImpl(SlotVisitor& visitor)
9895
{
9996
SuperSamplerScope superSamplerScope(false);
100-
MarkingConstraintSolver solver(*this);
97+
MarkingConstraintSolver solver(*this, visitor);
10198

10299
unsigned iteration = m_iteration++;
103100

‎Source/JavaScriptCore/heap/MarkingConstraintSet.h‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,13 @@
3333

3434
namespace JSC {
3535

36-
class Heap;
3736
class MarkingConstraintSolver;
3837

3938
class MarkingConstraintSet {
4039
WTF_MAKE_TZONE_ALLOCATED(MarkingConstraintSet);
4140
WTF_MAKE_NONCOPYABLE(MarkingConstraintSet);
4241
public:
43-
MarkingConstraintSet(Heap&);
42+
MarkingConstraintSet();
4443
~MarkingConstraintSet();
4544

4645
void didStartMarking();
@@ -50,17 +49,8 @@ class MarkingConstraintSet {
5049
ASCIICString name,
5150
MarkingConstraintExecutorPair&&,
5251
ConstraintVolatility,
53-
ConstraintConcurrency = ConstraintConcurrency::Concurrent,
54-
ConstraintParallelism = ConstraintParallelism::Sequential);
55-
56-
void add(
57-
ASCIICString abbreviatedName, ASCIICString name,
58-
MarkingConstraintExecutorPair&& executors,
59-
ConstraintVolatility volatility,
60-
ConstraintParallelism parallelism)
61-
{
62-
add(WTF::move(abbreviatedName), WTF::move(name), WTF::move(executors), volatility, ConstraintConcurrency::Concurrent, parallelism);
63-
}
52+
ConstraintConcurrency,
53+
ConstraintParallelism);
6454

6555
void add(std::unique_ptr<MarkingConstraint>);
6656

@@ -87,7 +77,6 @@ class MarkingConstraintSet {
8777

8878
bool executeConvergenceImpl(SlotVisitor&);
8979

90-
JSC::Heap& m_heap;
9180
BitVector m_unexecutedRoots;
9281
BitVector m_unexecutedOutgrowths;
9382
Vector<std::unique_ptr<MarkingConstraint>> m_set;

‎Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,11 @@
3333

3434
namespace JSC {
3535

36-
MarkingConstraintSolver::MarkingConstraintSolver(MarkingConstraintSet& set)
37-
: m_heap(set.m_heap)
38-
, m_mainVisitor(m_heap.collectorSlotVisitor())
36+
MarkingConstraintSolver::MarkingConstraintSolver(MarkingConstraintSet& set, SlotVisitor& mainVisitor)
37+
: m_mainVisitor(mainVisitor)
3938
, m_set(set)
4039
{
41-
m_heap.collector().forEachSlotVisitor(
40+
m_mainVisitor.collector().forEachSlotVisitor(
4241
[&] (SlotVisitor& visitor) {
4342
m_visitCounters.append(VisitCounter(visitor));
4443
});
@@ -63,7 +62,7 @@ void MarkingConstraintSolver::execute(SchedulerPreference preference, const Scop
6362
if (Options::useParallelMarkingConstraintSolver()) {
6463
dataLogIf(Options::logGC(), preference == ParallelWorkFirst ? "P" : "N", "<");
6564

66-
m_heap.collector().runFunctionInParallel(
65+
m_mainVisitor.collector().runFunctionInParallel(
6766
[&] (SlotVisitor& visitor) { runExecutionThread(visitor, preference, pickNext); });
6867

6968
dataLogIf(Options::logGC(), ">");

‎Source/JavaScriptCore/heap/MarkingConstraintSolver.h‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737

3838
namespace JSC {
3939

40-
class Heap;
4140
class MarkingConstraint;
4241
class MarkingConstraintSet;
4342

@@ -46,7 +45,7 @@ class MarkingConstraintSolver {
4645
WTF_MAKE_TZONE_ALLOCATED(MarkingConstraintSolver);
4746

4847
public:
49-
MarkingConstraintSolver(MarkingConstraintSet&);
48+
MarkingConstraintSolver(MarkingConstraintSet&, SlotVisitor& mainVisitor);
5049
~MarkingConstraintSolver();
5150

5251
bool NODELETE didVisitSomething() const;
@@ -85,7 +84,6 @@ class MarkingConstraintSolver {
8584
MarkingConstraint* constraint { nullptr };
8685
};
8786

88-
JSC::Heap& m_heap;
8987
SlotVisitor& m_mainVisitor;
9088
MarkingConstraintSet& m_set;
9189
BitVector m_executed;

0 commit comments

Comments
 (0)