Bug 1512045 - Simplify GC resets r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Tue, 11 Dec 2018 18:26:06 +0000
changeset 450063 b995059a83d2635cf78f4eb14ef48f5899358625
parent 450062 9e8ec8556200c532a63bdf69990f4b9f32fa38e9
child 450064 3a91b5014f05907245750d28e49316d783e43492
push id110438
push userjcoppeard@mozilla.com
push dateTue, 11 Dec 2018 18:26:26 +0000
treeherdermozilla-inbound@b995059a83d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1512045
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 1512045 - Simplify GC resets r=sfink
js/src/gc/GC.cpp
js/src/gc/GCEnum.h
js/src/gc/GCRuntime.h
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -156,20 +156,20 @@
  * in the background, are swept on the background thread. This accounts for most
  * of the sweeping work.
  *
  * Reset
  * -----
  *
  * During incremental collection it is possible, although unlikely, for
  * conditions to change such that incremental collection is no longer safe. In
- * this case, the collection is 'reset' by ResetIncrementalGC(). If we are in
+ * this case, the collection is 'reset' by resetIncrementalGC(). If we are in
  * the mark state, this just stops marking, but if we have started sweeping
- * already, we continue until we have swept the current sweep group. Following a
- * reset, a new non-incremental collection is started.
+ * already, we continue non-incrementally until we have swept the current sweep
+ * group. Following a reset, a new collection is started.
  *
  * Compacting GC
  * -------------
  *
  * Compacting GC happens at the end of a major GC as part of the last slice.
  * There are three parts:
  *
  *  - Arenas are selected for compaction.
