Bug 1549565 - Avoid firing unmark gray read barriers during iterations where no Cell pointers escape. r=jonco
authorSteve Fink <sfink@mozilla.com>
Thu, 16 May 2019 17:19:33 +0000
changeset 474203 271c49dac12ccb1dc07467fa34d9f2c30ffeac3b
parent 474202 2c3426c424110085bff92c2ffdb2fe4372c98cca
child 474204 f460289202a100a2efec62ee77a370594111706c
push id113144
push usershindli@mozilla.com
push dateFri, 17 May 2019 16:44:55 +0000
treeherdermozilla-inbound@f4c4b796f845 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjonco
bugs1549565
milestone68.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 1549565 - Avoid firing unmark gray read barriers during iterations where no Cell pointers escape. r=jonco Differential Revision: https://phabricator.services.mozilla.com/D30103
js/src/gc/GC-inl.h
js/src/gc/GC.cpp
js/src/gc/Heap.h
js/src/gc/PrivateIterators-inl.h
js/src/gc/PublicIterators.cpp
js/src/gc/Zone.cpp
--- a/js/src/gc/GC-inl.h
+++ b/js/src/gc/GC-inl.h
@@ -72,29 +72,23 @@ class ArenaIter {
     if (!arena) {
       arena = unsweptArena;
       unsweptArena = sweptArena;
       sweptArena = nullptr;
     }
   }
 };
 
