From 24b1a0f166bf0f661165aef67f3a799bb9e57287 Mon Sep 17 00:00:00 2001 From: Moonchild Date: Tue, 27 Jun 2023 18:39:24 +0200 Subject: [PATCH] Avoid TLS lookups when checking if zones need to be marked for GC. This converts the fromPointer() calls to an object cast to (hopefully) avoid the GC crashes. Logic is reversed from MustSkipMarking to ShouldMark with appropriate renames. --- js/src/gc/Marking.cpp | 48 +++++++++++++++++++++--------------------- js/src/gc/Zone.h | 8 +++---- js/src/jsgc.cpp | 22 +++++++++---------- js/src/jsweakmap.cpp | 4 ++-- js/src/vm/Debugger.cpp | 4 ++-- 5 files changed, 42 insertions(+), 44 deletions(-) diff --git a/js/src/gc/Marking.cpp b/js/src/gc/Marking.cpp index 058c495a77..bf40f0bdda 100644 --- a/js/src/gc/Marking.cpp +++ b/js/src/gc/Marking.cpp @@ -308,7 +308,7 @@ ShouldMarkCrossCompartment(JSTracer* trc, JSObject* src, Cell* cell) MOZ_ASSERT(!zone->isCollecting()); trc->runtime()->gc.setFoundBlackGrayEdges(tenured); } - return zone->isGCMarking(); + return zone->shouldMarkInZone(); } else { if (zone->isGCMarkingBlack()) { /* @@ -331,26 +331,26 @@ ShouldMarkCrossCompartment(JSTracer* trc, JSObject* src, const Value& val) } static void -AssertZoneIsMarking(Cell* thing) +AssertShouldMarkInZone(Cell* thing) { - MOZ_ASSERT(TenuredCell::fromPointer(thing)->zone()->isGCMarking()); + MOZ_ASSERT(thing->asTenured().zone()->shouldMarkInZone()); } static void -AssertZoneIsMarking(JSString* str) +AssertShouldMarkInZone(JSString* str) { #ifdef DEBUG - Zone* zone = TenuredCell::fromPointer(str)->zone(); - MOZ_ASSERT(zone->isGCMarking() || zone->isAtomsZone()); + Zone* zone = str->asTenured().zone(); + MOZ_ASSERT(zone->shouldMarkInZone() || zone->isAtomsZone()); #endif } static void -AssertZoneIsMarking(JS::Symbol* sym) +AssertShouldMarkInZone(JS::Symbol* sym) { #ifdef DEBUG - Zone* zone = TenuredCell::fromPointer(sym)->zone(); - MOZ_ASSERT(zone->isGCMarking() || zone->isAtomsZone()); + Zone* zone = sym->asTenured().zone(); + MOZ_ASSERT(zone->shouldMarkInZone() || zone->isAtomsZone()); #endif } @@ -759,35 +759,35 @@ GCMarker::markImplicitEdges(T* thing) template static inline bool -MustSkipMarking(GCMarker* gcmarker, T thing) +ShouldMark(GCMarker* gcmarker, T thing) { // Don't trace things that are owned by another runtime. if (IsOwnedByOtherRuntime(gcmarker->runtime(), thing)) - return true; + return false; // Don't mark things outside a zone if we are in a per-zone GC. - return !thing->zone()->isGCMarking(); + return thing->zone()->shouldMarkInZone(); } template <> bool -MustSkipMarking(GCMarker* gcmarker, JSObject* obj) +ShouldMark(GCMarker* gcmarker, JSObject* obj) { // Don't trace things that are owned by another runtime. if (IsOwnedByOtherRuntime(gcmarker->runtime(), obj)) - return true; + return false; // We may mark a Nursery thing outside the context of the // MinorCollectionTracer because of a pre-barrier. The pre-barrier is not // needed in this case because we perform a minor collection before each // incremental slice. if (IsInsideNursery(obj)) - return true; + return false; // Don't mark things outside a zone if we are in a per-zone GC. It is // faster to check our own arena, which we can do since we know that // the object is tenured. - return !TenuredCell::fromPointer(obj)->zone()->isGCMarking(); + return obj->asTenured().zone()->shouldMarkInZone(); } template @@ -795,7 +795,7 @@ void DoMarking(GCMarker* gcmarker, T* thing) { // Do per-type marking precondition checks. - if (MustSkipMarking(gcmarker, thing)) + if (!ShouldMark(gcmarker, thing)) return; CheckTracedThing(gcmarker, thing); @@ -822,7 +822,7 @@ void NoteWeakEdge(GCMarker* gcmarker, T** thingp) { // Do per-type marking precondition checks. - if (MustSkipMarking(gcmarker, *thingp)) + if (!ShouldMark(gcmarker, *thingp)) return; CheckTracedThing(gcmarker, *thingp); @@ -971,7 +971,7 @@ template bool js::GCMarker::mark(T* thing) { - AssertZoneIsMarking(thing); + AssertShouldMarkInZone(thing); MOZ_ASSERT(!IsInsideNursery(gc::TenuredCell::fromPointer(thing))); return gc::ParticipatesInCC::value ? gc::TenuredCell::fromPointer(thing)->markIfUnmarked(markColor()) @@ -1106,7 +1106,7 @@ JSString::traceBase(JSTracer* trc) inline void js::GCMarker::eagerlyMarkChildren(JSLinearString* linearStr) { - AssertZoneIsMarking(linearStr); + AssertShouldMarkInZone(linearStr); MOZ_ASSERT(linearStr->isMarked()); MOZ_ASSERT(linearStr->JSString::isLinear()); @@ -1116,7 +1116,7 @@ js::GCMarker::eagerlyMarkChildren(JSLinearString* linearStr) MOZ_ASSERT(linearStr->JSString::isLinear()); if (linearStr->isPermanentAtom()) break; - AssertZoneIsMarking(linearStr); + AssertShouldMarkInZone(linearStr); if (!mark(static_cast(linearStr))) break; } @@ -1169,7 +1169,7 @@ js::GCMarker::eagerlyMarkChildren(JSRope* rope) JS_DIAGNOSTICS_ASSERT(rope->getTraceKind() == JS::TraceKind::String); JS_DIAGNOSTICS_ASSERT(rope->JSString::isRope()); - AssertZoneIsMarking(rope); + AssertShouldMarkInZone(rope); MOZ_ASSERT(rope->isMarked()); JSRope* next = nullptr; @@ -1649,7 +1649,7 @@ GCMarker::processMarkStackTop(SliceBudget& budget) case ObjectTag: { obj = reinterpret_cast(addr); - AssertZoneIsMarking(obj); + AssertShouldMarkInZone(obj); goto scan_obj; } @@ -1712,7 +1712,7 @@ GCMarker::processMarkStackTop(SliceBudget& budget) scan_obj: { - AssertZoneIsMarking(obj); + AssertShouldMarkInZone(obj); budget.step(); if (budget.isOverBudget()) { diff --git a/js/src/gc/Zone.h b/js/src/gc/Zone.h index eedd5455a1..323aa27758 100644 --- a/js/src/gc/Zone.h +++ b/js/src/gc/Zone.h @@ -232,11 +232,9 @@ struct Zone : public JS::shadow::Zone, return rt->isHeapMajorCollecting() && !rt->gc.isHeapCompacting() && gcState_ != NoGC; } - bool isGCMarking() { - if (runtimeFromMainThread()->isHeapCollecting()) - return gcState_ == Mark || gcState_ == MarkGray; - else - return needsIncrementalBarrier(); + bool shouldMarkInZone() const { + return needsIncrementalBarrier() || + (gcState_ == Mark || gcState_ == MarkGray); } GCState gcState() const { return gcState_; } diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp index 575c075532..e172ea36c2 100644 --- a/js/src/jsgc.cpp +++ b/js/src/jsgc.cpp @@ -3952,7 +3952,7 @@ struct AddOutgoingEdgeFunctor { */ if (needsEdge_) { JS::Zone* zone = other.zone(); - if (zone->isGCMarking()) + if (zone->shouldMarkInZone()) finder_.addEdgeTo(zone); } } @@ -3983,14 +3983,14 @@ Zone::findOutgoingEdges(ZoneComponentFinder& finder) */ JSRuntime* rt = runtimeFromMainThread(); Zone* atomsZone = rt->atomsCompartment(finder.lock)->zone(); - if (atomsZone->isGCMarking()) + if (atomsZone->shouldMarkInZone()) finder.addEdgeTo(atomsZone); for (CompartmentsInZoneIter comp(this); !comp.done(); comp.next()) comp->findOutgoingEdges(finder); for (ZoneSet::Range r = gcZoneGroupEdges.all(); !r.empty(); r.popFront()) { - if (r.front()->isGCMarking()) + if (r.front()->shouldMarkInZone()) finder.addEdgeTo(r.front()); } @@ -4032,7 +4032,7 @@ GCRuntime::findZoneGroups(AutoLockForExclusiveAccess& lock) finder.useOneComponent(); for (GCZonesIter zone(rt); !zone.done(); zone.next()) { - MOZ_ASSERT(zone->isGCMarking()); + MOZ_ASSERT(zone->shouldMarkInZone()); finder.addNode(zone); } zoneGroups = finder.getResultsList(); @@ -4045,7 +4045,7 @@ GCRuntime::findZoneGroups(AutoLockForExclusiveAccess& lock) #ifdef DEBUG for (Zone* head = currentZoneGroup; head; head = head->nextGroup()) { for (Zone* zone = head; zone; zone = zone->nextNodeInGroup()) - MOZ_ASSERT(zone->isGCMarking()); + MOZ_ASSERT(zone->shouldMarkInZone()); } MOZ_ASSERT_IF(!isIncremental, !currentZoneGroup->nextGroup()); @@ -4068,7 +4068,7 @@ GCRuntime::getNextZoneGroup() } for (Zone* zone = currentZoneGroup; zone; zone = zone->nextNodeInGroup()) { - MOZ_ASSERT(zone->isGCMarking()); + MOZ_ASSERT(zone->shouldMarkInZone()); MOZ_ASSERT(!zone->isQueuedForBackgroundSweep()); } @@ -4079,7 +4079,7 @@ GCRuntime::getNextZoneGroup() MOZ_ASSERT(!isIncremental); for (GCZoneGroupIter zone(rt); !zone.done(); zone.next()) { MOZ_ASSERT(!zone->gcNextGraphComponent); - MOZ_ASSERT(zone->isGCMarking()); + MOZ_ASSERT(zone->shouldMarkInZone()); zone->setNeedsIncrementalBarrier(false, Zone::UpdateJit); zone->setGCState(Zone::NoGC); zone->gcGrayRoots.clearAndFree(); @@ -4522,7 +4522,7 @@ GCRuntime::beginSweepingZoneGroup(AutoLockForExclusiveAccess& lock) bool sweepingAtoms = false; for (GCZoneGroupIter zone(rt); !zone.done(); zone.next()) { /* Set the GC state to sweeping. */ - MOZ_ASSERT(zone->isGCMarking()); + MOZ_ASSERT(zone->shouldMarkInZone()); zone->setGCState(Zone::Sweep); /* Purge the ArenaLists before sweeping. */ @@ -5231,7 +5231,7 @@ GCRuntime::resetIncrementalGC(gc::AbortReason reason, AutoLockForExclusiveAccess ResetGrayList(c); for (GCZonesIter zone(rt); !zone.done(); zone.next()) { - MOZ_ASSERT(zone->isGCMarking()); + MOZ_ASSERT(zone->shouldMarkInZone()); zone->setNeedsIncrementalBarrier(false, Zone::UpdateJit); zone->setGCState(Zone::NoGC); } @@ -5355,7 +5355,7 @@ AutoGCSlice::AutoGCSlice(JSRuntime* rt) * is expensive) because Ion code doesn't run during GC. If need be, * we'll update the Ion barriers in ~AutoGCSlice. */ - if (zone->isGCMarking()) { + if (zone->shouldMarkInZone()) { MOZ_ASSERT(zone->needsIncrementalBarrier()); zone->setNeedsIncrementalBarrier(false, Zone::DontUpdateJit); } else { @@ -5368,7 +5368,7 @@ AutoGCSlice::~AutoGCSlice() { /* We can't use GCZonesIter if this is the end of the last slice. */ for (ZonesIter zone(runtime, WithAtoms); !zone.done(); zone.next()) { - if (zone->isGCMarking()) { + if (zone->shouldMarkInZone()) { zone->setNeedsIncrementalBarrier(true, Zone::UpdateJit); zone->arenas.purge(); } else { diff --git a/js/src/jsweakmap.cpp b/js/src/jsweakmap.cpp index 3b4579d652..03d1a0847e 100644 --- a/js/src/jsweakmap.cpp +++ b/js/src/jsweakmap.cpp @@ -121,7 +121,7 @@ WeakMapBase::restoreMarkedWeakMaps(WeakMapSet& markedWeakMaps) { for (WeakMapSet::Range r = markedWeakMaps.all(); !r.empty(); r.popFront()) { WeakMapBase* map = r.front(); - MOZ_ASSERT(map->zone->isGCMarking()); + MOZ_ASSERT(map->zone->shouldMarkInZone()); MOZ_ASSERT(!map->marked); map->marked = true; } @@ -144,7 +144,7 @@ ObjectValueMap::findZoneEdges() if (!delegate) continue; Zone* delegateZone = delegate->zone(); - if (delegateZone == zone || !delegateZone->isGCMarking()) + if (delegateZone == zone || !delegateZone->shouldMarkInZone()) continue; if (!delegateZone->gcZoneGroupEdges.put(key->zone())) return false; diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp index f844d1d482..6ef46388eb 100644 --- a/js/src/vm/Debugger.cpp +++ b/js/src/vm/Debugger.cpp @@ -2989,7 +2989,7 @@ Debugger::markAllIteratively(GCMarker* trc) * - it actually has hooks that might be called */ GCPtrNativeObject& dbgobj = dbg->toJSObjectRef(); - if (!dbgobj->zone()->isGCMarking()) + if (!dbgobj->zone()->shouldMarkInZone()) continue; bool dbgMarked = IsMarked(rt, &dbgobj); @@ -3139,7 +3139,7 @@ Debugger::findZoneEdges(Zone* zone, js::gc::ZoneComponentFinder& finder) */ for (Debugger* dbg : zone->runtimeFromMainThread()->debuggerList) { Zone* w = dbg->object->zone(); - if (w == zone || !w->isGCMarking()) + if (w == zone || !w->shouldMarkInZone()) continue; if (dbg->debuggeeZones.has(zone) || dbg->scripts.hasKeyInZone(zone) ||