Bug 1510145 - Refactor GC resets and ensure the store buffer is always empty when we start sweeping r=pbone a=dveditz
authorJon Coppeard <jcoppeard@mozilla.com>
Mon, 03 Dec 2018 17:17:34 -0500
changeset 450061 aba3963bc7656b590c5ac9d3a41b807c2dfafa73
parent 450060 48ade30f2983eb3806c7cfa08bf6946c86fc5d13
child 450062 9e8ec8556200c532a63bdf69990f4b9f32fa38e9
child 450119 ac7f3beb633340c6b3fea25059e8b8aa52b5fc8a
push id110437
push userjcoppeard@mozilla.com
push dateTue, 11 Dec 2018 18:14:39 +0000
treeherdermozilla-inbound@aba3963bc765 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbone, dveditz
bugs1510145
milestone66.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 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
@@ -6709,25 +6709,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();
 
@@ -6909,16 +6913,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().
    */
@@ -6954,16 +6960,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;
 
@@ -7029,41 +7036,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);
@@ -7103,16 +7094,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;
@@ -7176,53 +7170,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;
@@ -7245,17 +7238,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 {
@@ -7345,42 +7338,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());
@@ -7394,25 +7390,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. */
@@ -7600,19 +7587,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() {