From 39491c5168ee283e2a2e0053d0f6d809e1631058 Mon Sep 17 00:00:00 2001 From: ulan Date: Tue, 19 May 2015 11:12:18 -0700 Subject: [PATCH] Restore NothingOrDone action in idle time handler. This also adjusts transitioning between modes so that crbug.com/460090 remains fixed. BUG=chromium:489323, chromium:460090 LOG=NO Review URL: https://codereview.chromium.org/1141393002 Cr-Commit-Position: refs/heads/master@{#28490} --- src/heap/gc-idle-time-handler.cc | 57 +++++++++----- src/heap/gc-idle-time-handler.h | 37 ++++++--- .../heap/gc-idle-time-handler-unittest.cc | 75 ++++++++++++++++--- 3 files changed, 130 insertions(+), 39 deletions(-) diff --git a/src/heap/gc-idle-time-handler.cc b/src/heap/gc-idle-time-handler.cc index 40c2eff3f9..0a174d533d 100644 --- a/src/heap/gc-idle-time-handler.cc +++ b/src/heap/gc-idle-time-handler.cc @@ -192,6 +192,17 @@ bool GCIdleTimeHandler::ShouldDoOverApproximateWeakClosure( } +GCIdleTimeAction GCIdleTimeHandler::NothingOrDone() { + if (idle_times_which_made_no_progress_per_mode_ >= + kMaxNoProgressIdleTimesPerMode) { + return GCIdleTimeAction::Done(); + } else { + idle_times_which_made_no_progress_per_mode_++; + return GCIdleTimeAction::Nothing(); + } +} + + // The idle time handler has three modes and transitions between them // as shown in the diagram: // @@ -283,7 +294,7 @@ GCIdleTimeAction GCIdleTimeHandler::Action(double idle_time_in_ms, // get the right idle signal. if (ShouldDoContextDisposalMarkCompact(heap_state.contexts_disposed, heap_state.contexts_disposal_rate)) { - return GCIdleTimeAction::Nothing(); + return NothingOrDone(); } if (ShouldDoScavenge( @@ -306,13 +317,13 @@ GCIdleTimeAction GCIdleTimeHandler::Action(double idle_time_in_ms, if (heap_state.sweeping_completed) { return GCIdleTimeAction::FinalizeSweeping(); } else { - return GCIdleTimeAction::Nothing(); + return NothingOrDone(); } } if (heap_state.incremental_marking_stopped && !heap_state.can_start_incremental_marking && !reduce_memory) { - return GCIdleTimeAction::Nothing(); + return NothingOrDone(); } size_t step_size = EstimateMarkingStepSize( @@ -324,19 +335,19 @@ GCIdleTimeAction GCIdleTimeHandler::Action(double idle_time_in_ms, void GCIdleTimeHandler::UpdateCounters(double idle_time_in_ms) { if (mode_ == kReduceLatency) { - int mutator_gcs = scavenges_ + mark_compacts_ - idle_mark_compacts_; - if (mutator_gcs > 0) { - // There was a mutator GC since the last notification. + int gcs = scavenges_ + mark_compacts_; + if (gcs > 0) { + // There was a GC since the last notification. long_idle_notifications_ = 0; + background_idle_notifications_ = 0; } idle_mark_compacts_ = 0; mark_compacts_ = 0; scavenges_ = 0; - if (idle_time_in_ms >= kMinLongIdleTime) { - long_idle_notifications_ += - (idle_time_in_ms >= kLargeLongIdleTime) - ? kLongIdleNotificationsBeforeMutatorIsIdle - : 1; + if (idle_time_in_ms >= kMinBackgroundIdleTime) { + background_idle_notifications_++; + } else if (idle_time_in_ms >= kMinLongIdleTime) { + long_idle_notifications_++; } } } @@ -344,20 +355,29 @@ void GCIdleTimeHandler::UpdateCounters(double idle_time_in_ms) { void GCIdleTimeHandler::ResetCounters() { long_idle_notifications_ = 0; + background_idle_notifications_ = 0; idle_mark_compacts_ = 0; mark_compacts_ = 0; scavenges_ = 0; + idle_times_which_made_no_progress_per_mode_ = 0; } -bool GCIdleTimeHandler::IsMutatorActive(int contexts_disposed, int gcs) { - return contexts_disposed > 0 || gcs >= kGCsBeforeMutatorIsActive; +bool GCIdleTimeHandler::IsMutatorActive(int contexts_disposed, + int mark_compacts) { + return contexts_disposed > 0 || + mark_compacts >= kMarkCompactsBeforeMutatorIsActive; } -bool GCIdleTimeHandler::IsMutatorIdle(int long_idle_notifications, int gcs) { - return gcs == 0 && - long_idle_notifications >= kLongIdleNotificationsBeforeMutatorIsIdle; +bool GCIdleTimeHandler::IsMutatorIdle(int long_idle_notifications, + int background_idle_notifications, + int mutator_gcs) { + return mutator_gcs == 0 && + (long_idle_notifications >= + kLongIdleNotificationsBeforeMutatorIsIdle || + background_idle_notifications >= + kBackgroundIdleNotificationsBeforeMutatorIsIdle); } @@ -368,12 +388,13 @@ GCIdleTimeHandler::Mode GCIdleTimeHandler::NextMode( switch (mode_) { case kDone: DCHECK(idle_mark_compacts_ == 0); - if (IsMutatorActive(heap_state.contexts_disposed, mutator_gcs)) { + if (IsMutatorActive(heap_state.contexts_disposed, mark_compacts_)) { return kReduceLatency; } break; case kReduceLatency: - if (IsMutatorIdle(long_idle_notifications_, mutator_gcs)) { + if (IsMutatorIdle(long_idle_notifications_, + background_idle_notifications_, mutator_gcs)) { return kReduceMemory; } break; diff --git a/src/heap/gc-idle-time-handler.h b/src/heap/gc-idle-time-handler.h index e76178b7e6..efa4869956 100644 --- a/src/heap/gc-idle-time-handler.h +++ b/src/heap/gc-idle-time-handler.h @@ -151,18 +151,24 @@ class GCIdleTimeHandler { // the kDone mode. static const int kMaxIdleMarkCompacts = 3; - // The number of mutator GCs before transitioning to the kReduceLatency mode. - static const int kGCsBeforeMutatorIsActive = 7; + // The number of mutator MarkCompact GCs before transitioning to the + // kReduceLatency mode. + static const int kMarkCompactsBeforeMutatorIsActive = 1; // Mutator is considered idle if - // 1) there is an idle notification with time >= kLargeLongIdleTime, - // 2) or there are kLongIdleNotificationsBeforeMutatorIsIdle idle - // notifications - // with time >= kMinLongIdleTime and without any mutator GC in between. + // 1) there are N idle notification with time >= kMinBackgroundIdleTime, + // 2) or there are M idle notifications with time >= kMinLongIdleTime + // without any mutator GC in between. + // Where N = kBackgroundIdleNotificationsBeforeMutatorIsIdle, + // M = kLongIdleNotificationsBeforeMutatorIsIdle static const int kMinLongIdleTime = kMaxFrameRenderingIdleTime + 1; - static const int kLargeLongIdleTime = 900; - static const int kLongIdleNotificationsBeforeMutatorIsIdle = 600; - + static const int kMinBackgroundIdleTime = 900; + static const int kBackgroundIdleNotificationsBeforeMutatorIsIdle = 2; + static const int kLongIdleNotificationsBeforeMutatorIsIdle = 50; + // Number of times we will return a Nothing action in the current mode + // despite having idle time available before we returning a Done action to + // ensure we don't keep scheduling idle tasks and making no progress. + static const int kMaxNoProgressIdleTimesPerMode = 10; class HeapState { public: @@ -189,6 +195,8 @@ class GCIdleTimeHandler { mark_compacts_(0), scavenges_(0), long_idle_notifications_(0), + background_idle_notifications_(0), + idle_times_which_made_no_progress_per_mode_(0), mode_(kReduceLatency) {} GCIdleTimeAction Compute(double idle_time_in_ms, HeapState heap_state); @@ -232,19 +240,26 @@ class GCIdleTimeHandler { private: bool IsMutatorActive(int contexts_disposed, int gcs); - bool IsMutatorIdle(int long_idle_notifications, int gcs); + bool IsMutatorIdle(int long_idle_notifications, + int background_idle_notifications, int gcs); void UpdateCounters(double idle_time_in_ms); void ResetCounters(); Mode NextMode(const HeapState& heap_state); GCIdleTimeAction Action(double idle_time_in_ms, const HeapState& heap_state, bool reduce_memory); + GCIdleTimeAction NothingOrDone(); int idle_mark_compacts_; int mark_compacts_; int scavenges_; - // The number of long idle notifications with no mutator GC happening + // The number of long idle notifications with no GC happening // between the notifications. int long_idle_notifications_; + // The number of background idle notifications with no GC happening + // between the notifications. + int background_idle_notifications_; + // Idle notifications with no progress in the current mode. + int idle_times_which_made_no_progress_per_mode_; Mode mode_; diff --git a/test/unittests/heap/gc-idle-time-handler-unittest.cc b/test/unittests/heap/gc-idle-time-handler-unittest.cc index 357b08f881..0f1e498b26 100644 --- a/test/unittests/heap/gc-idle-time-handler-unittest.cc +++ b/test/unittests/heap/gc-idle-time-handler-unittest.cc @@ -48,7 +48,11 @@ class GCIdleTimeHandlerTest : public ::testing::Test { heap_state.can_start_incremental_marking; for (int i = 0; i < limit; i++) { GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state); - EXPECT_EQ(incremental ? DO_INCREMENTAL_MARKING : DO_NOTHING, action.type); + if (incremental) { + EXPECT_EQ(DO_INCREMENTAL_MARKING, action.type); + } else { + EXPECT_TRUE(DO_NOTHING == action.type || DONE == action.type); + } } handler()->Compute(idle_time_ms, heap_state); EXPECT_EQ(GCIdleTimeHandler::kReduceMemory, handler()->mode()); @@ -73,16 +77,12 @@ class GCIdleTimeHandlerTest : public ::testing::Test { void TransitionToReduceLatencyMode( const GCIdleTimeHandler::HeapState& heap_state) { EXPECT_EQ(GCIdleTimeHandler::kDone, handler()->mode()); - int limit = GCIdleTimeHandler::kGCsBeforeMutatorIsActive; + int limit = GCIdleTimeHandler::kMarkCompactsBeforeMutatorIsActive; double idle_time_ms = GCIdleTimeHandler::kMinLongIdleTime; for (int i = 0; i < limit; i++) { GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state); EXPECT_EQ(DONE, action.type); - if (i % 2 == 0) { - handler()->NotifyScavenge(); - } else { - handler()->NotifyMarkCompact(); - } + handler()->NotifyMarkCompact(); } handler()->Compute(idle_time_ms, heap_state); EXPECT_EQ(GCIdleTimeHandler::kReduceLatency, handler()->mode()); @@ -550,7 +550,7 @@ TEST_F(GCIdleTimeHandlerTest, SmallIdleTimeNothingToDo) { heap_state.can_start_incremental_marking = false; for (int i = 0; i < kMaxNotifications; i++) { GCIdleTimeAction action = handler()->Compute(10, heap_state); - EXPECT_EQ(DO_NOTHING, action.type); + EXPECT_TRUE(DO_NOTHING == action.type || DONE == action.type); } } @@ -563,7 +563,7 @@ TEST_F(GCIdleTimeHandlerTest, StayInReduceLatencyModeBecauseOfScavenges) { int limit = GCIdleTimeHandler::kLongIdleNotificationsBeforeMutatorIsIdle; for (int i = 0; i < kMaxNotifications; i++) { GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state); - EXPECT_EQ(DO_NOTHING, action.type); + EXPECT_TRUE(DO_NOTHING == action.type || DONE == action.type); if ((i + 1) % limit == 0) handler()->NotifyScavenge(); EXPECT_EQ(GCIdleTimeHandler::kReduceLatency, handler()->mode()); } @@ -578,7 +578,7 @@ TEST_F(GCIdleTimeHandlerTest, StayInReduceLatencyModeBecauseOfMarkCompacts) { int limit = GCIdleTimeHandler::kLongIdleNotificationsBeforeMutatorIsIdle; for (int i = 0; i < kMaxNotifications; i++) { GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state); - EXPECT_EQ(DO_NOTHING, action.type); + EXPECT_TRUE(DO_NOTHING == action.type || DONE == action.type); if ((i + 1) % limit == 0) handler()->NotifyMarkCompact(); EXPECT_EQ(GCIdleTimeHandler::kReduceLatency, handler()->mode()); } @@ -643,5 +643,60 @@ TEST_F(GCIdleTimeHandlerTest, ReduceMemoryToDone) { } +TEST_F(GCIdleTimeHandlerTest, DoneIfNotMakingProgressOnSweeping) { + // Regression test for crbug.com/489323. + GCIdleTimeHandler::HeapState heap_state = DefaultHeapState(); + + // Simulate sweeping being in-progress but not complete. + heap_state.incremental_marking_stopped = true; + heap_state.can_start_incremental_marking = false; + heap_state.sweeping_in_progress = true; + heap_state.sweeping_completed = false; + double idle_time_ms = 10.0; + for (int i = 0; i < GCIdleTimeHandler::kMaxNoProgressIdleTimesPerMode; i++) { + GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state); + EXPECT_EQ(DO_NOTHING, action.type); + } + // We should return DONE after not making progress for some time. + GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state); + EXPECT_EQ(DONE, action.type); +} + + +TEST_F(GCIdleTimeHandlerTest, DoneIfNotMakingProgressOnIncrementalMarking) { + // Regression test for crbug.com/489323. + GCIdleTimeHandler::HeapState heap_state = DefaultHeapState(); + + // Simulate incremental marking stopped and not eligible to start. + heap_state.incremental_marking_stopped = true; + heap_state.can_start_incremental_marking = false; + double idle_time_ms = 10.0; + for (int i = 0; i < GCIdleTimeHandler::kMaxNoProgressIdleTimesPerMode; i++) { + GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state); + EXPECT_EQ(DO_NOTHING, action.type); + } + // We should return DONE after not making progress for some time. + GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state); + EXPECT_EQ(DONE, action.type); +} + + +TEST_F(GCIdleTimeHandlerTest, BackgroundReduceLatencyToReduceMemory) { + GCIdleTimeHandler::HeapState heap_state = DefaultHeapState(); + heap_state.incremental_marking_stopped = false; + heap_state.can_start_incremental_marking = true; + double idle_time_ms = GCIdleTimeHandler::kMinBackgroundIdleTime; + handler()->NotifyScavenge(); + EXPECT_EQ(GCIdleTimeHandler::kReduceLatency, handler()->mode()); + int limit = + GCIdleTimeHandler::kBackgroundIdleNotificationsBeforeMutatorIsIdle; + for (int i = 0; i < limit; i++) { + GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state); + EXPECT_EQ(DO_INCREMENTAL_MARKING, action.type); + } + handler()->Compute(idle_time_ms, heap_state); + EXPECT_EQ(GCIdleTimeHandler::kReduceMemory, handler()->mode()); +} + } // namespace internal } // namespace v8