Skip to content

Commit 101392f

Browse files
dhechtdhecht
committed
[JSC] Remove SlotVisitor's Heap in preparation for a visitor that marks more than one heap
https://bugs.webkit.org/show_bug.cgi?id=325317 rdar://188428012 Reviewed by Yusuke Suzuki. Currently, a SlotVisitor holds the Heap it marks, which AbstractSlotVisitor::vm() and heap() rely on. We want one Collector to mark several heaps, so remove vm(), heap() and the Heap& behind them, leaving the visitor with only its Collector. What callers still asked through them is about the collection rather than a heap: * Structure and WeakBlock ask whether this is a full collection and whether a heap snapshot is being taken. The Collector now passes both to each visitor when marking starts, along with the marking version, and the visitor answers from its own fields. Heap::verifierMark does the same for VerifierSlotVisitor. * SlotVisitor::isMarked(const void*) uses the visitor's marking version instead of the heap's. * JSObject's assert that a cell is in the visitor's heap is removed. The visitor's data changes accordingly: * AbstractSlotVisitor loses Heap& m_heap and gains CollectionScope m_collectionScope. * HeapAnalyzer* m_heapAnalyzer moves from SlotVisitor to AbstractSlotVisitor, so that VerifierSlotVisitor has it too, and AbstractSlotVisitor::reset() clears it. * SlotVisitor::m_markingVersion stays, but is passed in rather than read from the heap. sizeof(SlotVisitor) stays at 256. Structure::visitChildren asks for the collection scope on every visit, and now reads it from the visitor instead of through the Heap. The uses that still need a heap, which are memory accounting, the world-state checks and the asserts that a cell belongs to the collection, go through Collector::heap() for now, and will be handled separately. No change in behavior. * Source/JavaScriptCore/heap/AbstractSlotVisitor.h: (JSC::AbstractSlotVisitor::collector const): (JSC::AbstractSlotVisitor::collectionScope const): (JSC::AbstractSlotVisitor::heapAnalyzer const): * Source/JavaScriptCore/heap/AbstractSlotVisitorInlines.h: (JSC::AbstractSlotVisitor::AbstractSlotVisitor): (JSC::AbstractSlotVisitor::didStartMarking): (JSC::AbstractSlotVisitor::reset): (JSC::AbstractSlotVisitor::heap const): Deleted. (JSC::AbstractSlotVisitor::collector const): Deleted. (JSC::AbstractSlotVisitor::vm): Deleted. (JSC::AbstractSlotVisitor::vm const): Deleted. * Source/JavaScriptCore/heap/Collector.cpp: (JSC::Collector::Collector): (JSC::Collector::runBeginPhase): * Source/JavaScriptCore/heap/Heap.cpp: (JSC::Heap::Heap): (JSC::Heap::clearConcurrentRetainedDataIfPossible): * Source/JavaScriptCore/heap/MarkingConstraint.cpp: (JSC::MarkingConstraint::execute): (JSC::MarkingConstraint::prepareToExecute): (JSC::MarkingConstraint::doParallelWork): * Source/JavaScriptCore/heap/SlotVisitor.cpp: (JSC::SlotVisitor::SlotVisitor): (JSC::SlotVisitor::didStartMarking): (JSC::SlotVisitor::reset): (JSC::SlotVisitor::appendJSCellOrAuxiliary): (JSC::SlotVisitor::appendToMarkStack): (JSC::SlotVisitor::markAuxiliary): (JSC::SlotVisitor::noteLiveAuxiliaryCell): (JSC::SlotVisitor::visitChildren): (JSC::SlotVisitor::propagateExternalMemoryVisitedIfNecessary): (JSC::SlotVisitor::updateMutatorIsStopped): (JSC::SlotVisitor::mutatorIsStoppedIsUpToDate const): (JSC::SlotVisitor::drainFromShared): (JSC::SlotVisitor::drainInParallelPassively): * Source/JavaScriptCore/heap/SlotVisitor.h: * Source/JavaScriptCore/heap/SlotVisitorInlines.h: (JSC::SlotVisitor::isMarked const): (JSC::SlotVisitor::reportExternalMemoryVisited): * Source/JavaScriptCore/heap/VerifierSlotVisitor.cpp: (JSC::VerifierSlotVisitor::VerifierSlotVisitor): (JSC::VerifierSlotVisitor::~VerifierSlotVisitor): (JSC::VerifierSlotVisitor::dumpMarkerData): (JSC::VerifierSlotVisitor::markAuxiliary): * Source/JavaScriptCore/heap/VerifierSlotVisitor.h: * Source/JavaScriptCore/heap/WeakBlock.cpp: (JSC::WeakBlock::specializedVisit): * Source/JavaScriptCore/runtime/JSObject.cpp: (JSC::JSObjectWithButterfly::markAuxiliaryAndVisitOutOfLineProperties): * Source/JavaScriptCore/runtime/Structure.cpp: (JSC::Structure::visitChildrenImpl): * Source/JavaScriptCore/SaferCPPExpectations/ForwardDeclCheckerExpectations: Canonical link: https://commits.webkit.org/322006@main
1 parent 501a722 commit 101392f

