Bug 1510145 - Refactor GC resets and ensure the store buffer is always empty when we start sweeping. r=pbone, a=dveditz DEVEDITION_65_0b4_BUILD1 DEVEDITION_65_0b4_RELEASE FENNEC_65_0b4_BUILD1 FENNEC_65_0b4_RELEASE FIREFOX_65_0b4_BUILD1 FIREFOX_65_0b4_RELEASE
authorJon Coppeard <jcoppeard@mozilla.com>
Mon, 03 Dec 2018 17:17:34 -0500
changeset 506177 b1216967cdd01665e807bea431a7b18d9ac169e2
parent 506176 100268636829cc2ed0f5618d130335f86bad6311
child 506178 be9114cafd5dded5f22b902b85ea2b91bacb6339
push id10319
push userryanvm@gmail.com
push dateTue, 11 Dec 2018 22:33:37 +0000
treeherdermozilla-beta@b1216967cdd0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbone, dveditz
bugs1510145
milestone65.0
Bug 1510145 - Refactor GC resets and ensure the store buffer is always empty when we start sweeping. r=pbone, a=dveditz
js/src/gc/GC.cpp
js/src/gc/GCRuntime.h
js/src/gc/StoreBuffer.cpp
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -6685,25 +6685,29 @@ AutoHeapSession::~AutoHeapSession() {
   runtime->heapState_ = prevState;
 }
 
 JS_PUBLIC_API JS::HeapState JS::RuntimeHeapState() {
   return TlsContext.get()->runtime()->heapState();
 }
 
 GCRuntime::IncrementalResult GCRuntime::resetIncrementalGC(
-    gc::AbortReason reason, AutoGCSession& session) {
-  MOZ_ASSERT(reason != gc::AbortReason::None);
+    gc::AbortReason reason) {
+  if (incrementalState == State::NotActive) {
+    return IncrementalResult::Ok;
+  }
+
+  minorGC(JS::gcreason::RESET, gcstats::PhaseKind::EVICT_NURSERY_FOR_MAJOR_GC);
+
+  AutoGCSession session(rt, JS::HeapState::MajorCollecting);
 
   switch (incrementalState) {
     case State::NotActive:
-      return IncrementalResult::Ok;
-
     case State::MarkRoots:
-      MOZ_CRASH("resetIncrementalGC did not expect MarkRoots state");
+      MOZ_CRASH("Unexpected GC state in resetIncrementalGC");
       break;
 
     case State::Mark: {
       /* Cancel any ongoing marking. */
       marker.reset();
       marker.stop();
       clearBufferedGrayRoots();
 
@@ -6885,16 +6889,18 @@ static bool ShouldCleanUpEverything(JS::
 }
 
 GCRuntime::IncrementalResult GCRuntime::incrementalSlice(
     SliceBudget& budget, JS::gcreason::Reason reason, AutoGCSession& session) {
   AutoDisableBarriers disableBarriers(rt);
 
   bool destroyingRuntime = (reason == JS::gcreason::DESTROY_RUNTIME);
 
+  number++;
+
   initialState = incrementalState;
 
 #ifdef JS_GC_ZEAL
   /*
    * Do the incremental collection type specified by zeal mode if the
    * collection was triggered by runDebugGC() and incremental GC has not been
    * cancelled by resetIncrementalGC().
    */
@@ -6930,16 +6936,17 @@ GCRuntime::IncrementalResult GCRuntime::
      * the budget is not used.
      */
     stats().writeLogMessage("Using unlimited budget for two-slice zeal mode");
     budget.makeUnlimited();
   }
 
   switch (incrementalState) {
     case State::NotActive:
+      incMajorGcNumber();
       initialReason = reason;
       cleanUpEverything = ShouldCleanUpEverything(reason, invocationKind);
       isCompacting = shouldCompact();
       MOZ_ASSERT(!lastMarkSlice);
       rootsRemoved = false;
 
       incrementalState = State::MarkRoots;
 
@@ -7005,41 +7012,25 @@ GCRuntime::IncrementalResult GCRuntime::
              !(useZeal && hasZealMode(ZealMode::YieldBeforeMarking))) ||
             (useZeal && hasZealMode(ZealMode::YieldBeforeSweeping))) {
           lastMarkSlice = true;
           stats().writeLogMessage("Yielding before starting sweeping");
           break;
         }
       }
 
-      /*
-       * If the nursery is not-empty we need to collect it before sweeping.
-       *
-       * This can happen regardless of 'isIncremental' since the GC may have
-       * been incremental when it started and later made a decision to do
-       * non-incremental collection.
-       *
-       * It's important to check this after the above case since this one
-       * wont give the mutator a chance to run.
-       */
-      if (!nursery().isEmpty()) {
-        lastMarkSlice = true;
-        stats().writeLogMessage(
-            "returning to collect the nursery before sweeping");
-        return IncrementalResult::ReturnToEvictNursery;
-      }
-
       incrementalState = State::Sweep;
       lastMarkSlice = false;
       beginSweepPhase(reason, session);
 
       MOZ_FALLTHROUGH;
 
     case State::Sweep:
       MOZ_ASSERT(nursery().isEmpty());
+      storeBuffer().checkEmpty();
 
       AutoGCRooter::traceAllWrappers(rt->mainContextFromOwnThread(), &marker);
 
       if (performSweepActions(budget) == NotFinished) {
         break;
       }
 
       endSweepPhase(destroyingRuntime);
@@ -7079,16 +7070,19 @@ GCRuntime::IncrementalResult GCRuntime::
       // Always yield before compacting since it is not incremental.
       if (isCompacting && !budget.isUnlimited()) {
         break;
       }
 
       MOZ_FALLTHROUGH;
 
     case State::Compact:
+      MOZ_ASSERT(nursery().isEmpty());
+      storeBuffer().checkEmpty();
+
       if (isCompacting) {
         MOZ_ASSERT(nursery().isEmpty());
         if (!startedCompacting) {
           beginCompactPhase();
         }
 
         if (compactPhase(reason, budget, session) == NotFinished) {
           break;
@@ -7152,53 +7146,52 @@ static inline void CheckZoneIsScheduled(
             zone->isGCScheduled() ? " scheduled" : "");
   }
   fflush(stderr);
   MOZ_CRASH("Zone not scheduled");
 #endif
 }
 
 GCRuntime::IncrementalResult GCRuntime::budgetIncrementalGC(
-    bool nonincrementalByAPI, JS::gcreason::Reason reason, SliceBudget& budget,
-    AutoGCSession& session) {
+    bool nonincrementalByAPI, JS::gcreason::Reason reason,
+    SliceBudget& budget) {
   if (nonincrementalByAPI) {
     stats().nonincremental(gc::AbortReason::NonIncrementalRequested);
     budget.makeUnlimited();
 
     // Reset any in progress incremental GC if this was triggered via the
     // API. This isn't required for correctness, but sometimes during tests
     // the caller expects this GC to collect certain objects, and we need
     // to make sure to collect everything possible.
     if (reason != JS::gcreason::ALLOC_TRIGGER) {
-      return resetIncrementalGC(gc::AbortReason::NonIncrementalRequested,
-                                session);
+      return resetIncrementalGC(gc::AbortReason::NonIncrementalRequested);
     }
 
     return IncrementalResult::Ok;
   }
 
   if (reason == JS::gcreason::ABORT_GC) {
     budget.makeUnlimited();
     stats().nonincremental(gc::AbortReason::AbortRequested);
-    return resetIncrementalGC(gc::AbortReason::AbortRequested, session);
+    return resetIncrementalGC(gc::AbortReason::AbortRequested);
   }
 
   AbortReason unsafeReason = IsIncrementalGCUnsafe(rt);
   if (unsafeReason == AbortReason::None) {
     if (reason == JS::gcreason::COMPARTMENT_REVIVED) {
       unsafeReason = gc::AbortReason::CompartmentRevived;
     } else if (mode != JSGC_MODE_INCREMENTAL) {
       unsafeReason = gc::AbortReason::ModeChange;
     }
   }
 
   if (unsafeReason != AbortReason::None) {
     budget.makeUnlimited();
     stats().nonincremental(unsafeReason);
-    return resetIncrementalGC(unsafeReason, session);
+    return resetIncrementalGC(unsafeReason);
   }
 
   if (mallocCounter.shouldTriggerGC(tunables) == NonIncrementalTrigger) {
     budget.makeUnlimited();
     stats().nonincremental(AbortReason::MallocBytesTrigger);
   }
 
   bool reset = false;
@@ -7221,17 +7214,17 @@ GCRuntime::IncrementalResult GCRuntime::
 
     if (isIncrementalGCInProgress() &&
         zone->isGCScheduled() != zone->wasGCStarted()) {
       reset = true;
     }
   }
 
   if (reset) {
-    return resetIncrementalGC(AbortReason::ZoneChange, session);
+    return resetIncrementalGC(AbortReason::ZoneChange);
   }
 
   return IncrementalResult::Ok;
 }
 
 namespace {
 
 class AutoScheduleZonesForGC {
@@ -7321,42 +7314,45 @@ void GCRuntime::maybeCallGCCallback(JSGC
 /*
  * We disable inlining to ensure that the bottom of the stack with possible GC
  * roots recorded in MarkRuntime excludes any pointers we use during the marking
  * implementation.
  */
 MOZ_NEVER_INLINE GCRuntime::IncrementalResult GCRuntime::gcCycle(
     bool nonincrementalByAPI, SliceBudget& budget,
     JS::gcreason::Reason reason) {
+  // Assert if this is a GC unsafe region.
+  rt->mainContextFromOwnThread()->verifyIsSafeToGC();
+
+  // It's ok if threads other than the main thread have suppressGC set, as
+  // they are operating on zones which will not be collected from here.
+  MOZ_ASSERT(!rt->mainContextFromOwnThread()->suppressGC);
+
   // Note that GC callbacks are allowed to re-enter GC.
   AutoCallGCCallbacks callCallbacks(*this);
 
   gcstats::AutoGCSlice agc(stats(), scanZonesBeforeGC(), invocationKind, budget,
                            reason);
 
+  auto result = budgetIncrementalGC(nonincrementalByAPI, reason, budget);
+
+  // If an ongoing incremental GC was reset, we may need to restart.
+  if (result == IncrementalResult::ResetIncremental) {
+    MOZ_ASSERT(!isIncrementalGCInProgress());
+    return result;
+  }
+
   if (shouldCollectNurseryForSlice(nonincrementalByAPI, budget)) {
     minorGC(reason, gcstats::PhaseKind::EVICT_NURSERY_FOR_MAJOR_GC);
   }
 
   AutoGCSession session(rt, JS::HeapState::MajorCollecting);
 
   majorGCTriggerReason = JS::gcreason::NO_REASON;
 
-  number++;
-  if (!isIncrementalGCInProgress()) {
-    incMajorGcNumber();
-  }
-
-  // It's ok if threads other than the main thread have suppressGC set, as
-  // they are operating on zones which will not be collected from here.
-  MOZ_ASSERT(!rt->mainContextFromOwnThread()->suppressGC);
-
-  // Assert if this is a GC unsafe region.
-  rt->mainContextFromOwnThread()->verifyIsSafeToGC();
-
   {
     gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::WAIT_BACKGROUND_THREAD);
 
     // Background finalization and decommit are finished by defininition
     // before we can start a new GC session.
     if (!isIncrementalGCInProgress()) {
       assertBackgroundSweepingFinished();
       MOZ_ASSERT(!decommitTask.isRunning());
@@ -7370,25 +7366,16 @@ MOZ_NEVER_INLINE GCRuntime::IncrementalR
   }
 
   // We don't allow off-thread parsing to start while we're doing an
   // incremental GC of the atoms zone.
   if (rt->activeGCInAtomsZone()) {
     session.maybeCheckAtomsAccess.emplace(rt);
   }
 
-  auto result =
-      budgetIncrementalGC(nonincrementalByAPI, reason, budget, session);
-
-  // If an ongoing incremental GC was reset, we may need to restart.
-  if (result == IncrementalResult::ResetIncremental) {
-    MOZ_ASSERT(!isIncrementalGCInProgress());
-    return result;
-  }
-
   gcTracer.traceMajorGCStart();
 
   result = incrementalSlice(budget, reason, session);
 
   chunkAllocationSinceLastGC = false;
 
 #ifdef JS_GC_ZEAL
   /* Keeping these around after a GC is dangerous. */
@@ -7576,19 +7563,16 @@ void GCRuntime::collect(bool nonincremen
         /* Need to re-schedule all zones for GC. */
         JS::PrepareForFullGC(rt->mainContextFromOwnThread());
         repeat = true;
         reason = JS::gcreason::ROOTS_REMOVED;
       } else if (shouldRepeatForDeadZone(reason)) {
         repeat = true;
         reason = JS::gcreason::COMPARTMENT_REVIVED;
       }
-    } else if (cycleResult == ReturnToEvictNursery) {
-      /* Repeat so we can collect the nursery and run another slice. */
-      repeat = true;
     }
   } while (repeat);
 
   if (reason == JS::gcreason::COMPARTMENT_REVIVED) {
     maybeDoCycleCollection();
   }
 
 #ifdef JS_GC_ZEAL
--- a/js/src/gc/GCRuntime.h
+++ b/js/src/gc/GCRuntime.h
@@ -521,17 +521,17 @@ class GCRuntime {
   void startTask(GCParallelTask& task, gcstats::PhaseKind phase,
                  AutoLockHelperThreadState& locked);
   void joinTask(GCParallelTask& task, gcstats::PhaseKind phase,
                 AutoLockHelperThreadState& locked);
 
   void mergeRealms(JS::Realm* source, JS::Realm* target);
 
  private:
-  enum IncrementalResult { ResetIncremental = 0, ReturnToEvictNursery, Ok };
+  enum IncrementalResult { ResetIncremental = 0, Ok };
 
   // Delete an empty zone after its contents have been merged.
   void deleteEmptyZone(Zone* zone);
 
   // For ArenaLists::allocateFromArena()
   friend class ArenaLists;
   Chunk* pickChunk(AutoLockGCBgAlloc& lock);
   Arena* allocateArena(Chunk* chunk, Zone* zone, AllocKind kind,
@@ -561,20 +561,18 @@ class GCRuntime {
   friend class BackgroundAllocTask;
   bool wantBackgroundAllocation(const AutoLockGC& lock) const;
   bool startBackgroundAllocTaskIfIdle();
 
   void requestMajorGC(JS::gcreason::Reason reason);
   SliceBudget defaultBudget(JS::gcreason::Reason reason, int64_t millis);
   IncrementalResult budgetIncrementalGC(bool nonincrementalByAPI,
                                         JS::gcreason::Reason reason,
-                                        SliceBudget& budget,
-                                        AutoGCSession& session);
-  IncrementalResult resetIncrementalGC(AbortReason reason,
-                                       AutoGCSession& session);
+                                        SliceBudget& budget);
+  IncrementalResult resetIncrementalGC(AbortReason reason);
 
   // Assert if the system state is such that we should never
   // receive a request to do GC work.
   void checkCanCallAPI();
 
   // Check if the system state is such that GC has been supressed
   // or otherwise delayed.
   MOZ_MUST_USE bool checkIfGCAllowedInCurrentState(JS::gcreason::Reason reason);
@@ -585,18 +583,16 @@ class GCRuntime {
 
   /*
    * Run one GC "cycle" (either a slice of incremental GC or an entire
    * non-incremental GC).
    *
    * Returns:
    *  * ResetIncremental if we "reset" an existing incremental GC, which would
    *    force us to run another cycle or
-   *  * ReturnToEvictNursery if the collector needs the nursery to be
-   *    evicted before it can continue or
    *  * Ok otherwise.
    */
   MOZ_MUST_USE IncrementalResult gcCycle(bool nonincrementalByAPI,
                                          SliceBudget& budget,
                                          JS::gcreason::Reason reason);
   bool shouldRepeatForDeadZone(JS::gcreason::Reason reason);
   IncrementalResult incrementalSlice(SliceBudget& budget,
                                      JS::gcreason::Reason reason,
--- a/js/src/gc/StoreBuffer.cpp
+++ b/js/src/gc/StoreBuffer.cpp
@@ -26,17 +26,17 @@ void StoreBuffer::GenericBuffer::trace(S
 
   for (LifoAlloc::Enum e(*storage_); !e.empty();) {
     unsigned size = *e.read<unsigned>();
     BufferableRef* edge = e.read<BufferableRef>(size);
     edge->trace(trc);
   }
 }
 
-inline void StoreBuffer::checkEmpty() const {
+void StoreBuffer::checkEmpty() const {
   MOZ_ASSERT(bufferVal.isEmpty());
   MOZ_ASSERT(bufferCell.isEmpty());
   MOZ_ASSERT(bufferSlot.isEmpty());
   MOZ_ASSERT(bufferWholeCell.isEmpty());
   MOZ_ASSERT(bufferGeneric.isEmpty());
 }
 
 bool StoreBuffer::enable() {