Bug 1647319 - Use NestedIterator to implement ZoneAllCellIter r=sfink
☠☠ backed out by 17e8b5e9f8f9 ☠ ☠
authorJon Coppeard <jcoppeard@mozilla.com>
Tue, 23 Jun 2020 08:31:59 +0000
changeset 600841 61916f49f2b3c38a26a8a48b597c7434e6b3cc59
parent 600840 32b62f886374909e479a29c345ff4b238da09192
child 600842 cf488a06921ec04de214013170daf0aca6a83ff6
push id13310
push userffxbld-merge
push dateMon, 29 Jun 2020 14:50:06 +0000
treeherdermozilla-beta@15a59a0afa5c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1647319
milestone79.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1647319 - Use NestedIterator to implement ZoneAllCellIter r=sfink This also means we can simplify ArenaCellIter is it doesn't need to support reset() any more. I had to rename the getCell/get methods returning TenuredCell*/T* to get/as to make this work. I also changed use of |ArenaCellIter i| to |ArenaCellIter cell|, like we do for ZonesIter. Differential Revision: https://phabricator.services.mozilla.com/D80486
js/src/gc/GC-inl.h
js/src/gc/GC.cpp
js/src/gc/IteratorUtils.h
js/src/gc/Marking.cpp
js/src/gc/PublicIterators.cpp
--- a/js/src/gc/GC-inl.h
+++ b/js/src/gc/GC-inl.h
@@ -7,16 +7,17 @@
 #ifndef gc_GC_inl_h
 #define gc_GC_inl_h
 
 #include "gc/GC.h"
 
 #include "mozilla/DebugOnly.h"
 #include "mozilla/Maybe.h"
 
