Bug 1597206 - Refactor GCMarker state management r=jonco

Differential Revision: https://phabricator.services.mozilla.com/D54579

--HG--
extra : source : e372ae10adb5880afcf820a51ff3bdd559269b76

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();

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);
       }

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 <typename T>
   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<bool> delayedMarkingWorkAdded;
 
-  /*
-   * If the weakKeys table OOMs, disable the linear algorithm and fall back
-   * to iterating until the next GC.
-   */
-  MainThreadOrGCTaskData<bool> linearWeakMarkingDisabled_;
-
   /* The count of marked objects during GC. */
   size_t markCount;
 
+  /* Track the state of marking. */
+  MainThreadOrGCTaskData<MarkingState> state;
+
 #ifdef DEBUG
   /* Count of arenas that are currently in the stack. */
   MainThreadOrGCTaskData<size_t> markLaterArenas;

js/src/gc/Marking.cpp

@@ -653,7 +653,7 @@ void GCMarker::markEphemeronValues(gc::Cell* markedCell,
 
 template <typename T>
 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*> cell = static_cast<Cell*>(p);
   MOZ_ASSERT_IF(cell->isTenured(),
                 cell->asTenured().zone()->isCollectingFromAnyThread());