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

@@ -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;

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_;
 

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,17 +77,13 @@ 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()->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