+#include "gc/IteratorUtils.h"
 #include "gc/Zone.h"
 #include "vm/Runtime.h"
 
 #include "gc/ArenaList-inl.h"
 
 namespace js {
 namespace gc {
 
@@ -78,109 +79,84 @@ class ArenaIter {
 };
 
 class ArenaCellIter {
   size_t firstThingOffset;
   size_t thingSize;
   Arena* arenaAddr;
   FreeSpan span;
   uint_fast16_t thing;
+
+#ifdef DEBUG
   JS::TraceKind traceKind;
-  mozilla::DebugOnly<bool> initialized;
+#endif
 
   // Upon entry, |thing| points to any thing (free or used) and finds the
   // first used thing, which may be |thing|.
-  void moveForwardIfFree() {
+  void settle() {
     MOZ_ASSERT(!done());
     MOZ_ASSERT(thing);
     // Note: if |span| is empty, this test will fail, which is what we want
     // -- |span| being empty means that we're past the end of the last free
     // thing, all the remaining things in the arena are used, and we'll
     // never need to move forward.
     if (thing == span.first) {
       thing = span.last + thingSize;
       span = *span.nextSpan(arenaAddr);
     }
   }
 
  public:
-  ArenaCellIter()
-      : firstThingOffset(0),
-        thingSize(0),
-        arenaAddr(nullptr),
-        thing(0),
-        traceKind(JS::TraceKind::Null),
-        initialized(false) {
-    span.initAsEmpty();
-  }
-
-  explicit ArenaCellIter(Arena* arena) : initialized(false) { init(arena); }
-
-  void init(Arena* arena) {
-    MOZ_ASSERT(!initialized);
+  explicit ArenaCellIter(Arena* arena) {
     MOZ_ASSERT(arena);
-    initialized = true;
     AllocKind kind = arena->getAllocKind();
     firstThingOffset = Arena::firstThingOffset(kind);
     thingSize = Arena::thingSize(kind);
     traceKind = MapAllocToTraceKind(kind);
-    reset(arena);
-  }
-
-  // Use this to move from an Arena of a particular kind to another Arena of
-  // the same kind.
-  void reset(Arena* arena) {
-    MOZ_ASSERT(initialized);
-    MOZ_ASSERT(arena);
     arenaAddr = arena;
     span = *arena->getFirstFreeSpan();
     thing = firstThingOffset;
-    moveForwardIfFree();
+    settle();
   }
 
   bool done() const {
-    MOZ_ASSERT(initialized);
     MOZ_ASSERT(thing <= ArenaSize);
     return thing == ArenaSize;
   }
 
-  TenuredCell* getCell() const {
+  TenuredCell* get() const {
     MOZ_ASSERT(!done());
     return reinterpret_cast<TenuredCell*>(uintptr_t(arenaAddr) + thing);
   }
 
   template <typename T>
-  T* get() const {
+  T* as() const {
     MOZ_ASSERT(!done());
     MOZ_ASSERT(JS::MapTypeToTraceKind<T>::kind == traceKind);
-    return reinterpret_cast<T*>(getCell());
+    return reinterpret_cast<T*>(get());
   }
 
   void next() {
     MOZ_ASSERT(!done());
     thing += thingSize;
     if (thing < ArenaSize) {
-      moveForwardIfFree();
+      settle();
     }
   }
+
+  operator TenuredCell*() const { return get(); }
+  TenuredCell* operator->() const { return get(); }
 };
 
-template <>
-inline JSObject* ArenaCellIter::get<JSObject>() const {
-  MOZ_ASSERT(!done());
-  return reinterpret_cast<JSObject*>(getCell());
-}
-
 template <typename T>
 class ZoneAllCellIter;
 
 template <>
 class ZoneAllCellIter<TenuredCell> {
-  ArenaIter arenaIter;
-  ArenaCellIter cellIter;
+  mozilla::Maybe<NestedIterator<ArenaIter, ArenaCellIter>> iter;
   mozilla::Maybe<JS::AutoAssertNoGC> nogc;
 
  protected:
   // For use when a subclass wants to insert some setup before init().
   ZoneAllCellIter() = default;
 
   void init(JS::Zone* zone, AllocKind kind) {
     MOZ_ASSERT_IF(IsNurseryAllocable(kind),
@@ -202,21 +178,17 @@ class ZoneAllCellIter<TenuredCell> {
     // We have a single-threaded runtime, so there's no need to protect
     // against other threads iterating or allocating. However, we do have
     // background finalization; we may have to wait for this to finish if
     // it's currently active.
     if (IsBackgroundFinalized(kind) &&
         zone->arenas.needBackgroundFinalizeWait(kind)) {
       rt->gc.waitBackgroundSweepEnd();
     }
-    arenaIter.init(zone, kind);
-    if (!arenaIter.done()) {
-      cellIter.init(arenaIter.get());
-      settle();
-    }
+    iter.emplace(zone, kind);
   }
 
  public:
   ZoneAllCellIter(JS::Zone* zone, AllocKind kind) {
     // If we are iterating a nursery-allocated kind then we need to
     // evict first so that we can see all things.
     if (IsNurseryAllocable(kind)) {
       zone->runtimeFromMainThread()->gc.evictNursery();
@@ -227,43 +199,26 @@ class ZoneAllCellIter<TenuredCell> {
 
   ZoneAllCellIter(JS::Zone* zone, AllocKind kind,
                   const js::gc::AutoAssertEmptyNursery&) {
     // No need to evict the nursery. (This constructor is known statically
     // to not GC.)
     init(zone, kind);
   }
 
-  bool done() const { return arenaIter.done(); }
+  bool done() const { return iter->done(); }
 
   template <typename T>
   T* get() const {
-    MOZ_ASSERT(!done());
-    return cellIter.get<T>();
-  }
-
-  TenuredCell* getCell() const {
-    MOZ_ASSERT(!done());
-    return cellIter.getCell();
+    return iter->ref().as<T>();
   }
 
-  void settle() {
-    while (cellIter.done() && !arenaIter.done()) {
-      arenaIter.next();
-      if (!arenaIter.done()) {
-        cellIter.reset(arenaIter.get());
-      }
-    }
-  }
+  TenuredCell* getCell() const { return iter->get(); }
 
-  void next() {
-    MOZ_ASSERT(!done());
-    cellIter.next();
-    settle();
-  }
+  void next() { iter->next(); }
 };
 
 /* clang-format off */
 //
 // Iterator over the cells in a Zone, where the GC type (JSString, JSObject) is
 // known, for a single AllocKind. Example usages:
 //
 //   for (auto obj = zone->cellIter<JSObject>(AllocKind::OBJECT0); !obj.done(); obj.next()) {
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -465,18 +465,18 @@ inline size_t Arena::finalize(JSFreeOp* 
   uint_fast16_t firstThing = firstThingOffset(thingKind);
   uint_fast16_t firstThingOrSuccessorOfLastMarkedThing = firstThing;
   uint_fast16_t lastThing = ArenaSize - thingSize;
 
   FreeSpan newListHead;
   FreeSpan* newListTail = &newListHead;
   size_t nmarked = 0;
 
-  for (ArenaCellIterUnderFinalize i(this); !i.done(); i.next()) {
-    T* t = i.get<T>();
+  for (ArenaCellIterUnderFinalize cell(this); !cell.done(); cell.next()) {
+    T* t = cell.as<T>();
     if (t->asTenured().isMarkedAny()) {
       uint_fast16_t thing = uintptr_t(t) & ArenaMask;
       if (thing != firstThingOrSuccessorOfLastMarkedThing) {
         // We just finished passing over one or more free things,
         // so record a new FreeSpan.
         newListTail->initBounds(firstThingOrSuccessorOfLastMarkedThing,
                                 thing - thingSize, this);
         newListTail = newListTail->nextSpanUnchecked(this);
@@ -1940,24 +1940,24 @@ static void RelocateArena(Arena* arena, 
   MOZ_ASSERT(!arena->onDelayedMarkingList());
   MOZ_ASSERT(arena->bufferedCells()->isEmpty());
 
   Zone* zone = arena->zone;
 
   AllocKind thingKind = arena->getAllocKind();
   size_t thingSize = arena->getThingSize();
 
-  for (ArenaCellIterUnderGC i(arena); !i.done(); i.next()) {
-    RelocateCell(zone, i.getCell(), thingKind, thingSize);
+  for (ArenaCellIterUnderGC cell(arena); !cell.done(); cell.next()) {
+    RelocateCell(zone, cell, thingKind, thingSize);
     sliceBudget.step();
   }
 
 #ifdef DEBUG
-  for (ArenaCellIterUnderGC i(arena); !i.done(); i.next()) {
-    TenuredCell* src = i.getCell();
+  for (ArenaCellIterUnderGC cell(arena); !cell.done(); cell.next()) {
+    TenuredCell* src = cell;
     MOZ_ASSERT(src->isForwarded());
     TenuredCell* dest = Forwarded(src);
     MOZ_ASSERT(src->isMarkedBlack() == dest->isMarkedBlack());
     MOZ_ASSERT(src->isMarkedGray() == dest->isMarkedGray());
   }
 #endif
 }
 
@@ -2204,18 +2204,18 @@ static inline void UpdateCellPointers(Mo
   MOZ_ASSERT(!cell->isForwarded());
 
   cell->fixupAfterMovingGC();
   cell->traceChildren(trc);
 }
 
 template <typename T>
 static void UpdateArenaPointersTyped(MovingTracer* trc, Arena* arena) {
-  for (ArenaCellIterUnderGC i(arena); !i.done(); i.next()) {
-    UpdateCellPointers(trc, reinterpret_cast<T*>(i.getCell()));
+  for (ArenaCellIterUnderGC cell(arena); !cell.done(); cell.next()) {
+    UpdateCellPointers(trc, cell.as<T>());
   }
 }
 
 static bool CanUpdateKindInBackground(AllocKind kind) {
   // We try to update as many GC things in parallel as we can, but there are
   // kinds for which this might not be safe:
   //  - we assume JSObjects that are foreground finalized are not safe to
   //    update in parallel
@@ -5656,18 +5656,18 @@ static void SweepThing(JSFreeOp* fop, Ob
 }
 
 template <typename T>
 static bool SweepArenaList(JSFreeOp* fop, Arena** arenasToSweep,
                            SliceBudget& sliceBudget) {
   while (Arena* arena = *arenasToSweep) {
     MOZ_ASSERT(arena->zone->isGCSweeping());
 
-    for (ArenaCellIterUnderGC i(arena); !i.done(); i.next()) {
-      SweepThing(fop, i.get<T>());
+    for (ArenaCellIterUnderGC cell(arena); !cell.done(); cell.next()) {
+      SweepThing(fop, cell.as<T>());
     }
 
     Arena* next = arena->next;
     MOZ_ASSERT_IF(next, next->zone == arena->zone);
     *arenasToSweep = next;
 
     AllocKind kind = MapTypeToFinalizeKind<T>::kind;
     sliceBudget.step(Arena::thingsPerArena(kind));
@@ -7895,18 +7895,17 @@ void GCRuntime::mergeRealms(Realm* sourc
     for (ArenaIter aiter(source->zone(), thingKind); !aiter.done();
          aiter.next()) {
       Arena* arena = aiter.get();
       arena->zone = target->zone();
       if (MOZ_UNLIKELY(targetZoneIsCollecting)) {
         // If we are currently collecting the target zone then we must
         // treat all merged things as if they were allocated during the
         // collection.
-        for (ArenaCellIter iter(arena); !iter.done(); iter.next()) {
-          TenuredCell* cell = iter.getCell();
+        for (ArenaCellIter cell(arena); !cell.done(); cell.next()) {
           MOZ_ASSERT(!cell->isMarkedAny());
           cell->markBlack();
         }
       }
     }
   }
 
   // The source should be the only realm in its zone.
--- a/js/src/gc/IteratorUtils.h
+++ b/js/src/gc/IteratorUtils.h
@@ -41,16 +41,18 @@ class NestedIterator {
     b->next();
     if (b->done()) {
       b.reset();
       a.next();
       settle();
     }
   }
 
+  const IteratorB& ref() const { return *b; }
+
   operator T() const { return get(); }
 
   T operator->() const { return get(); }
 
  private:
   void settle() {
     MOZ_ASSERT(b.isNothing());
     while (!a.done()) {
--- a/js/src/gc/Marking.cpp
+++ b/js/src/gc/Marking.cpp
@@ -2854,20 +2854,19 @@ void GCMarker::delayMarkingChildren(Cell
   }
 }
 
 void GCMarker::markDelayedChildren(Arena* arena, MarkColor color) {
   JS::TraceKind kind = MapAllocToTraceKind(arena->getAllocKind());
   MOZ_ASSERT_IF(color == MarkColor::Gray, TraceKindCanBeMarkedGray(kind));
 
   AutoSetMarkColor setColor(*this, color);
-  for (ArenaCellIterUnderGC i(arena); !i.done(); i.next()) {
-    TenuredCell* t = i.getCell();
-    if (t->isMarked(color)) {
-      js::TraceChildren(this, t, kind);
+  for (ArenaCellIterUnderGC cell(arena); !cell.done(); cell.next()) {
+    if (cell->isMarked(color)) {
+      js::TraceChildren(this, cell, kind);
     }
   }
 }
 
 /*
  * Process arenas from |delayedMarkingList| by marking the unmarked children of
  * marked cells of color |color|. Return early if the |budget| is exceeded.
  *
--- a/js/src/gc/PublicIterators.cpp
+++ b/js/src/gc/PublicIterators.cpp
@@ -32,19 +32,19 @@ static void IterateRealmsArenasCellsUnba
 
   for (auto thingKind : AllAllocKinds()) {
     JS::TraceKind traceKind = MapAllocToTraceKind(thingKind);
     size_t thingSize = Arena::thingSize(thingKind);
 
     for (ArenaIter aiter(zone, thingKind); !aiter.done(); aiter.next()) {
       Arena* arena = aiter.get();
       (*arenaCallback)(cx->runtime(), data, arena, traceKind, thingSize);
-      for (ArenaCellIter iter(arena); !iter.done(); iter.next()) {
-        (*cellCallback)(cx->runtime(), data,
-                        JS::GCCellPtr(iter.getCell(), traceKind), thingSize);
+      for (ArenaCellIter cell(arena); !cell.done(); cell.next()) {
+        (*cellCallback)(cx->runtime(), data, JS::GCCellPtr(cell, traceKind),
+                        thingSize);
       }
     }
   }
 }
 
 void js::IterateHeapUnbarriered(JSContext* cx, void* data,
                                 IterateZoneCallback zoneCallback,
                                 JS::IterateRealmCallback realmCallback,