@@ -4954,31 +4954,32 @@ static void ResetGrayList(Compartment* c
 void GCRuntime::getNextSweepGroup() {
   currentSweepGroup = currentSweepGroup->nextGroup();
   ++sweepGroupIndex;
   if (!currentSweepGroup) {
     abortSweepAfterCurrentGroup = false;
     return;
   }
 
+  MOZ_ASSERT_IF(abortSweepAfterCurrentGroup, !isIncremental);
+  if (!isIncremental) {
+    ZoneComponentFinder::mergeGroups(currentSweepGroup);
+  }
+
   for (Zone* zone = currentSweepGroup; zone; zone = zone->nextNodeInGroup()) {
     MOZ_ASSERT(zone->isGCMarking());
     MOZ_ASSERT(!zone->isQueuedForBackgroundSweep());
   }
 
-  if (!isIncremental) {
-    ZoneComponentFinder::mergeGroups(currentSweepGroup);
-  }
-
   if (abortSweepAfterCurrentGroup) {
-    MOZ_ASSERT(!isIncremental);
     for (SweepGroupZonesIter zone(rt); !zone.done(); zone.next()) {
       MOZ_ASSERT(!zone->gcNextGraphComponent);
       zone->setNeedsIncrementalBarrier(false);
       zone->changeGCState(Zone::Mark, Zone::NoGC);
+      zone->arenas.unmarkPreMarkedFreeCells();
       zone->gcGrayRoots().clearAndFree();
     }
 
     for (SweepGroupCompartmentsIter comp(rt); !comp.done(); comp.next()) {
       ResetGrayList(comp);
     }
 
     abortSweepAfterCurrentGroup = false;
@@ -6658,18 +6659,19 @@ void GCRuntime::finishCollection() {
   schedulingState.updateHighFrequencyMode(lastGCTime, currentTime, tunables);
 
   for (ZonesIter zone(rt, WithAtoms); !zone.done(); zone.next()) {
     if (zone->isCollecting()) {
       zone->changeGCState(Zone::Finished, Zone::NoGC);
       zone->notifyObservingDebuggers();
     }
 
-    MOZ_ASSERT(!zone->isCollectingFromAnyThread());
     MOZ_ASSERT(!zone->wasGCStarted());
+    MOZ_ASSERT(!zone->needsIncrementalBarrier());
+    MOZ_ASSERT(!zone->isOnList());
   }
 
   MOZ_ASSERT(zonesToMaybeCompact.ref().isEmpty());
 
   lastGCTime = currentTime;
 }
 
 static const char* HeapStateToLabel(JS::HeapState heapState) {
@@ -6710,34 +6712,34 @@ AutoHeapSession::~AutoHeapSession() {
 }
 
 JS_PUBLIC_API JS::HeapState JS::RuntimeHeapState() {
   return TlsContext.get()->runtime()->heapState();
 }
 
 GCRuntime::IncrementalResult GCRuntime::resetIncrementalGC(
     gc::AbortReason reason) {
+  // Drop as much work as possible from an ongoing incremental GC so
+  // we can start a new GC after it has finished.
   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:
     case State::MarkRoots:
+    case State::Finish:
       MOZ_CRASH("Unexpected GC state in resetIncrementalGC");
       break;
 
     case State::Mark: {
-      /* Cancel any ongoing marking. */
+      // Cancel any ongoing marking.
       marker.reset();
-      marker.stop();
       clearBufferedGrayRoots();
 
       for (GCCompartmentsIter c(rt); !c.done(); c.next()) {
         ResetGrayList(c);
       }
 
       for (GCZonesIter zone(rt); !zone.done(); zone.next()) {
         zone->setNeedsIncrementalBarrier(false);
@@ -6746,108 +6748,57 @@ GCRuntime::IncrementalResult GCRuntime::
       }
 
       {
         AutoLockHelperThreadState lock;
         blocksToFreeAfterSweeping.ref().freeAll();
       }
 
       lastMarkSlice = false;
-      incrementalState = State::NotActive;
+      incrementalState = State::Finish;
 
       MOZ_ASSERT(!marker.shouldCheckCompartments());
 
       break;
     }
 
     case State::Sweep: {
+      // Finish sweeping the current sweep group, then abort.
       marker.reset();
 
       for (CompartmentsIter c(rt); !c.done(); c.next()) {
         c->gcState.scheduledForDestruction = false;
       }
 
-      for (GCZonesIter zone(rt); !zone.done(); zone.next()) {
-        if (zone->isGCMarking()) {
-          zone->arenas.unmarkPreMarkedFreeCells();
-        }
-      }
-
-      /* Finish sweeping the current sweep group, then abort. */
       abortSweepAfterCurrentGroup = true;
-
-      /* Don't perform any compaction after sweeping. */
-      bool wasCompacting = isCompacting;
       isCompacting = false;
 
-      auto unlimited = SliceBudget::unlimited();
-      incrementalSlice(unlimited, JS::gcreason::RESET, session);
-
-      isCompacting = wasCompacting;
-
-      {
-        gcstats::AutoPhase ap(stats(),
-                              gcstats::PhaseKind::WAIT_BACKGROUND_THREAD);
-        waitBackgroundSweepOrAllocEnd();
-      }
       break;
     }
 
     case State::Finalize: {
-      {
-        gcstats::AutoPhase ap(stats(),
-                              gcstats::PhaseKind::WAIT_BACKGROUND_THREAD);
-        waitBackgroundSweepOrAllocEnd();
-      }
-
-      bool wasCompacting = isCompacting;
       isCompacting = false;
-
-      auto unlimited = SliceBudget::unlimited();
-      incrementalSlice(unlimited, JS::gcreason::RESET, session);
-
-      isCompacting = wasCompacting;
-
       break;
     }
 
     case State::Compact: {
-      bool wasCompacting = isCompacting;
-
-      isCompacting = true;
+      // Skip any remaining zones that would have been compacted.
+      MOZ_ASSERT(isCompacting);
       startedCompacting = true;
       zonesToMaybeCompact.ref().clear();
-
-      auto unlimited = SliceBudget::unlimited();
-      incrementalSlice(unlimited, JS::gcreason::RESET, session);
-
-      isCompacting = wasCompacting;
       break;
     }
 
     case State::Decommit: {
-      auto unlimited = SliceBudget::unlimited();
-      incrementalSlice(unlimited, JS::gcreason::RESET, session);
       break;
     }
   }
 
   stats().reset(reason);
 
-#ifdef DEBUG
-  assertBackgroundSweepingFinished();
-  for (ZonesIter zone(rt, WithAtoms); !zone.done(); zone.next()) {
-    MOZ_ASSERT(!zone->isCollectingFromAnyThread());
-    MOZ_ASSERT(!zone->needsIncrementalBarrier());
-    MOZ_ASSERT(!zone->isOnList());
-  }
-  MOZ_ASSERT(zonesToMaybeCompact.ref().isEmpty());
-  MOZ_ASSERT(incrementalState == State::NotActive);
-#endif
-
   return IncrementalResult::ResetIncremental;
 }
 
 namespace {
 
 /*
  * Temporarily disable barriers during GC slices.
  */
@@ -6907,17 +6858,17 @@ static bool IsShutdownGC(JS::gcreason::R
 static bool ShouldCleanUpEverything(JS::gcreason::Reason reason,
                                     JSGCInvocationKind gckind) {
   // During shutdown, we must clean everything up, for the sake of leak
   // detection. When a runtime has no contexts, or we're doing a GC before a
   // shutdown CC, those are strong indications that we're shutting down.
   return IsShutdownGC(reason) || gckind == GC_SHRINK;
 }
 
-GCRuntime::IncrementalResult GCRuntime::incrementalSlice(
+void GCRuntime::incrementalSlice(
     SliceBudget& budget, JS::gcreason::Reason reason, AutoGCSession& session) {
   AutoDisableBarriers disableBarriers(rt);
 
   bool destroyingRuntime = (reason == JS::gcreason::DESTROY_RUNTIME);
 
   number++;
 
   initialState = incrementalState;
@@ -6974,17 +6925,17 @@ GCRuntime::IncrementalResult GCRuntime::
 
       incrementalState = State::MarkRoots;
 
       MOZ_FALLTHROUGH;
 
     case State::MarkRoots:
       if (!beginMarkPhase(reason, session)) {
         incrementalState = State::NotActive;
-        return IncrementalResult::Ok;
+        return;
       }
 
       /* If we needed delayed marking for gray roots, then collect until done.
        */
       if (isIncremental && !hasValidGrayRootsBuffer()) {
         budget.makeUnlimited();
         isIncremental = false;
         stats().nonincremental(AbortReason::GrayRootBufferingFailed);
@@ -7058,30 +7009,31 @@ GCRuntime::IncrementalResult GCRuntime::
       }
 
       endSweepPhase(destroyingRuntime);
 
       incrementalState = State::Finalize;
 
       MOZ_FALLTHROUGH;
 
-    case State::Finalize: {
-      gcstats::AutoPhase ap(stats(),
-                            gcstats::PhaseKind::WAIT_BACKGROUND_THREAD);
-
-      // Yield until background finalization is done.
-      if (!budget.isUnlimited()) {
-        // Poll for end of background sweeping
-        if (isBackgroundSweeping()) {
-          break;
+    case State::Finalize:
+      {
+        gcstats::AutoPhase ap(stats(),
+                              gcstats::PhaseKind::WAIT_BACKGROUND_THREAD);
+
+        // Yield until background finalization is done.
+        if (!budget.isUnlimited()) {
+          // Poll for end of background sweeping
+          if (isBackgroundSweeping()) {
+            break;
+          }
+        } else {
+          waitBackgroundSweepEnd();
         }
-      } else {
-        waitBackgroundSweepEnd();
       }
-    }
 
       {
         // Re-sweep the zones list, now that background finalization is
         // finished to actually remove and free dead zones.
         gcstats::AutoPhase ap1(stats(), gcstats::PhaseKind::SWEEP);
         gcstats::AutoPhase ap2(stats(), gcstats::PhaseKind::DESTROY);
         AutoSetThreadIsSweeping threadIsSweeping;
         FreeOp fop(rt);
@@ -7125,26 +7077,29 @@ GCRuntime::IncrementalResult GCRuntime::
                             gcstats::PhaseKind::WAIT_BACKGROUND_THREAD);
 
       // Yield until background decommit is done.
       if (!budget.isUnlimited() && decommitTask.isRunning()) {
         break;
       }
 
       decommitTask.join();
-    }
-
+
+      incrementalState = State::Finish;
+
+      MOZ_FALLTHROUGH;
+    }
+
+    case State::Finish:
       finishCollection();
       incrementalState = State::NotActive;
       break;
   }
 
   MOZ_ASSERT(safeToYield);
-
-  return IncrementalResult::Ok;
 }
 
 gc::AbortReason gc::IsIncrementalGCUnsafe(JSRuntime* rt) {
   MOZ_ASSERT(!rt->mainContextFromOwnThread()->suppressGC);
 
   if (!rt->gc.isIncrementalGCAllowed()) {
     return gc::AbortReason::IncrementalDisabled;
   }
@@ -7238,16 +7193,17 @@ GCRuntime::IncrementalResult GCRuntime::
 
     if (isIncrementalGCInProgress() &&
         zone->isGCScheduled() != zone->wasGCStarted()) {
       reset = true;
     }
   }
 
   if (reset) {
+    budget.makeUnlimited();
     return resetIncrementalGC(AbortReason::ZoneChange);
   }
 
   return IncrementalResult::Ok;
 }
 
 namespace {
 
@@ -7336,37 +7292,34 @@ 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,
+    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;
+    reason = JS::gcreason::RESET;
   }
 
   if (shouldCollectNurseryForSlice(nonincrementalByAPI, budget)) {
     minorGC(reason, gcstats::PhaseKind::EVICT_NURSERY_FOR_MAJOR_GC);
   }
 
   AutoGCSession session(rt, JS::HeapState::MajorCollecting);
 
@@ -7392,27 +7345,30 @@ 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);
   }
 
   gcTracer.traceMajorGCStart();
 
-  result = incrementalSlice(budget, reason, session);
+  incrementalSlice(budget, reason, session);
 
   chunkAllocationSinceLastGC = false;
 
 #ifdef JS_GC_ZEAL
   /* Keeping these around after a GC is dangerous. */
   clearSelectedForMarking();
 #endif
 
   gcTracer.traceMajorGCEnd();
 
+  MOZ_ASSERT_IF(result == IncrementalResult::ResetIncremental,
+                !isIncrementalGCInProgress());
+
   return result;
 }
 
 bool GCRuntime::shouldCollectNurseryForSlice(bool nonincrementalByAPI,
                                              SliceBudget& budget) {
   if (!nursery().isEnabled()) {
     return false;
   }
@@ -7425,20 +7381,23 @@ bool GCRuntime::shouldCollectNurseryForS
     case State::Decommit:
       return true;
     case State::Mark:
       return (nonincrementalByAPI || budget.isUnlimited() || lastMarkSlice ||
               nursery().minorGCRequested() ||
               nursery().freeSpace() <
                   tunables.nurseryFreeThresholdForIdleCollection() ||
               hasIncrementalTwoSliceZealMode());
-    default:
-      // State::MarkRoots can't ever happen here.
-      MOZ_CRASH("Unhandled GC state");
-  }
+    case State::Finish:
+      return false;
+    case State::MarkRoots:
+      MOZ_CRASH("Unexpected GC state");
+  }
+
+  return false;
 }
 
 #ifdef JS_GC_ZEAL
 static bool IsDeterministicGCReason(JS::gcreason::Reason reason) {
   switch (reason) {
     case JS::gcreason::API:
     case JS::gcreason::DESTROY_RUNTIME:
     case JS::gcreason::LAST_DITCH:
--- a/js/src/gc/GCEnum.h
+++ b/js/src/gc/GCEnum.h
@@ -22,17 +22,18 @@ enum class MarkColor : uint32_t { Black 
 // The phases of an incremental GC.
 #define GCSTATES(D) \
   D(NotActive)      \
   D(MarkRoots)      \
   D(Mark)           \
   D(Sweep)          \
   D(Finalize)       \
   D(Compact)        \
-  D(Decommit)
+  D(Decommit)       \
+  D(Finish)
 enum class State {
 #define MAKE_STATE(name) name,
   GCSTATES(MAKE_STATE)
 #undef MAKE_STATE
 };
 
 // Reasons we reset an ongoing incremental GC or perform a non-incremental GC.
 #define GC_ABORT_REASONS(D)     \
--- a/js/src/gc/GCRuntime.h
+++ b/js/src/gc/GCRuntime.h
@@ -586,22 +586,22 @@ class GCRuntime {
    * non-incremental GC).
    *
    * Returns:
    *  * ResetIncremental if we "reset" an existing incremental GC, which would
    *    force us to run another cycle or
    *  * Ok otherwise.
    */
   MOZ_MUST_USE IncrementalResult gcCycle(bool nonincrementalByAPI,
-                                         SliceBudget& budget,
+                                         SliceBudget budget,
                                          JS::gcreason::Reason reason);
   bool shouldRepeatForDeadZone(JS::gcreason::Reason reason);
-  IncrementalResult incrementalSlice(SliceBudget& budget,
-                                     JS::gcreason::Reason reason,
-                                     AutoGCSession& session);
+  void incrementalSlice(SliceBudget& budget,
+                        JS::gcreason::Reason reason,
+                        AutoGCSession& session);
   MOZ_MUST_USE bool shouldCollectNurseryForSlice(bool nonincrementalByAPI,
                                                  SliceBudget& budget);
 
   friend class AutoCallGCCallbacks;
   void maybeCallGCCallback(JSGCStatus status);
 
   void pushZealSelectedObjects();
   void purgeRuntime();