Bug 1337450 - Simplify GC resets and aborts r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Wed, 08 Feb 2017 13:35:49 +0000
changeset 341339 67160e6118d12e4ac9d6c38686587d2371ebf059
parent 341338 a75a77d1bb99e3238d422572fbf5e8fd102af8bb
child 341340 a04acd9bb5d2a8870c16da59f1a6d9e7057b8108
push id86689
push userjcoppeard@mozilla.com
push dateWed, 08 Feb 2017 13:43:18 +0000
treeherdermozilla-inbound@a04acd9bb5d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1337450
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 1337450 - Simplify GC resets and aborts r=sfink
js/src/builtin/TestingFunctions.cpp
js/src/gc/GCRuntime.h
js/src/jsgc.cpp
--- a/js/src/builtin/TestingFunctions.cpp
+++ b/js/src/builtin/TestingFunctions.cpp
@@ -973,17 +973,17 @@ AbortGC(JSContext* cx, unsigned argc, Va
     CallArgs args = CallArgsFromVp(argc, vp);
 
     if (args.length() != 0) {
         RootedObject callee(cx, &args.callee());
         ReportUsageErrorASCII(cx, callee, "Wrong number of arguments");
         return false;
     }
 
-    cx->runtime()->gc.abortGC();
+    JS::AbortIncrementalGC(cx);
     args.rval().setUndefined();
     return true;
 }
 
 static bool
 FullCompartmentChecks(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
--- a/js/src/gc/GCRuntime.h
+++ b/js/src/gc/GCRuntime.h
@@ -844,16 +844,22 @@ class GCRuntime
 
   private:
     enum IncrementalProgress
     {
         NotFinished = 0,
         Finished
     };
 
+    enum IncrementalResult
+    {
+        Reset = 0,
+        Ok
+    };
+
     // For ArenaLists::allocateFromArena()
     friend class ArenaLists;
     Chunk* pickChunk(const AutoLockGC& lock,
                      AutoMaybeStartBackgroundAllocation& maybeStartBGAlloc);
     Arena* allocateArena(Chunk* chunk, Zone* zone, AllocKind kind,
                          ShouldCheckThresholds checkThresholds, const AutoLockGC& lock);
     void arenaAllocatedDuringGC(JS::Zone* zone, Arena* arena);
 
@@ -878,32 +884,32 @@ class GCRuntime
 
     friend class BackgroundAllocTask;
     friend class AutoMaybeStartBackgroundAllocation;
     bool wantBackgroundAllocation(const AutoLockGC& lock) const;
     void startBackgroundAllocTaskIfIdle();
 
     void requestMajorGC(JS::gcreason::Reason reason);
     SliceBudget defaultBudget(JS::gcreason::Reason reason, int64_t millis);
-    void budgetIncrementalGC(JS::gcreason::Reason reason, SliceBudget& budget,
-                             AutoLockForExclusiveAccess& lock);
-    void resetIncrementalGC(AbortReason reason, AutoLockForExclusiveAccess& lock);
+    IncrementalResult budgetIncrementalGC(bool nonincrementalByAPI, JS::gcreason::Reason reason,
+                                          SliceBudget& budget, AutoLockForExclusiveAccess& lock);
+    IncrementalResult resetIncrementalGC(AbortReason reason, AutoLockForExclusiveAccess& lock);
 
     // 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);
 
     gcstats::ZoneGCStats scanZonesBeforeGC();
     void collect(bool nonincrementalByAPI, SliceBudget budget, JS::gcreason::Reason reason) JS_HAZ_GC_CALL;
-    MOZ_MUST_USE bool gcCycle(bool nonincrementalByAPI, SliceBudget& budget,
-                              JS::gcreason::Reason reason);
+    MOZ_MUST_USE IncrementalResult gcCycle(bool nonincrementalByAPI, SliceBudget& budget,
+                                           JS::gcreason::Reason reason);
     bool shouldRepeatForDeadZone(JS::gcreason::Reason reason);
     void incrementalCollectSlice(SliceBudget& budget, JS::gcreason::Reason reason,
                                  AutoLockForExclusiveAccess& lock);
 
     void pushZealSelectedObjects();
     void purgeRuntime(AutoLockForExclusiveAccess& lock);
     MOZ_MUST_USE bool beginMarkPhase(JS::gcreason::Reason reason, AutoLockForExclusiveAccess& lock);
     bool shouldPreserveJITCode(JSCompartment* comp, int64_t currentTime,
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -5640,24 +5640,24 @@ AutoTraceSession::~AutoTraceSession()
 }
 
 JS_PUBLIC_API(JS::HeapState)
 JS::CurrentThreadHeapState()
 {
     return TlsContext.get()->heapState;
 }
 
-void
+GCRuntime::IncrementalResult
 GCRuntime::resetIncrementalGC(gc::AbortReason reason, AutoLockForExclusiveAccess& lock)
 {
     MOZ_ASSERT(reason != gc::AbortReason::None);
 
     switch (incrementalState) {
       case State::NotActive:
-        return;
+          return IncrementalResult::Ok;
 
       case State::MarkRoots:
         MOZ_CRASH("resetIncrementalGC did not expect MarkRoots state");
         break;
 
       case State::Mark: {
         /* Cancel any ongoing marking. */
         marker.reset();
@@ -5752,16 +5752,18 @@ GCRuntime::resetIncrementalGC(gc::AbortR
     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::Reset;
 }
 
 namespace {
 
 class AutoGCSlice {
   public:
     explicit AutoGCSlice(JSRuntime* rt);
     ~AutoGCSlice();
@@ -6029,33 +6031,52 @@ gc::IsIncrementalGCUnsafe(JSRuntime* rt)
     MOZ_ASSERT(!TlsContext.get()->suppressGC);
 
     if (!rt->gc.isIncrementalGCAllowed())
         return gc::AbortReason::IncrementalDisabled;
 
     return gc::AbortReason::None;
 }
 
-void
-GCRuntime::budgetIncrementalGC(JS::gcreason::Reason reason, SliceBudget& budget,
-                               AutoLockForExclusiveAccess& lock)
-{
+GCRuntime::IncrementalResult
+GCRuntime::budgetIncrementalGC(bool nonincrementalByAPI, JS::gcreason::Reason reason,
+                               SliceBudget& budget, AutoLockForExclusiveAccess& lock)
+{
+    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, lock);
+
+        return IncrementalResult::Ok;
+    }
+
+    if (reason == JS::gcreason::ABORT_GC) {
+        budget.makeUnlimited();
+        stats().nonincremental(gc::AbortReason::AbortRequested);
+        return resetIncrementalGC(gc::AbortReason::AbortRequested, lock);
+    }
+
     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) {
-        resetIncrementalGC(unsafeReason, lock);
         budget.makeUnlimited();
         stats().nonincremental(unsafeReason);
-        return;
+        return resetIncrementalGC(unsafeReason, lock);
     }
 
     if (isTooMuchMalloc()) {
         budget.makeUnlimited();
         stats().nonincremental(AbortReason::MallocBytesTrigger);
     }
 
     bool reset = false;
@@ -6070,17 +6091,19 @@ GCRuntime::budgetIncrementalGC(JS::gcrea
 
         if (zone->isTooMuchMalloc()) {
             budget.makeUnlimited();
             stats().nonincremental(AbortReason::MallocBytesTrigger);
         }
     }
 
     if (reset)
-        resetIncrementalGC(AbortReason::ZoneChange, lock);
+        return resetIncrementalGC(AbortReason::ZoneChange, lock);
+
+    return IncrementalResult::Ok;
 }
 
 namespace {
 
 class AutoScheduleZonesForGC
 {
     JSRuntime* rt_;
     AutoAccessZoneGroups aazg;
@@ -6151,17 +6174,17 @@ class AutoExposeLiveCrossZoneEdges
  * Run one GC "cycle" (either a slice of incremental GC or an entire
  * non-incremental GC. 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.
  *
  * Returns true if we "reset" an existing incremental GC, which would force us
  * to run another cycle.
  */
-MOZ_NEVER_INLINE bool
+MOZ_NEVER_INLINE GCRuntime::IncrementalResult
 GCRuntime::gcCycle(bool nonincrementalByAPI, SliceBudget& budget, JS::gcreason::Reason reason)
 {
     // Note that the following is allowed to re-enter GC in the finalizer.
     AutoNotifyGCActivity notify(*this);
 
     gcstats::AutoGCSlice agc(stats(), scanZonesBeforeGC(), invocationKind, budget, reason);
 
     AutoExposeLiveCrossZoneEdges aelcze(&foundBlackGrayEdges.ref());
@@ -6196,39 +6219,27 @@ GCRuntime::gcCycle(bool nonincrementalBy
 
         // We must also wait for background allocation to finish so we can
         // avoid taking the GC lock when manipulating the chunks during the GC.
         // The background alloc task can run between slices, so we must wait
         // for it at the start of every slice.
         allocTask.cancel(GCParallelTask::CancelAndWait);
     }
 
-    State prevState = incrementalState;
-
     // We don't allow off-main-thread parsing to start while we're doing an
     // incremental GC.
     MOZ_ASSERT_IF(rt->activeGCInAtomsZone(), !rt->exclusiveThreadsPresent());
 
-    if (nonincrementalByAPI) {
-        // 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)
-            resetIncrementalGC(gc::AbortReason::NonIncrementalRequested, session.lock);
-
-        stats().nonincremental(gc::AbortReason::NonIncrementalRequested);
-        budget.makeUnlimited();
-    } else {
-        budgetIncrementalGC(reason, budget, session.lock);
-    }
-
-    /* The GC was reset, so we need a do-over. */
-    if (prevState != State::NotActive && !isIncrementalGCInProgress())
-        return true;
+    auto result = budgetIncrementalGC(nonincrementalByAPI, reason, budget, session.lock);
+
+    // If an ongoing incremental GC was reset, we may need to restart.
+    if (result == IncrementalResult::Reset) {
+        MOZ_ASSERT(!isIncrementalGCInProgress());
+        return result;
+    }
 
     TraceMajorGCStart();
 
     incrementalCollectSlice(budget, reason, session.lock);
 
     chunkAllocationSinceLastGC = false;
 
 #ifdef JS_GC_ZEAL
@@ -6239,17 +6250,17 @@ GCRuntime::gcCycle(bool nonincrementalBy
     /* Clear gcMallocBytes for all zones. */
     for (ZonesIter zone(rt, WithAtoms); !zone.done(); zone.next())
         zone->resetGCMallocBytes();
 
     resetMallocBytes();
 
     TraceMajorGCEnd();
 
-    return false;
+    return IncrementalResult::Ok;
 }
 
 #ifdef JS_GC_ZEAL
 static bool
 IsDeterministicGCReason(JS::gcreason::Reason reason)
 {
     if (reason > JS::gcreason::DEBUG_GC &&
         reason != JS::gcreason::CC_FORCED && reason != JS::gcreason::SHUTDOWN_CC)
@@ -6366,17 +6377,22 @@ GCRuntime::collect(bool nonincrementalBy
     AutoTraceLog logGC(TraceLoggerForCurrentThread(), TraceLogger_GC);
     AutoStopVerifyingBarriers av(rt, IsShutdownGC(reason));
     AutoEnqueuePendingParseTasksAfterGC aept(*this);
     AutoScheduleZonesForGC asz(rt);
 
     bool repeat = false;
     do {
         poked = false;
-        bool wasReset = gcCycle(nonincrementalByAPI, budget, reason);
+        bool wasReset = gcCycle(nonincrementalByAPI, budget, reason) == IncrementalResult::Reset;
+
+        if (reason == JS::gcreason::ABORT_GC) {
+            MOZ_ASSERT(!isIncrementalGCInProgress());
+            break;
+        }
 
         bool repeatForDeadZone = false;
         if (poked && cleanUpEverything) {
             /* Need to re-schedule all zones for GC. */
             JS::PrepareForFullGC(rt->contextFromMainThread());
         } else if (shouldRepeatForDeadZone(reason) && !wasReset) {
             /*
              * This code makes an extra effort to collect compartments that we
@@ -6472,30 +6488,21 @@ GCRuntime::finishGC(JS::gcreason::Reason
     }
 
     collect(false, SliceBudget::unlimited(), reason);
 }
 
 void
 GCRuntime::abortGC()
 {
+    MOZ_ASSERT(isIncrementalGCInProgress());
     checkCanCallAPI();
     MOZ_ASSERT(!TlsContext.get()->suppressGC);
 
-    AutoStopVerifyingBarriers av(rt, false);
-    AutoEnqueuePendingParseTasksAfterGC aept(*this);
-
-    gcstats::AutoGCSlice agc(stats(), scanZonesBeforeGC(), invocationKind,
-                             SliceBudget::unlimited(), JS::gcreason::ABORT_GC);
-
-    rt->zoneGroupFromMainThread()->evictNursery(JS::gcreason::ABORT_GC);
-    AutoTraceSession session(rt, JS::HeapState::MajorCollecting);
-
-    number++;
-    resetIncrementalGC(gc::AbortReason::AbortRequested, session.lock);
+    collect(false, SliceBudget::unlimited(), JS::gcreason::ABORT_GC);
 }
 
 void
 GCRuntime::notifyDidPaint()
 {
     MOZ_ASSERT(CurrentThreadCanAccessRuntime(rt));
 
 #ifdef JS_GC_ZEAL
@@ -7239,17 +7246,18 @@ JS_PUBLIC_API(void)
 JS::FinishIncrementalGC(JSContext* cx, gcreason::Reason reason)
 {
     cx->runtime()->gc.finishGC(reason);
 }
 
 JS_PUBLIC_API(void)
 JS::AbortIncrementalGC(JSContext* cx)
 {
-    cx->runtime()->gc.abortGC();
+    if (IsIncrementalGCInProgress(cx))
+        cx->runtime()->gc.abortGC();
 }
 
 char16_t*
 JS::GCDescription::formatSliceMessage(JSContext* cx) const
 {
     UniqueChars cstr = cx->runtime()->gc.stats().formatCompactSliceMessage();
 
     size_t nchars = strlen(cstr.get());