From bab2b3554270387954186952ad96973222c07aa9 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Mon, 3 Dec 2018 17:17:34 -0500 Subject: [PATCH] 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 | 90 ++++++++++++++++----------------------- js/src/gc/GCRuntime.h | 10 ++--- js/src/gc/StoreBuffer.cpp | 2 +- 3 files changed, 41 insertions(+), 61 deletions(-) diff --git a/js/src/gc/GC.cpp b/js/src/gc/GC.cpp index e631d6922a30..0c2269e69d41 100644 --- a/js/src/gc/GC.cpp +++ b/js/src/gc/GC.cpp @@ -6714,15 +6714,19 @@ JS_PUBLIC_API JS::HeapState JS::RuntimeHeapState() { } 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: { @@ -6914,6 +6918,8 @@ GCRuntime::IncrementalResult GCRuntime::incrementalSlice( bool destroyingRuntime = (reason == JS::gcreason::DESTROY_RUNTIME); + number++; + initialState = incrementalState; #ifdef JS_GC_ZEAL @@ -6959,6 +6965,7 @@ GCRuntime::IncrementalResult GCRuntime::incrementalSlice( switch (incrementalState) { case State::NotActive: + incMajorGcNumber(); initialReason = reason; cleanUpEverything = ShouldCleanUpEverything(reason, invocationKind); isCompacting = shouldCompact(); @@ -7034,23 +7041,6 @@ GCRuntime::IncrementalResult GCRuntime::incrementalSlice( } } - /* - * 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); @@ -7059,6 +7049,7 @@ GCRuntime::IncrementalResult GCRuntime::incrementalSlice( case State::Sweep: MOZ_ASSERT(nursery().isEmpty()); + storeBuffer().checkEmpty(); AutoGCRooter::traceAllWrappers(rt->mainContextFromOwnThread(), &marker); @@ -7108,6 +7099,9 @@ GCRuntime::IncrementalResult GCRuntime::incrementalSlice( MOZ_FALLTHROUGH; case State::Compact: + MOZ_ASSERT(nursery().isEmpty()); + storeBuffer().checkEmpty(); + if (isCompacting) { MOZ_ASSERT(nursery().isEmpty()); if (!startedCompacting) { @@ -7181,8 +7175,8 @@ static inline void CheckZoneIsScheduled(Zone* zone, JS::gcreason::Reason reason, } 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(); @@ -7192,8 +7186,7 @@ GCRuntime::IncrementalResult GCRuntime::budgetIncrementalGC( // 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; @@ -7202,7 +7195,7 @@ GCRuntime::IncrementalResult GCRuntime::budgetIncrementalGC( 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); @@ -7217,7 +7210,7 @@ GCRuntime::IncrementalResult GCRuntime::budgetIncrementalGC( if (unsafeReason != AbortReason::None) { budget.makeUnlimited(); stats().nonincremental(unsafeReason); - return resetIncrementalGC(unsafeReason, session); + return resetIncrementalGC(unsafeReason); } if (mallocCounter.shouldTriggerGC(tunables) == NonIncrementalTrigger) { @@ -7250,7 +7243,7 @@ GCRuntime::IncrementalResult GCRuntime::budgetIncrementalGC( } if (reset) { - return resetIncrementalGC(AbortReason::ZoneChange, session); + return resetIncrementalGC(AbortReason::ZoneChange); } return IncrementalResult::Ok; @@ -7350,12 +7343,27 @@ void GCRuntime::maybeCallGCCallback(JSGCStatus status) { 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); } @@ -7364,18 +7372,6 @@ MOZ_NEVER_INLINE GCRuntime::IncrementalResult GCRuntime::gcCycle( 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); @@ -7399,15 +7395,6 @@ MOZ_NEVER_INLINE GCRuntime::IncrementalResult GCRuntime::gcCycle( 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); @@ -7605,9 +7592,6 @@ void GCRuntime::collect(bool nonincrementalByAPI, SliceBudget budget, 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); diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h index d5c4f8657826..e23039b01ed2 100644 --- a/js/src/gc/GCRuntime.h +++ b/js/src/gc/GCRuntime.h @@ -526,7 +526,7 @@ class GCRuntime { 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); @@ -566,10 +566,8 @@ class GCRuntime { 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. @@ -590,8 +588,6 @@ class GCRuntime { * 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, diff --git a/js/src/gc/StoreBuffer.cpp b/js/src/gc/StoreBuffer.cpp index 7dcea96c3a8e..e81443d6aab8 100644 --- a/js/src/gc/StoreBuffer.cpp +++ b/js/src/gc/StoreBuffer.cpp @@ -31,7 +31,7 @@ void StoreBuffer::GenericBuffer::trace(StoreBuffer* owner, JSTracer* trc) { } } -inline void StoreBuffer::checkEmpty() const { +void StoreBuffer::checkEmpty() const { MOZ_ASSERT(bufferVal.isEmpty()); MOZ_ASSERT(bufferCell.isEmpty()); MOZ_ASSERT(bufferSlot.isEmpty());