14 files changed

Lines changed: 81 additions & 92 deletions

‎Source/JavaScriptCore/SaferCPPExpectations/ForwardDeclCheckerExpectations‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ ftl/FTLState.h
1717
heap/AbstractSlotVisitor.h
1818
heap/HeapProfiler.h
1919
heap/HeapSnapshotBuilder.h
20-
heap/SlotVisitor.h
2120
inspector/InspectorBackendDispatchers.h
2221
jit/JIT.h
2322
jit/JITCode.h

‎Source/JavaScriptCore/heap/AbstractSlotVisitor.h‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
#pragma once
2727

28+
#include <JavaScriptCore/CollectionScope.h>
2829
#include <JavaScriptCore/JSCJSValue.h>
2930
#include <JavaScriptCore/MarkStack.h>
3031
#include <JavaScriptCore/RootMarkReason.h>
@@ -39,6 +40,7 @@ namespace JSC {
3940
class Collector;
4041
class ConservativeRoots;
4142
class Heap;
43+
class HeapAnalyzer;
4244
class HeapCell;
4345
class JSCell;
4446
class MarkedBlock;
@@ -139,10 +141,12 @@ class AbstractSlotVisitor {
139141

140142
inline bool isEmpty(); // Defined in AbstractSlotVisitorInlines.h
141143

142-
VM& vm();
143-
const VM& vm() const;
144-
Heap* heap() const;
145-
Collector& collector() const LIFETIME_BOUND;
144+
Collector& collector() const { return m_collector; }
145+
146+
CollectionScope collectionScope() const { return m_collectionScope; }
147+
HeapAnalyzer* heapAnalyzer() const { return m_heapAnalyzer; }
148+
149+
inline void NODELETE didStartMarking(CollectionScope, HeapAnalyzer*); // Defined in AbstractSlotVisitorInlines.h
146150

147151
virtual void append(const ConservativeRoots&) = 0;
148152

@@ -215,7 +219,7 @@ class AbstractSlotVisitor {
215219
const ASCIICString& codeName() const LIFETIME_BOUND { return m_codeName; }
216220

217221
protected:
218-
inline AbstractSlotVisitor(Heap&, Collector&, ASCIICString codeName, ConcurrentPtrHashSet&);
222+
inline AbstractSlotVisitor(Collector&, ASCIICString codeName, ConcurrentPtrHashSet&);
219223

220224
virtual void didAddOpaqueRoot(void*) { }
221225
virtual void didFindOpaqueRoot(void*) { }
@@ -228,7 +232,7 @@ class AbstractSlotVisitor {
228232

229233
size_t m_visitCount { 0 };
230234

231-
Heap& m_heap;
235+
HeapAnalyzer* m_heapAnalyzer { nullptr };
232236
Collector& m_collector;
233237
ReferrerContext* m_context { nullptr };
234238
ASCIICString m_codeName;
@@ -241,6 +245,7 @@ class AbstractSlotVisitor {
241245
bool m_suppressVerifier { false };
242246
bool m_ignoreNewOpaqueRoots { false }; // Useful as a debugging mode.
243247
bool m_needsExtraOpaqueRootHandling { false };
248+
CollectionScope m_collectionScope { CollectionScope::Full };
244249

245250
friend class MarkingConstraintSolver;
246251
};

‎Source/JavaScriptCore/heap/AbstractSlotVisitorInlines.h‎

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -106,34 +106,13 @@ inline AbstractSlotVisitor::ReferrerContext::~ReferrerContext()
106106
m_visitor.m_context = m_previous;
107107
}
108108

109-
inline AbstractSlotVisitor::AbstractSlotVisitor(JSC::Heap& heap, Collector& collector, ASCIICString codeName, ConcurrentPtrHashSet& opaqueRoots)
110-
: m_heap(heap)
111-
, m_collector(collector)
109+
inline AbstractSlotVisitor::AbstractSlotVisitor(Collector& collector, ASCIICString codeName, ConcurrentPtrHashSet& opaqueRoots)
110+
: m_collector(collector)
112111
, m_codeName(WTF::move(codeName))
113112
, m_opaqueRoots(opaqueRoots)
114113
{
115114
}
116115

117-
inline Heap* AbstractSlotVisitor::heap() const
118-
{
119-
return &m_heap;
120-
}
121-
122-
inline Collector& AbstractSlotVisitor::collector() const
123-
{
124-
return m_collector;
125-
}
126-
127-
inline VM& AbstractSlotVisitor::vm()
128-
{
129-
return m_heap.vm();
130-
}
131-
132-
inline const VM& AbstractSlotVisitor::vm() const
133-
{
134-
return m_heap.vm();
135-
}
136-
137116
inline bool AbstractSlotVisitor::addOpaqueRoot(void* ptr)
138117
{
139118
if (!ptr)
@@ -236,9 +215,16 @@ ALWAYS_INLINE ReferrerToken AbstractSlotVisitor::referrer() const
236215
return m_context->referrer();
237216
}
238217

218+
inline void AbstractSlotVisitor::didStartMarking(CollectionScope scope, HeapAnalyzer* analyzer)
219+
{
220+
m_collectionScope = scope;
221+
m_heapAnalyzer = analyzer;
222+
}
223+
239224
ALWAYS_INLINE void AbstractSlotVisitor::reset()
240225
{
241226
m_visitCount = 0;
227+
m_heapAnalyzer = nullptr;
242228
}
243229

244230
} // namespace JSC

‎Source/JavaScriptCore/heap/Collector.cpp‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "CodeBlock.h"
2626
#include "CodeBlockSetInlines.h"
2727
#include "CollectorInlines.h"
28+
#include "HeapAnalyzer.h"
2829
#include "HeapHelperPool.h"
2930
#include "HeapInlines.h"
3031
#include "HeapVerifier.h"
@@ -114,7 +115,7 @@ Collector::Collector(Heap& heap)
114115
, m_sharedCollectorMarkStack(makeUnique<MarkStackArray>())
115116
, m_sharedMutatorMarkStack(makeUnique<MarkStackArray>())
116117
, m_raceMarkStack(makeUnique<MarkStackArray>())
117-
, m_collectorSlotVisitor(makeUnique<SlotVisitor>(heap, *this, "C"_s))
118+
, m_collectorSlotVisitor(makeUnique<SlotVisitor>(*this, "C"_s))
118119
, m_helperClient(&heapHelperPool())
119120
, m_threadLock(Box<Lock>::create())
120121
, m_threadCondition(AutomaticThreadCondition::create())
@@ -290,7 +291,8 @@ NEVER_INLINE bool Collector::runBeginPhase(GCConductor conn)
290291
}
291292

292293
ASSERT(m_heap.m_collectionScope);
293-
bool isFullGC = m_heap.m_collectionScope.value() == CollectionScope::Full;
294+
CollectionScope scope = m_heap.m_collectionScope.value();
295+
bool isFullGC = scope == CollectionScope::Full;
294296
if (Options::useGCSignpost()) [[unlikely]] {
295297
StringPrintStream stream;
296298
stream.print("GC:(", RawPointer(&m_heap), "),mode:(", (isFullGC ? "Full" : "Eden"), "),version:(", m_heap.m_gcVersion, "),conn:(", gcConductorShortName(conn), "),capacity(", m_heap.capacity() / 1024, "kb)");
@@ -311,9 +313,11 @@ NEVER_INLINE bool Collector::runBeginPhase(GCConductor conn)
311313

312314
m_heap.beginMarking();
313315

316+
HeapVersion markingVersion = m_heap.objectSpace().markingVersion();
317+
HeapAnalyzer* heapAnalyzer = m_heap.vm().activeHeapAnalyzer();
314318
forEachSlotVisitor(
315319
[&] (SlotVisitor& visitor) {
316-
visitor.didStartMarking();
320+
visitor.didStartMarking(scope, markingVersion, heapAnalyzer);
317321
});
318322

319323
m_parallelMarkersShouldExit = false;

‎Source/JavaScriptCore/heap/Heap.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ Heap::Heap(VM& vm, HeapType heapType)
297297
, m_objectSpace(this)
298298
, m_machineThreads(makeUnique<MachineThreads>())
299299
, m_collector(makeUnique<Collector>(*this))
300-
, m_mutatorSlotVisitor(makeUnique<SlotVisitor>(*this, *m_collector, "M"_s))
300+
, m_mutatorSlotVisitor(makeUnique<SlotVisitor>(*m_collector, "M"_s))
301301
, m_mutatorMarkStack(makeUnique<MarkStackArray>())
302302
, m_constraintSet(makeUnique<MarkingConstraintSet>(*this))
303303
, m_strongSet(vm)
@@ -395,7 +395,7 @@ Heap::Heap(VM& vm, HeapType heapType)
395395
// FIXME: move Collector related initialization into Collector
396396

397397
for (unsigned i = 0, numberOfParallelThreads = heapHelperPool().numberOfThreads(); i < numberOfParallelThreads; ++i) {
398-
std::unique_ptr<SlotVisitor> visitor = makeUnique<SlotVisitor>(*this, *m_collector, toASCIICString("P", i + 1));
398+
std::unique_ptr<SlotVisitor> visitor = makeUnique<SlotVisitor>(*m_collector, toASCIICString("P", i + 1));
399399
if (Options::optimizeParallelSlotVisitorsForStoppedMutator())
400400
visitor->optimizeForStoppedMutator();
401401
m_collector->m_availableParallelSlotVisitors.append(visitor.get());
@@ -1800,7 +1800,7 @@ void Heap::willStartCollection()
18001800

18011801
++m_gcVersion;
18021802
if (Options::verifyGC()) [[unlikely]] {
1803-
m_verifierSlotVisitor = makeUnique<VerifierSlotVisitor>(*this, *m_collector);
1803+
m_verifierSlotVisitor = makeUnique<VerifierSlotVisitor>(*m_collector);
18041804
ASSERT(!m_isMarkingForGCVerifier);
18051805
}
18061806

@@ -2745,9 +2745,11 @@ void Heap::removeGCCompletionCallback(const GCCompletionCallback& callback)
27452745
void Heap::verifierMark()
27462746
{
27472747
RELEASE_ASSERT(!m_isMarkingForGCVerifier);
2748+
RELEASE_ASSERT(m_collectionScope);
27482749

27492750
SetForScope isMarkingForGCVerifierScope(m_isMarkingForGCVerifier, true);
27502751
VerifierSlotVisitor& visitor = *m_verifierSlotVisitor;
2752+
visitor.didStartMarking(m_collectionScope.value(), vm().activeHeapAnalyzer());
27512753
do {
27522754
while (!visitor.isEmpty())
27532755
visitor.drain();

‎Source/JavaScriptCore/heap/MarkingConstraint.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ void MarkingConstraint::resetStats()
5454

5555
void MarkingConstraint::execute(SlotVisitor& visitor)
5656
{
57-
ASSERT(!visitor.heap()->isMarkingForGCVerifier());
57+
ASSERT(!visitor.collector().heap().isMarkingForGCVerifier());
5858
VisitCounter visitCounter(visitor);
5959
executeImpl(visitor);
6060
m_lastVisitCount += visitCounter.visitCount();
@@ -80,7 +80,7 @@ double MarkingConstraint::workEstimate(SlotVisitor& visitor)
8080

8181
void MarkingConstraint::prepareToExecute(const AbstractLocker& constraintSolvingLocker, SlotVisitor& visitor)
8282
{
83-
ASSERT(!visitor.heap()->isMarkingForGCVerifier());
83+
ASSERT(!visitor.collector().heap().isMarkingForGCVerifier());
8484
dataLogIf(Options::logGC(), abbreviatedName());
8585
VisitCounter visitCounter(visitor);
8686
prepareToExecuteImpl(constraintSolvingLocker, visitor);
@@ -91,7 +91,7 @@ void MarkingConstraint::prepareToExecute(const AbstractLocker& constraintSolving
9191

9292
void MarkingConstraint::doParallelWork(SlotVisitor& visitor, SharedTask<void(SlotVisitor&)>& task)
9393
{
94-
ASSERT(!visitor.heap()->isMarkingForGCVerifier());
94+
ASSERT(!visitor.collector().heap().isMarkingForGCVerifier());
9595
VisitCounter visitCounter(visitor);
9696
task.run(visitor);
9797
if (verboseMarkingConstraint && visitCounter.visitCount())

0 commit comments

Comments
 (0)