-enum CellIterNeedsBarrier : uint8_t {
-  CellIterDoesntNeedBarrier = 0,
-  CellIterMayNeedBarrier = 1
-};
-
-class ArenaCellIterImpl {
+class ArenaCellIter {
   size_t firstThingOffset;
   size_t thingSize;
   Arena* arenaAddr;
   FreeSpan span;
   uint_fast16_t thing;
   JS::TraceKind traceKind;
-  bool needsBarrier;
   mozilla::DebugOnly<bool> initialized;
 
   // Upon entry, |thing| points to any thing (free or used) and finds the
   // first used thing, which may be |thing|.
   void moveForwardIfFree() {
     MOZ_ASSERT(!done());
     MOZ_ASSERT(thing);
     // Note: if |span| is empty, this test will fail, which is what we want
@@ -103,39 +97,37 @@ class ArenaCellIterImpl {
     // never need to move forward.
     if (thing == span.first) {
       thing = span.last + thingSize;
       span = *span.nextSpan(arenaAddr);
     }
   }
 
  public:
-  ArenaCellIterImpl()
+  ArenaCellIter()
       : firstThingOffset(0),
         thingSize(0),
         arenaAddr(nullptr),
         thing(0),
         traceKind(JS::TraceKind::Null),
-        needsBarrier(false),
         initialized(false) {}
 
-  explicit ArenaCellIterImpl(Arena* arena, CellIterNeedsBarrier mayNeedBarrier)
+  explicit ArenaCellIter(Arena* arena)
       : initialized(false) {
-    init(arena, mayNeedBarrier);
+    init(arena);
   }
 
-  void init(Arena* arena, CellIterNeedsBarrier mayNeedBarrier) {
+  void init(Arena* arena) {
     MOZ_ASSERT(!initialized);
     MOZ_ASSERT(arena);
     initialized = true;
     AllocKind kind = arena->getAllocKind();
     firstThingOffset = Arena::firstThingOffset(kind);
     thingSize = Arena::thingSize(kind);
     traceKind = MapAllocToTraceKind(kind);
-    needsBarrier = mayNeedBarrier && !JS::RuntimeHeapIsCollecting();
     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);
@@ -148,27 +140,17 @@ class ArenaCellIterImpl {
   bool done() const {
     MOZ_ASSERT(initialized);
     MOZ_ASSERT(thing <= ArenaSize);
     return thing == ArenaSize;
   }
 
   TenuredCell* getCell() const {
     MOZ_ASSERT(!done());
-    TenuredCell* cell =
-        reinterpret_cast<TenuredCell*>(uintptr_t(arenaAddr) + thing);
-
-    // This can result in a a new reference being created to an object that
-    // an ongoing incremental GC may find to be unreachable, so we may need
-    // a barrier here.
-    if (needsBarrier) {
-      ExposeGCThingToActiveJS(JS::GCCellPtr(cell, traceKind));
-    }
-
-    return cell;
+    return reinterpret_cast<TenuredCell*>(uintptr_t(arenaAddr) + thing);
   }
 
   template <typename T>
   T* get() const {
     MOZ_ASSERT(!done());
     MOZ_ASSERT(JS::MapTypeToTraceKind<T>::kind == traceKind);
     return reinterpret_cast<T*>(getCell());
   }
@@ -178,33 +160,28 @@ class ArenaCellIterImpl {
     thing += thingSize;
     if (thing < ArenaSize) {
       moveForwardIfFree();
     }
   }
 };
 
 template <>
-JSObject* ArenaCellIterImpl::get<JSObject>() const;
-
-class ArenaCellIter : public ArenaCellIterImpl {
- public:
-  explicit ArenaCellIter(Arena* arena)
-      : ArenaCellIterImpl(arena, CellIterMayNeedBarrier) {
-    MOZ_ASSERT(JS::RuntimeHeapIsTracing());
-  }
-};
+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;
-  ArenaCellIterImpl cellIter;
+  ArenaCellIter cellIter;
   mozilla::Maybe<JS::AutoAssertNoGC> nogc;
 
  protected:
   // For use when a subclass wants to insert some setup before init().
   ZoneAllCellIter() {}
 
   void init(JS::Zone* zone, AllocKind kind) {
     MOZ_ASSERT_IF(IsNurseryAllocable(kind),
@@ -228,17 +205,17 @@ class ZoneAllCellIter<TenuredCell> {
     // 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(), CellIterMayNeedBarrier);
+      cellIter.init(arenaIter.get());
       settle();
     }
   }
 
  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.
@@ -320,16 +297,20 @@ class ZoneAllCellIter<TenuredCell> {
 // nursery (again). You may select a variant that will skip the eviction either
 // by specializing on a GCType that is never allocated in the nursery, or
 // explicitly by passing in a trailing AutoAssertEmptyNursery argument.
 //
 // NOTE: This class can return items that are about to be swept/finalized.
 //       You must not keep pointers to such items across GCs.  Use
 //       ZoneCellIter below to filter these out.
 //
+// NOTE: This class also does not read barrier returned items, so may return
+//       gray cells. You must not store such items anywhere on the heap without
+//       gray-unmarking them. Use ZoneCellIter to automatically unmark them.
+//
 /* clang-format on */
 template <typename GCType>
 class ZoneAllCellIter : public ZoneAllCellIter<TenuredCell> {
  public:
   // Non-nursery allocated (equivalent to having an entry in
   // MapTypeToFinalizeKind). The template declaration here is to discard this
   // constructor overload if MapTypeToFinalizeKind<GCType>::kind does not
   // exist. Note that there will be no remaining overloads that will work,
@@ -360,18 +341,22 @@ class ZoneAllCellIter : public ZoneAllCe
       : ZoneAllCellIter<TenuredCell>(zone, kind, empty) {}
 
   GCType* get() const { return ZoneAllCellIter<TenuredCell>::get<GCType>(); }
   operator GCType*() const { return get(); }
   GCType* operator->() const { return get(); }
 };
 
 // Like the above class but filter out cells that are about to be finalized.
+// Also, read barrier all cells returned (unless the Unbarriered variants are
+// used) to prevent gray cells from escaping.
 template <typename T>
-class ZoneCellIter : public ZoneAllCellIter<T> {
+class ZoneCellIter : protected ZoneAllCellIter<T> {
+  using Base = ZoneAllCellIter<T>;
+
  public:
   /*
    * The same constructors as above.
    */
   explicit ZoneCellIter(JS::Zone* zone) : ZoneAllCellIter<T>(zone) {
     skipDying();
   }
   ZoneCellIter(JS::Zone* zone, const js::gc::AutoAssertEmptyNursery& empty)
@@ -383,21 +368,47 @@ class ZoneCellIter : public ZoneAllCellI
     skipDying();
   }
   ZoneCellIter(JS::Zone* zone, AllocKind kind,
                const js::gc::AutoAssertEmptyNursery& empty)
       : ZoneAllCellIter<T>(zone, kind, empty) {
     skipDying();
   }
 
+  using Base::done;
+
   void next() {
     ZoneAllCellIter<T>::next();
     skipDying();
   }
 
+  TenuredCell* getCell() const {
+    TenuredCell* cell = Base::getCell();
+
+    // This can result in a new reference being created to an object that an
+    // ongoing incremental GC may find to be unreachable, so we may need a
+    // barrier here.
+    JSRuntime* rt = cell->runtimeFromAnyThread();
+    if (!JS::RuntimeHeapIsCollecting(rt->heapState())) {
+      JS::TraceKind traceKind = JS::MapTypeToTraceKind<T>::kind;
+      ExposeGCThingToActiveJS(JS::GCCellPtr(cell, traceKind));
+    }
+
+    return cell;
+  }
+
+  T* get() const {
+    return reinterpret_cast<T*>(getCell());
+  }
+
+  TenuredCell* unbarrieredGetCell() const { return Base::getCell(); }
+  T* unbarrieredGet() const { return Base::get(); }
+  operator T*() const { return get(); }
+  T* operator->() const { return get(); }
+
  private:
   void skipDying() {
     while (!ZoneAllCellIter<T>::done()) {
       T* current = ZoneAllCellIter<T>::get();
       if (!IsAboutToBeFinalizedUnbarriered(&current)) {
         return;
       }
       ZoneAllCellIter<T>::next();
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -519,22 +519,16 @@ static constexpr FinalizePhase Backgroun
     {gcstats::PhaseKind::SWEEP_STRING,
      {AllocKind::FAT_INLINE_STRING, AllocKind::STRING,
       AllocKind::EXTERNAL_STRING, AllocKind::FAT_INLINE_ATOM, AllocKind::ATOM,
       AllocKind::SYMBOL, AllocKind::BIGINT}},
     {gcstats::PhaseKind::SWEEP_SHAPE,
      {AllocKind::SHAPE, AllocKind::ACCESSOR_SHAPE, AllocKind::BASE_SHAPE,
       AllocKind::OBJECT_GROUP}}};
 
-template <>
-JSObject* ArenaCellIterImpl::get<JSObject>() const {
-  MOZ_ASSERT(!done());
-  return reinterpret_cast<JSObject*>(getCell());
-}
-
 void Arena::unmarkAll() {
   uintptr_t* word = chunk()->bitmap.arenaBits(this);
   memset(word, 0, ArenaBitmapWords * sizeof(uintptr_t));
 }
 
 void Arena::unmarkPreMarkedFreeCells() {
   for (ArenaFreeCellIter iter(this); !iter.done(); iter.next()) {
     TenuredCell* cell = iter.getCell();
@@ -8151,17 +8145,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 (ArenaCellIterUnbarriered iter(arena); !iter.done(); iter.next()) {
+        for (ArenaCellIter iter(arena); !iter.done(); iter.next()) {
           TenuredCell* cell = iter.getCell();
           MOZ_ASSERT(!cell->isMarkedAny());
           cell->markBlack();
         }
       }
     }
   }
 
@@ -8516,17 +8510,17 @@ void js::gc::CheckHashTablesAfterMovingG
    */
   rt->geckoProfiler().checkStringsMapAfterMovingGC();
   for (ZonesIter zone(rt, SkipAtoms); !zone.done(); zone.next()) {
     zone->checkUniqueIdTableAfterMovingGC();
     zone->checkInitialShapesTableAfterMovingGC();
     zone->checkBaseShapeTableAfterMovingGC();
 
     JS::AutoCheckCannotGC nogc;
-    for (auto baseShape = zone->cellIter<BaseShape>(); !baseShape.done();
+    for (auto baseShape = zone->cellIterUnsafe<BaseShape>(); !baseShape.done();
          baseShape.next()) {
       ShapeCachePtr p = baseShape->getCache(nogc);
       p.checkAfterMovingGC();
     }
   }
 
   for (CompartmentsIter c(rt); !c.done(); c.next()) {
     c->checkWrapperMapAfterMovingGC();
--- a/js/src/gc/Heap.h
+++ b/js/src/gc/Heap.h
@@ -77,17 +77,17 @@ const size_t ArenaBitmapWords =
  * - In a non-empty span, |first| is the address of the first free thing in the
  *   span, and |last| is the address of the last free thing in the span.
  *   Furthermore, the memory pointed to by |last| holds a FreeSpan structure
  *   that points to the next span (which may be empty); this works because
  *   sizeof(FreeSpan) is less than the smallest thingSize.
  */
 class FreeSpan {
   friend class Arena;
-  friend class ArenaCellIterImpl;
+  friend class ArenaCellIter;
   friend class ArenaFreeCellIter;
 
   uint16_t first;
   uint16_t last;
 
  public:
   // This inits just |first| and |last|; if the span is non-empty it doesn't
   // do anything with the next span stored at |last|.
--- a/js/src/gc/PrivateIterators-inl.h
+++ b/js/src/gc/PrivateIterators-inl.h
@@ -13,38 +13,32 @@
 
 #include "gc/PublicIterators.h"
 
 #include "gc/GC-inl.h"
 
 namespace js {
 namespace gc {
 
-class ArenaCellIterUnderGC : public ArenaCellIterImpl {
+class ArenaCellIterUnderGC : public ArenaCellIter {
  public:
   explicit ArenaCellIterUnderGC(Arena* arena)
-      : ArenaCellIterImpl(arena, CellIterDoesntNeedBarrier) {
+      : ArenaCellIter(arena) {
     MOZ_ASSERT(CurrentThreadIsPerformingGC());
   }
 };
 
-class ArenaCellIterUnderFinalize : public ArenaCellIterImpl {
+class ArenaCellIterUnderFinalize : public ArenaCellIter {
  public:
   explicit ArenaCellIterUnderFinalize(Arena* arena)
-      : ArenaCellIterImpl(arena, CellIterDoesntNeedBarrier) {
+      : ArenaCellIter(arena) {
     MOZ_ASSERT(CurrentThreadIsGCSweeping());
   }
 };
 
-class ArenaCellIterUnbarriered : public ArenaCellIterImpl {
- public:
-  explicit ArenaCellIterUnbarriered(Arena* arena)
-      : ArenaCellIterImpl(arena, CellIterDoesntNeedBarrier) {}
-};
-
 class GrayObjectIter : public ZoneAllCellIter<js::gc::TenuredCell> {
  public:
   explicit GrayObjectIter(JS::Zone* zone, AllocKind kind)
       : ZoneAllCellIter<js::gc::TenuredCell>() {
     initForTenuredIteration(zone, kind);
   }
 
   JSObject* get() const {
@@ -128,17 +122,17 @@ class SweepGroupZonesIter {
   JS::Zone* operator->() const { return get(); }
 };
 
 using SweepGroupCompartmentsIter =
     CompartmentsOrRealmsIterT<SweepGroupZonesIter, CompartmentsInZoneIter>;
 using SweepGroupRealmsIter =
     CompartmentsOrRealmsIterT<SweepGroupZonesIter, RealmsInZoneIter>;
 
-// Iterate the free cells in an arena. See also ArenaCellIterImpl which iterates
+// Iterate the free cells in an arena. See also ArenaCellIter which iterates
 // the allocated cells.
 class ArenaFreeCellIter {
   Arena* arena;
   size_t thingSize;
   FreeSpan span;
   uint_fast16_t thing;
 
  public:
--- a/js/src/gc/PublicIterators.cpp
+++ b/js/src/gc/PublicIterators.cpp
@@ -32,17 +32,17 @@ 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 (ArenaCellIterUnbarriered iter(arena); !iter.done(); iter.next()) {
+      for (ArenaCellIter iter(arena); !iter.done(); iter.next()) {
         (*cellCallback)(cx->runtime(), data, iter.getCell(), traceKind,
                         thingSize);
       }
     }
   }
 }
 
 void js::IterateHeapUnbarriered(JSContext* cx, void* data,
--- a/js/src/gc/Zone.cpp
+++ b/js/src/gc/Zone.cpp
@@ -205,17 +205,17 @@ void Zone::discardJitCode(FreeOp* fop,
   if (isPreservingCode()) {
     return;
   }
 
   if (discardBaselineCode || releaseTypes) {
 #ifdef DEBUG
     // Assert no TypeScripts are marked as active.
     for (auto script = cellIter<JSScript>(); !script.done(); script.next()) {
-      if (TypeScript* types = script->types()) {
+      if (TypeScript* types = script.unbarrieredGet()->types()) {
         MOZ_ASSERT(!types->active());
       }
     }
 #endif
 
     // Mark TypeScripts on the stack as active.
     jit::MarkActiveTypeScripts(this);
   }