From fe4316847893242bf3e1c67ed09cbaf89f205575 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Tue, 14 Sep 2021 07:49:39 +0000 Subject: [PATCH] Bug 1730140 - Remove the gray root buffer and mark gray roots after the start of collection r=sfink,mccr8 This removes gray root buffering from the first marking slice and traces the gray roots directly in a later slice. This relies on Heap read barriers being sufficient to ensure correctness. This is conservative in that it makes no effort to skip tracing roots added after the start of GC. It also doesn't trace roots removed after the start of GC, but this is OK because barriers ensure marking of any observed values. The gray root tracing callback will be called once per sweep group, which means we will trace all zone holders and xpconnect gray roots for every group rather than just once. This should not be a problem in practice as we expect the number of zones and hence zone groups to decrease with fission. On the plus side we no longer have to do a virtual dispatch per root traced (for the buffering tracer), allocate memory for the buffer, or trace each root twice. Note that this doesn't make the gray root marking itself incremental yet. Differential Revision: https://phabricator.services.mozilla.com/D125188 --- js/public/GCAPI.h | 6 +- js/src/gc/Compacting.cpp | 2 +- js/src/gc/GC.cpp | 39 +------ js/src/gc/GCRuntime.h | 24 ---- js/src/gc/Marking.cpp | 13 ++- js/src/gc/RootMarking.cpp | 148 ------------------------- js/src/gc/Zone.cpp | 1 - js/src/gc/Zone.h | 8 -- js/src/jsapi.cpp | 4 - js/src/jsfriendapi.h | 10 ++ xpcom/base/CycleCollectedJSRuntime.cpp | 6 +- xpcom/base/CycleCollectedJSRuntime.h | 2 +- 12 files changed, 33 insertions(+), 230 deletions(-) diff --git a/js/public/GCAPI.h b/js/public/GCAPI.h index c4788c34f73f..6bbb9981972b 100644 --- a/js/public/GCAPI.h +++ b/js/public/GCAPI.h @@ -15,6 +15,7 @@ #include "mozilla/Vector.h" #include "js/GCAnnotations.h" +#include "js/shadow/Zone.h" #include "js/TypeDecls.h" #include "js/UniquePtr.h" #include "js/Utility.h" @@ -1197,7 +1198,10 @@ extern JS_PUBLIC_API void SetHostCleanupFinalizationRegistryCallback( */ extern JS_PUBLIC_API void ClearKeptObjects(JSContext* cx); -extern JS_PUBLIC_API bool ZoneIsCollecting(Zone* zone); +inline JS_PUBLIC_API bool ZoneIsGrayMarking(Zone* zone) { + return shadow::Zone::from(zone)->isGCMarkingBlackAndGray(); +} + extern JS_PUBLIC_API bool AtomsZoneIsCollecting(JSRuntime* runtime); extern JS_PUBLIC_API bool IsAtomsZone(Zone* zone); diff --git a/js/src/gc/Compacting.cpp b/js/src/gc/Compacting.cpp index 08fd31b3c123..17f732794532 100644 --- a/js/src/gc/Compacting.cpp +++ b/js/src/gc/Compacting.cpp @@ -860,7 +860,7 @@ void GCRuntime::updateRuntimePointersToRelocatedCells(AutoGCSession& session) { DebugAPI::traceAllForMovingGC(&trc); DebugAPI::traceCrossCompartmentEdges(&trc); - // Mark all gray roots. We call the trace callback to get the current set. + // Mark all gray roots. traceEmbeddingGrayRoots(&trc); Compartment::traceIncomingCrossCompartmentEdgesForZoneGC( &trc, Compartment::GrayEdges); diff --git a/js/src/gc/GC.cpp b/js/src/gc/GC.cpp index 157d01a555ad..282ad7ddff9e 100644 --- a/js/src/gc/GC.cpp +++ b/js/src/gc/GC.cpp @@ -1144,7 +1144,6 @@ GCRuntime::GCRuntime(JSRuntime* rt) perZoneGCEnabled(TuningDefaults::PerZoneGCEnabled), numActiveZoneIters(0), cleanUpEverything(false), - grayBufferState(GCRuntime::GrayBufferState::Unused), grayBitsValid(false), majorGCTriggerReason(JS::GCReason::NO_REASON), fullGCForAtomsRequested_(false), @@ -3717,17 +3716,6 @@ void GCRuntime::endPreparePhase(JS::GCReason reason) { gcstats::PhaseKind::UNMARK_WEAKMAPS, helperLock); - /* - * Buffer gray roots for incremental collections. This is linear in the - * number of roots which can be in the tens of thousands. Do this in - * parallel with the rest of this block. - */ - Maybe bufferGrayRootsTask; - if (isIncremental) { - bufferGrayRootsTask.emplace(this, &GCRuntime::bufferGrayRoots, - gcstats::PhaseKind::BUFFER_GRAY_ROOTS, - helperLock); - } AutoUnlockHelperThreadState unlock(helperLock); // Discard JIT code. For incremental collections, the sweep phase will @@ -3961,16 +3949,10 @@ void GCRuntime::markGrayRoots(gcstats::PhaseKind phase) { MOZ_ASSERT(marker.markColor() == MarkColor::Gray); gcstats::AutoPhase ap(stats(), phase); - if (hasValidGrayRootsBuffer()) { - for (ZoneIterT zone(this); !zone.done(); zone.next()) { - markBufferedGrayRoots(zone); - } - } else { - MOZ_ASSERT(!isIncremental); - traceEmbeddingGrayRoots(&marker); - Compartment::traceIncomingCrossCompartmentEdgesForZoneGC( - &marker, Compartment::GrayEdges); - } + + traceEmbeddingGrayRoots(&marker); + Compartment::traceIncomingCrossCompartmentEdgesForZoneGC( + &marker, Compartment::GrayEdges); } IncrementalProgress GCRuntime::markAllWeakReferences() { @@ -4184,7 +4166,6 @@ void GCRuntime::getNextSweepGroup() { zone->changeGCState(Zone::MarkBlackOnly, Zone::NoGC); zone->arenas.unmarkPreMarkedFreeCells(); zone->arenas.mergeNewArenasInMarkPhase(); - zone->gcGrayRoots().Clear(); zone->clearGCSliceThresholds(); } @@ -5763,8 +5744,6 @@ void GCRuntime::finishCollection() { MOZ_ASSERT(marker.isDrained()); marker.stop(); - clearBufferedGrayRoots(); - maybeStopPretenuring(); { @@ -5971,7 +5950,6 @@ GCRuntime::IncrementalResult GCRuntime::resetIncrementalGC( case State::Mark: { // Cancel any ongoing marking. marker.reset(); - clearBufferedGrayRoots(); for (GCCompartmentsIter c(rt); !c.done(); c.next()) { ResetGrayList(c); @@ -6182,16 +6160,7 @@ void GCRuntime::incrementalSlice(SliceBudget& budget, } endPreparePhase(reason); - beginMarkPhase(session); - - // If we needed delayed marking for gray roots, then collect until done. - if (isIncremental && !hasValidGrayRootsBuffer()) { - budget = SliceBudget::unlimited(); - isIncremental = false; - stats().nonincremental(GCAbortReason::GrayRootBufferingFailed); - } - incrementalState = State::Mark; if (useZeal && hasZealMode(ZealMode::YieldBeforeMarking) && diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h index ab7d99d837fc..4dff1efb2df4 100644 --- a/js/src/gc/GCRuntime.h +++ b/js/src/gc/GCRuntime.h @@ -726,7 +726,6 @@ class GCRuntime { [[nodiscard]] bool beginPreparePhase(JS::GCReason reason, AutoGCSession& session); bool prepareZonesForCollection(JS::GCReason reason, bool* isFullOut); - void bufferGrayRoots(); void unmarkWeakMaps(); void endPreparePhase(JS::GCReason reason); void beginMarkPhase(AutoGCSession& session); @@ -749,27 +748,6 @@ class GCRuntime { void maybeDoCycleCollection(); void findDeadCompartments(); - // Gray marking must be done after all black marking is complete. However, - // we do not have write barriers on XPConnect roots. Therefore, XPConnect - // roots must be accumulated in the first slice of incremental GC. We - // accumulate these roots in each zone's gcGrayRoots vector and then mark - // them later, after black marking is complete for each compartment. This - // accumulation can fail, but in that case we switch to non-incremental GC. - enum class GrayBufferState { Unused, Okay, Failed }; - - bool hasValidGrayRootsBuffer() const { - return grayBufferState == GrayBufferState::Okay; - } - - // Clear each zone's gray buffers, but do not change the current state. - void resetBufferedGrayRoots(); - - // Reset the gray buffering state to Unused. - void clearBufferedGrayRoots() { - grayBufferState = GrayBufferState::Unused; - resetBufferedGrayRoots(); - } - friend class BackgroundMarkTask; IncrementalProgress markUntilBudgetExhausted( SliceBudget& sliceBudget, @@ -987,8 +965,6 @@ class GCRuntime { /* During shutdown, the GC needs to clean up every possible object. */ MainThreadData cleanUpEverything; - MainThreadOrGCTaskData grayBufferState; - /* * The gray bits can become invalid if UnmarkGray overflows the stack. A * full GC will reset this bit, since it fills in all the gray bits. diff --git a/js/src/gc/Marking.cpp b/js/src/gc/Marking.cpp index d09658387e15..e4e348a430af 100644 --- a/js/src/gc/Marking.cpp +++ b/js/src/gc/Marking.cpp @@ -946,6 +946,14 @@ void DoMarking(GCMarker* gcmarker, T* thing) { return; } + // Don't mark gray in zones which we are still marking black, except for the + // atoms zone. + Zone* zone = thing->asTenured().zone(); + if (gcmarker->markColor() == MarkColor::Gray && + zone->isGCMarkingBlackOnly() && !zone->isAtomsZone()) { + return; + } + CheckTracedThing(gcmarker, thing); AutoClearTracingSource acts(gcmarker); gcmarker->markAndTraverse(thing); @@ -1175,7 +1183,7 @@ struct TraceKindCanBeGray {}; #define EXPAND_TRACEKIND_DEF(_, type, canBeGray, _1) \ template <> \ struct TraceKindCanBeGray { \ - static const bool value = canBeGray; \ + static constexpr bool value = canBeGray; \ }; JS_FOR_EACH_TRACEKIND(EXPAND_TRACEKIND_DEF) #undef EXPAND_TRACEKIND_DEF @@ -2715,9 +2723,6 @@ void GCMarker::checkZone(void* p) { size_t GCMarker::sizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const { size_t size = stack.sizeOfExcludingThis(mallocSizeOf); size += auxStack.sizeOfExcludingThis(mallocSizeOf); - for (ZonesIter zone(runtime(), WithAtoms); !zone.done(); zone.next()) { - size += zone->gcGrayRoots().SizeOfExcludingThis(mallocSizeOf); - } return size; } diff --git a/js/src/gc/RootMarking.cpp b/js/src/gc/RootMarking.cpp index 04a36add2399..68a93718b722 100644 --- a/js/src/gc/RootMarking.cpp +++ b/js/src/gc/RootMarking.cpp @@ -468,154 +468,6 @@ void js::gc::GCRuntime::checkNoRuntimeRoots(AutoGCSession& session) { #endif // DEBUG } -// Append traced things to a buffer on the zone for use later in the GC. -// See the comment in GCRuntime.h above grayBufferState for details. -class BufferGrayRootsTracer final : public GenericTracer { - // Set to false if we OOM while buffering gray roots. - bool bufferingGrayRootsFailed; - - JSObject* onObjectEdge(JSObject* obj) override { return bufferRoot(obj); } - JSString* onStringEdge(JSString* string) override { - return bufferRoot(string); - } - js::BaseScript* onScriptEdge(js::BaseScript* script) override { - return bufferRoot(script); - } - JS::Symbol* onSymbolEdge(JS::Symbol* symbol) override { - return bufferRoot(symbol); - } - JS::BigInt* onBigIntEdge(JS::BigInt* bi) override { return bufferRoot(bi); } - - js::Shape* onShapeEdge(js::Shape* shape) override { - unsupportedEdge(); - return nullptr; - } - js::BaseShape* onBaseShapeEdge(js::BaseShape* base) override { - unsupportedEdge(); - return nullptr; - } - js::GetterSetter* onGetterSetterEdge(js::GetterSetter* gs) override { - unsupportedEdge(); - return nullptr; - } - js::PropMap* onPropMapEdge(js::PropMap* map) override { - unsupportedEdge(); - return nullptr; - } - js::jit::JitCode* onJitCodeEdge(js::jit::JitCode* code) override { - unsupportedEdge(); - return nullptr; - } - js::Scope* onScopeEdge(js::Scope* scope) override { - unsupportedEdge(); - return nullptr; - } - js::RegExpShared* onRegExpSharedEdge(js::RegExpShared* shared) override { - unsupportedEdge(); - return nullptr; - } - - void unsupportedEdge() { MOZ_CRASH("Unsupported gray root edge kind"); } - - template - inline T* bufferRoot(T* thing); - - public: - explicit BufferGrayRootsTracer(JSRuntime* rt) - : GenericTracer(rt, JS::TracerKind::GrayBuffering), - bufferingGrayRootsFailed(false) {} - - bool failed() const { return bufferingGrayRootsFailed; } - void setFailed() { bufferingGrayRootsFailed = true; } -}; - -void js::gc::GCRuntime::bufferGrayRoots() { - // Precondition: the state has been reset to "unused" after the last GC - // and the zone's buffers have been cleared. - MOZ_ASSERT(grayBufferState == GrayBufferState::Unused); - for (GCZonesIter zone(this); !zone.done(); zone.next()) { - MOZ_ASSERT(zone->gcGrayRoots().IsEmpty()); - } - - BufferGrayRootsTracer grayBufferer(rt); - traceEmbeddingGrayRoots(&grayBufferer); - Compartment::traceIncomingCrossCompartmentEdgesForZoneGC( - &grayBufferer, Compartment::GrayEdges); - - // Propagate the failure flag from the marker to the runtime. - if (grayBufferer.failed()) { - grayBufferState = GrayBufferState::Failed; - resetBufferedGrayRoots(); - } else { - grayBufferState = GrayBufferState::Okay; - } -} - -template -inline T* BufferGrayRootsTracer::bufferRoot(T* thing) { - MOZ_ASSERT(JS::RuntimeHeapIsBusy()); - MOZ_ASSERT(thing); - // Check if |thing| is corrupt by calling a method that touches the heap. - MOZ_ASSERT(thing->getTraceKind() != JS::TraceKind(0xff)); - - TenuredCell* tenured = &thing->asTenured(); - - // This is run from a helper thread while the mutator is paused so we have - // to use *FromAnyThread methods here. - Zone* zone = tenured->zoneFromAnyThread(); - if (zone->isCollectingFromAnyThread()) { - // See the comment on SetMaybeAliveFlag to see why we only do this for - // objects and scripts. We rely on gray root buffering for this to work, - // but we only need to worry about uncollected dead compartments during - // incremental GCs (when we do gray root buffering). - SetMaybeAliveFlag(thing); - - if (!zone->gcGrayRoots().Append(tenured)) { - bufferingGrayRootsFailed = true; - } - } - - return thing; -} - -void GCRuntime::markBufferedGrayRoots(JS::Zone* zone) { - MOZ_ASSERT(grayBufferState == GrayBufferState::Okay); - MOZ_ASSERT(zone->isGCMarkingBlackAndGray() || zone->isGCCompacting()); - - auto& roots = zone->gcGrayRoots(); - if (roots.IsEmpty()) { - return; - } - - for (auto iter = roots.Iter(); !iter.Done(); iter.Next()) { - Cell* cell = iter.Get(); - - // Bug 1203273: Check for bad pointers on OSX and output diagnostics. -#if defined(XP_DARWIN) && defined(MOZ_DIAGNOSTIC_ASSERT_ENABLED) - auto addr = uintptr_t(cell); - if (addr < ChunkSize || addr % CellAlignBytes != 0) { - MOZ_CRASH_UNSAFE_PRINTF( - "Bad GC thing pointer in gray root buffer: %p at address %p", cell, - &iter.Get()); - } -#else - MOZ_ASSERT(IsCellPointerValid(cell)); -#endif - - TraceManuallyBarrieredGenericPointerEdge(&marker, &cell, - "buffered gray root"); - } -} - -void GCRuntime::resetBufferedGrayRoots() { - MOZ_ASSERT( - grayBufferState != GrayBufferState::Okay, - "Do not clear the gray buffers unless we are Failed or becoming Unused"); - for (GCZonesIter zone(this); !zone.done(); zone.next()) { - zone->gcGrayRoots().Clear(); - } -} - JS_PUBLIC_API void JS::AddPersistentRoot( JS::RootingContext* cx, RootKind kind, PersistentRooted* root) { diff --git a/js/src/gc/Zone.cpp b/js/src/gc/Zone.cpp index 3412eb415c89..e21cc0d17132 100644 --- a/js/src/gc/Zone.cpp +++ b/js/src/gc/Zone.cpp @@ -162,7 +162,6 @@ JS::Zone::Zone(JSRuntime* rt, Kind kind) gcWeakMapList_(this), compartments_(), crossZoneStringWrappers_(this), - gcGrayRoots_(this), weakCaches_(this), gcEphemeronEdges_(this, SystemAllocPolicy(), rt->randomHashCodeScrambler()), diff --git a/js/src/gc/Zone.h b/js/src/gc/Zone.h index ac62ceffb927..fac0914cecb0 100644 --- a/js/src/gc/Zone.h +++ b/js/src/gc/Zone.h @@ -257,12 +257,6 @@ class Zone : public js::ZoneAllocator, public js::gc::GraphNodeBase { // All cross-zone string wrappers in the zone. js::MainThreadOrGCTaskData crossZoneStringWrappers_; - // This zone's gray roots. - using GrayRootVector = - mozilla::SegmentedVector; - js::ZoneOrGCTaskData gcGrayRoots_; - // List of non-ephemeron weak containers to sweep during // beginSweepingSweepGroup. js::ZoneOrGCTaskData> weakCaches_; @@ -521,8 +515,6 @@ class Zone : public js::ZoneAllocator, public js::gc::GraphNodeBase { void sweepAllCrossCompartmentWrappers(); static void fixupAllCrossCompartmentWrappersAfterMovingGC(JSTracer* trc); - GrayRootVector& gcGrayRoots() { return gcGrayRoots_.ref(); } - mozilla::LinkedList& weakCaches() { return weakCaches_.ref(); } diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index ab14accc0a63..6e04c1f69c30 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -1350,10 +1350,6 @@ JS_PUBLIC_API void JS::ClearKeptObjects(JSContext* cx) { } } -JS_PUBLIC_API bool JS::ZoneIsCollecting(JS::Zone* zone) { - return zone->wasGCStarted(); -} - JS_PUBLIC_API bool JS::AtomsZoneIsCollecting(JSRuntime* runtime) { return runtime->activeGCInAtomsZone(); } diff --git a/js/src/jsfriendapi.h b/js/src/jsfriendapi.h index 46540b648d92..a8c1927f06a8 100644 --- a/js/src/jsfriendapi.h +++ b/js/src/jsfriendapi.h @@ -20,6 +20,16 @@ class JSJitInfo; +/* + * Set a callback used to trace gray roots. + * + * The callback is called after the first slice of GC so the embedding must + * implement appropriate barriers on its gray roots to ensure correctness. + * + * This callback may be called multiple times for different sets of zones. Use + * JS::ZoneIsGrayMarking() to determine whether roots from a particular zone are + * required. + */ extern JS_PUBLIC_API void JS_SetGrayGCRootsTracer(JSContext* cx, JSTraceDataOp traceOp, void* data); diff --git a/xpcom/base/CycleCollectedJSRuntime.cpp b/xpcom/base/CycleCollectedJSRuntime.cpp index 12f594b779e7..e7aea86995e0 100644 --- a/xpcom/base/CycleCollectedJSRuntime.cpp +++ b/xpcom/base/CycleCollectedJSRuntime.cpp @@ -491,8 +491,8 @@ inline void JSHolderMap::ForEach(F&& f, WhichHolders aWhich) { ForEach(mAnyZoneJSHolders, f, nullptr); for (auto i = mPerZoneJSHolders.modIter(); !i.done(); i.next()) { - if (aWhich == HoldersInCollectingZones && - !JS::ZoneIsCollecting(i.get().key())) { + if (aWhich == HoldersInGrayMarkingZones && + !JS::ZoneIsGrayMarking(i.get().key())) { continue; } @@ -972,7 +972,7 @@ void CycleCollectedJSRuntime::TraceGrayJS(JSTracer* aTracer, void* aData) { // Mark these roots as gray so the CC can walk them later. - JSHolderMap::WhichHolders which = JSHolderMap::HoldersInCollectingZones; + JSHolderMap::WhichHolders which = JSHolderMap::HoldersInGrayMarkingZones; if (JS::AtomsZoneIsCollecting(self->Runtime())) { // Any holder may point into the atoms zone. which = JSHolderMap::AllHolders; diff --git a/xpcom/base/CycleCollectedJSRuntime.h b/xpcom/base/CycleCollectedJSRuntime.h index 3644d0d06565..fad9ac0531bb 100644 --- a/xpcom/base/CycleCollectedJSRuntime.h +++ b/xpcom/base/CycleCollectedJSRuntime.h @@ -88,7 +88,7 @@ class IncrementalFinalizeRunnable; // SegmentedVector to speed up iteration. class JSHolderMap { public: - enum WhichHolders { AllHolders, HoldersInCollectingZones }; + enum WhichHolders { AllHolders, HoldersInGrayMarkingZones }; JSHolderMap();