Bug 1322420 - Expose cells found via iteration r=sfink a=abillings
authorJon Coppeard <jcoppeard@mozilla.com>
Fri, 10 Feb 2017 10:22:38 +0000
changeset 342264 4e919ac282a4b42906d8695a0e1671e8ec2a044c
parent 342263 104807b2a841742a6e999cd7d81caaf1e9eb06e6
child 342265 147e70a78d1a2d9bdc2dc5f3af039611a3a6d42c
push id31347
push userkwierso@gmail.com
push dateFri, 10 Feb 2017 23:23:41 +0000
treeherdermozilla-central@855e6b2f6199 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink, abillings
bugs1322420
milestone54.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 1322420 - Expose cells found via iteration r=sfink a=abillings
js/public/GCAPI.h
js/src/jscompartment.cpp
js/src/jsgc.cpp
js/src/jsgcinlines.h
--- a/js/public/GCAPI.h
+++ b/js/public/GCAPI.h
@@ -624,18 +624,16 @@ namespace js {
 namespace gc {
 
 extern JS_FRIEND_API(bool)
 BarriersAreAllowedOnCurrentThread();
 
 static MOZ_ALWAYS_INLINE void
 ExposeGCThingToActiveJS(JS::GCCellPtr thing)
 {
-    MOZ_ASSERT(thing.kind() != JS::TraceKind::Shape);
-
     // GC things residing in the nursery cannot be gray: they have no mark bits.
     // All live objects in the nursery are moved to tenured at the beginning of
     // each GC slice, so the gray marker never sees nursery things.
     if (IsInsideNursery(thing.asCell()))
         return;
 
     // There's nothing to do for permanent GC things that might be owned by
     // another runtime.
--- a/js/src/jscompartment.cpp
+++ b/js/src/jscompartment.cpp
@@ -1052,25 +1052,16 @@ AddLazyFunctionsForCompartment(JSContext
         // are about to be finalized. GC things referenced by objects that are
         // about to be finalized (e.g., in slots) may already be freed.
         if (gc::IsAboutToBeFinalizedUnbarriered(&fun) ||
             fun->compartment() != cx->compartment())
         {
             continue;
         }
 
-        // This creates a new reference to an object that an ongoing incremental
-        // GC may find to be unreachable. Treat as if we're reading a weak
-        // reference and trigger the read barrier.
-        if (cx->zone()->needsIncrementalBarrier())
-            fun->readBarrier(fun);
-
-        // TODO: The above checks should be rolled into the cell iterator (see
-        // bug 1322971).
-
         if (fun->isInterpretedLazy()) {
             LazyScript* lazy = fun->lazyScriptOrNull();
             if (lazy && lazy->sourceObject() && !lazy->hasUncompiledEnclosingScript()) {
                 if (!lazyFunctions.append(fun))
                     return false;
             }
         }
     }
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -3555,18 +3555,18 @@ ArenaLists::checkEmptyArenaList(AllocKin
 #ifdef DEBUG
     if (!arenaLists(kind).isEmpty()) {
         size_t max_cells = 20;
         char *env = getenv("JS_GC_MAX_LIVE_CELLS");
         if (env && *env)
             max_cells = atol(env);
         for (Arena* current = arenaLists(kind).head(); current; current = current->next) {
             for (ArenaCellIterUnderGC i(current); !i.done(); i.next()) {
-                Cell* t = i.get<Cell>();
-                MOZ_ASSERT(t->asTenured().isMarked(), "unmarked cells should have been finalized");
+                TenuredCell* t = i.getCell();
+                MOZ_ASSERT(t->isMarked(), "unmarked cells should have been finalized");
                 if (++num_live <= max_cells) {
                     fprintf(stderr, "ERROR: GC found live Cell %p of kind %s at shutdown\n",
                             t, AllocKindToAscii(kind));
                 }
             }
         }
         fprintf(stderr, "ERROR: GC found %" PRIuSIZE " live Cells at shutdown\n", num_live);
     }
--- a/js/src/jsgcinlines.h
+++ b/js/src/jsgcinlines.h
@@ -97,23 +97,31 @@ class ArenaIter
         if (!arena) {
             arena = unsweptArena;
             unsweptArena = sweptArena;
             sweptArena = nullptr;
         }
     }
 };
 
+enum CellIterNeedsBarrier : uint8_t
+{
+    CellIterDoesntNeedBarrier = 0,
+    CellIterMayNeedBarrier = 1
+};
+
 class ArenaCellIterImpl
 {
     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
@@ -123,27 +131,42 @@ class ArenaCellIterImpl
         if (thing == span.first) {
             thing = span.last + thingSize;
             span = *span.nextSpan(arenaAddr);
         }
     }
 
   public:
     ArenaCellIterImpl()
-      : firstThingOffset(0), thingSize(0), arenaAddr(nullptr), thing(0), initialized(false) {}
+      : firstThingOffset(0),
+        thingSize(0),
+        arenaAddr(nullptr),
+        thing(0),
+        traceKind(JS::TraceKind::Null),
+        needsBarrier(false),
+        initialized(false)
+    {}
 
-    explicit ArenaCellIterImpl(Arena* arena) : initialized(false) { init(arena); }
+    explicit ArenaCellIterImpl(Arena* arena, CellIterNeedsBarrier mayNeedBarrier)
+      : initialized(false)
+    {
+        init(arena, mayNeedBarrier);
+    }
 
-    void init(Arena* arena) {
+    void init(Arena* arena, CellIterNeedsBarrier mayNeedBarrier) {
         MOZ_ASSERT(!initialized);
         MOZ_ASSERT(arena);
+        MOZ_ASSERT_IF(!mayNeedBarrier,
+                      CurrentThreadIsPerformingGC() || CurrentThreadIsGCSweeping());
         initialized = true;
         AllocKind kind = arena->getAllocKind();
         firstThingOffset = Arena::firstThingOffset(kind);
         thingSize = Arena::thingSize(kind);
+        traceKind = MapAllocToTraceKind(kind);
+        needsBarrier = mayNeedBarrier && !JS::CurrentThreadIsHeapCollecting();
         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);
@@ -156,21 +179,30 @@ class ArenaCellIterImpl
     bool done() const {
         MOZ_ASSERT(initialized);
         MOZ_ASSERT(thing <= ArenaSize);
         return thing == ArenaSize;
     }
 
     TenuredCell* getCell() const {
         MOZ_ASSERT(!done());
-        return reinterpret_cast<TenuredCell*>(uintptr_t(arenaAddr) + thing);
+        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;
     }
 
     template<typename T> T* get() const {
         MOZ_ASSERT(!done());
+        MOZ_ASSERT(JS::MapTypeToTraceKind<T>::kind == traceKind);
         return static_cast<T*>(getCell());
     }
 
     void next() {
         MOZ_ASSERT(!done());
         thing += thingSize;
         if (thing < ArenaSize)
             moveForwardIfFree();
@@ -180,37 +212,37 @@ class ArenaCellIterImpl
 template<>
 JSObject*
 ArenaCellIterImpl::get<JSObject>() const;
 
 class ArenaCellIter : public ArenaCellIterImpl
 {
   public:
     explicit ArenaCellIter(Arena* arena)
-      : ArenaCellIterImpl(arena)
+      : ArenaCellIterImpl(arena, CellIterMayNeedBarrier)
     {
         MOZ_ASSERT(JS::CurrentThreadIsHeapTracing());
     }
 };
 
 class ArenaCellIterUnderGC : public ArenaCellIterImpl
 {
   public:
     explicit ArenaCellIterUnderGC(Arena* arena)
-      : ArenaCellIterImpl(arena)
+      : ArenaCellIterImpl(arena, CellIterDoesntNeedBarrier)
     {
         MOZ_ASSERT(CurrentThreadIsPerformingGC());
     }
 };
 
 class ArenaCellIterUnderFinalize : public ArenaCellIterImpl
 {
   public:
     explicit ArenaCellIterUnderFinalize(Arena* arena)
-      : ArenaCellIterImpl(arena)
+      : ArenaCellIterImpl(arena, CellIterDoesntNeedBarrier)
     {
         MOZ_ASSERT(CurrentThreadIsGCSweeping());
     }
 };
 
 template <typename T>
 class ZoneCellIter;
 
@@ -243,17 +275,17 @@ class ZoneCellIter<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());
+            cellIter.init(arenaIter.get(), CellIterMayNeedBarrier);
     }
 
   public:
     ZoneCellIter(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->group()->evictNursery();