From 6632830564eb1cd26625e97f4d254e8b2bd843c8 Mon Sep 17 00:00:00 2001 From: Steve Fink Date: Thu, 28 Nov 2019 00:49:37 +0000 Subject: [PATCH] Bug 1597206 - Refactor GCMarker state management r=jonco Differential Revision: https://phabricator.services.mozilla.com/D54579 --HG-- extra : source : e372ae10adb5880afcf820a51ff3bdd559269b76 --- js/public/TracingAPI.h | 13 +---------- js/src/gc/GC.cpp | 2 +- js/src/gc/GCMarker.h | 33 ++++++++++++++++++++++------ js/src/gc/Marking.cpp | 49 +++++++++++++++++++++--------------------- 4 files changed, 52 insertions(+), 45 deletions(-) diff --git a/js/public/TracingAPI.h b/js/public/TracingAPI.h index 471aae6f1f49..fc962f2b3674 100644 --- a/js/public/TracingAPI.h +++ b/js/public/TracingAPI.h @@ -69,12 +69,6 @@ class JS_PUBLIC_API JSTracer { // everything reachable by regular edges has been marked. Marking, - // Same as Marking, except we have now moved on to the "weak marking - // phase", in which every marked obj/script is immediately looked up to - // see if it is a weak map key (and therefore might require marking its - // weak map value). - WeakMarking, - // A tracer that traverses the graph for the purposes of moving objects // from the nursery to the tenured area. Tenuring, @@ -83,12 +77,7 @@ class JS_PUBLIC_API JSTracer { // Traversing children is the responsibility of the callback. Callback }; - bool isMarkingTracer() const { - return tag_ == TracerKindTag::Marking || tag_ == TracerKindTag::WeakMarking; - } - bool isWeakMarkingTracer() const { - return tag_ == TracerKindTag::WeakMarking; - } + bool isMarkingTracer() const { return tag_ == TracerKindTag::Marking; } bool isTenuringTracer() const { return tag_ == TracerKindTag::Tenuring; } bool isCallbackTracer() const { return tag_ == TracerKindTag::Callback; } inline JS::CallbackTracer* asCallbackTracer(); diff --git a/js/src/gc/GC.cpp b/js/src/gc/GC.cpp index ee6c63ccdf5a..d9ba23e6f6bc 100644 --- a/js/src/gc/GC.cpp +++ b/js/src/gc/GC.cpp @@ -4171,7 +4171,7 @@ void GCRuntime::markWeakReferences(gcstats::PhaseKind phase) { for (;;) { bool markedAny = false; - if (!marker.isWeakMarkingTracer()) { + if (!marker.isWeakMarking()) { for (ZoneIterT zone(this); !zone.done(); zone.next()) { markedAny |= WeakMapBase::markZoneIteratively(zone, &marker); } diff --git a/js/src/gc/GCMarker.h b/js/src/gc/GCMarker.h index 5993e4434829..79202d0406e1 100644 --- a/js/src/gc/GCMarker.h +++ b/js/src/gc/GCMarker.h @@ -235,6 +235,26 @@ class MarkStackIter { } /* namespace gc */ +enum MarkingState : uint8_t { + // Have not yet started marking. + NotActive, + + // Main marking mode. Weakmap marking will be populating the weakKeys tables + // but not consulting them. The state will transition to WeakMarking until it + // is done, then back to RegularMarking. + RegularMarking, + + // Same as RegularMarking except now every marked obj/script is immediately + // looked up in the weakKeys table to see if it is a weakmap key, and + // therefore might require marking its value. Transitions back to + // RegularMarking when done. + WeakMarking, + + // Same as RegularMarking, but we OOMed (or obeyed a directive in the test + // marking queue) and fell back to iterating until the next GC. + IterativeMarking +}; + class GCMarker : public JSTracer { public: explicit GCMarker(JSRuntime* rt); @@ -288,7 +308,7 @@ class GCMarker : public JSTracer { void leaveWeakMarkingMode(); void abortLinearWeakMarking() { leaveWeakMarkingMode(); - linearWeakMarkingDisabled_ = true; + state = MarkingState::IterativeMarking; } void delayMarkingChildren(gc::Cell* cell); @@ -330,6 +350,8 @@ class GCMarker : public JSTracer { template void markImplicitEdges(T* oldThing); + bool isWeakMarking() const { return state == MarkingState::WeakMarking; } + private: #ifdef DEBUG void checkZone(void* p); @@ -436,15 +458,12 @@ class GCMarker : public JSTracer { /* Whether more work has been added to the delayed marking list. */ MainThreadOrGCTaskData delayedMarkingWorkAdded; - /* - * If the weakKeys table OOMs, disable the linear algorithm and fall back - * to iterating until the next GC. - */ - MainThreadOrGCTaskData linearWeakMarkingDisabled_; - /* The count of marked objects during GC. */ size_t markCount; + /* Track the state of marking. */ + MainThreadOrGCTaskData state; + #ifdef DEBUG /* Count of arenas that are currently in the stack. */ MainThreadOrGCTaskData markLaterArenas; diff --git a/js/src/gc/Marking.cpp b/js/src/gc/Marking.cpp index d9cc4ac210b6..2fecb12f488b 100644 --- a/js/src/gc/Marking.cpp +++ b/js/src/gc/Marking.cpp @@ -653,7 +653,7 @@ void GCMarker::markEphemeronValues(gc::Cell* markedCell, template void GCMarker::markImplicitEdgesHelper(T markedThing) { - if (!isWeakMarkingTracer()) { + if (!isWeakMarking()) { return; } @@ -1593,7 +1593,7 @@ GCMarker::MarkQueueProgress GCMarker::processMarkQueue() { return QueueYielded; } else if (js::StringEqualsLiteral(str, "enter-weak-marking-mode") || js::StringEqualsLiteral(str, "abort-weak-marking-mode")) { - if (!isWeakMarkingTracer() && !linearWeakMarkingDisabled_) { + if (state == MarkingState::RegularMarking) { // We can't enter weak marking mode at just any time, so instead // we'll stop processing the queue and continue on with the GC. Once // we enter weak marking mode, we can continue to the rest of the @@ -2430,11 +2430,11 @@ GCMarker::GCMarker(JSRuntime* rt) color(MarkColor::Black), mainStackColor(MarkColor::Black), delayedMarkingList(nullptr), - delayedMarkingWorkAdded(false) + delayedMarkingWorkAdded(false), + state(MarkingState::NotActive) #ifdef DEBUG , markLaterArenas(0), - started(false), strictCompartmentChecking(false), markQueue(rt), queuePos(0) @@ -2448,12 +2448,9 @@ bool GCMarker::init(JSGCMode gcMode) { } void GCMarker::start() { -#ifdef DEBUG - MOZ_ASSERT(!started); - started = true; -#endif + MOZ_ASSERT(state == MarkingState::NotActive); + state = MarkingState::RegularMarking; color = MarkColor::Black; - linearWeakMarkingDisabled_ = false; #ifdef DEBUG queuePos = 0; @@ -2465,15 +2462,11 @@ void GCMarker::start() { } void GCMarker::stop() { -#ifdef DEBUG MOZ_ASSERT(isDrained()); - - MOZ_ASSERT(started); - started = false; - MOZ_ASSERT(!delayedMarkingList); MOZ_ASSERT(markLaterArenas == 0); -#endif + MOZ_ASSERT(state != MarkingState::NotActive); + state = MarkingState::NotActive; stack.clear(); auxStack.clear(); @@ -2582,13 +2575,13 @@ void GCMarker::repush(JSObject* obj) { void GCMarker::enterWeakMarkingMode() { MOZ_ASSERT(runtime()->gc.nursery().isEmpty()); - MOZ_ASSERT(tag_ == TracerKindTag::Marking); - if (linearWeakMarkingDisabled_) { + MOZ_ASSERT(isMarkingTracer()); + if (state != MarkingState::RegularMarking) { return; } if (weakMapAction() != ExpandWeakMaps) { - return; + return; // This marker does not do linear-time weak marking. } // During weak marking mode, we maintain a table mapping weak keys to @@ -2598,7 +2591,10 @@ void GCMarker::enterWeakMarkingMode() { // weakmap marking, this initialization step will become unnecessary, as // the table will already hold all such keys.) - tag_ = TracerKindTag::WeakMarking; + // Set state before doing anything else, so any new key that is marked + // during the following gcWeakKeys scan will itself be looked up in + // gcWeakKeys and marked according to ephemeron rules. + state = MarkingState::WeakMarking; // If there was an 'enter-weak-marking-mode' token in the queue, then it // and everything after it will still be in the queue so we can process @@ -2616,11 +2612,6 @@ void GCMarker::enterWeakMarkingMode() { } void GCMarker::leaveWeakMarkingMode() { - MOZ_ASSERT_IF( - weakMapAction() == ExpandWeakMaps && !linearWeakMarkingDisabled_, - tag_ == TracerKindTag::WeakMarking); - tag_ = TracerKindTag::Marking; - // Table is expensive to maintain when not in weak marking mode, so we'll // rebuild it upon entry rather than allow it to contain stale data. AutoEnterOOMUnsafeRegion oomUnsafe; @@ -2629,6 +2620,14 @@ void GCMarker::leaveWeakMarkingMode() { oomUnsafe.crash("clearing weak keys in GCMarker::leaveWeakMarkingMode()"); } } + + MOZ_ASSERT_IF(weakMapAction() == ExpandWeakMaps, + state == MarkingState::WeakMarking || + state == MarkingState::IterativeMarking); + + if (state != MarkingState::IterativeMarking) { + state = MarkingState::RegularMarking; + } } void GCMarker::delayMarkingChildren(Cell* cell) { @@ -2780,7 +2779,7 @@ void gc::PushArena(GCMarker* gcmarker, Arena* arena) { #ifdef DEBUG void GCMarker::checkZone(void* p) { - MOZ_ASSERT(started); + MOZ_ASSERT(state != MarkingState::NotActive); DebugOnly cell = static_cast(p); MOZ_ASSERT_IF(cell->isTenured(), cell->asTenured().zone()->isCollectingFromAnyThread());