Add mode to reduce memory usage in idle notification.

src/heap/gc-idle-time-handler.cc

Index: src/heap/gc-idle-time-handler.cc
diff --git a/src/heap/gc-idle-time-handler.cc b/src/heap/gc-idle-time-handler.cc
index a3383b9c0c0dcde34864a7fbbafef853d24d5d55..eeaf3b056a82774a2a6d3ae922b36955f57c1635 100644
--- a/src/heap/gc-idle-time-handler.cc
+++ b/src/heap/gc-idle-time-handler.cc
@@ -12,8 +12,6 @@ namespace internal {
 const double GCIdleTimeHandler::kConservativeTimeRatio = 0.9;
 const size_t GCIdleTimeHandler::kMaxMarkCompactTimeInMs = 1000;
 const size_t GCIdleTimeHandler::kMaxFinalIncrementalMarkCompactTimeInMs = 1000;
-const int GCIdleTimeHandler::kMaxMarkCompactsInIdleRound = 2;
-const int GCIdleTimeHandler::kIdleScavengeThreshold = 5;
 const double GCIdleTimeHandler::kHighContextDisposalRate = 100;
 const size_t GCIdleTimeHandler::kMinTimeForOverApproximatingWeakClosureInMs = 1;
 
@@ -187,13 +185,56 @@ bool GCIdleTimeHandler::ShouldDoOverApproximateWeakClosure(
 }
 
 
-GCIdleTimeAction GCIdleTimeHandler::NothingOrDone() {
-  if (idle_times_which_made_no_progress_since_last_idle_round_ >=
-      kMaxNoProgressIdleTimesPerIdleRound) {
+// The idle time handler has three modes and transitions between them
+// as shown in the diagram:
+//
+//  kReduceLatency -----> kReduceMemory -----> kDone
+//      ^    ^                  |                |
+//      |    |                  |                |
+//      |    +------------------+                |
+//      |                                        |
+//      +----------------------------------------+
+//
+// In kReduceLatency mode the handler does only start incremental marking

[Erik Corry, 2015/05/05 21:38:52]

does only start -> only starts

[ulan, 2015/05/06 09:47:19]

Done.

+// if can_start_incremental_marking is false.

[Erik Corry, 2015/05/05 21:38:52]

This contradiction indicates to me that can_start_incremental_marking is
misnamed.

[ulan, 2015/05/06 09:47:19]

Is should_start_incremental_marking better? I can rename it in a separate CL.

[Erik Corry, 2015/05/06 09:57:29]

Yes, we can do it in a different CL.  I was thinking some name that explains why
we can start incremental, since we can start it for other reasons when this flag
is false.  allocated_enough_to_start_incremental, perhaps.

[ulan, 2015/05/07 10:33:30]

Acknowledged.

+// In kReduceMemory mode the handler can force a new GC cycle by starting
+// incremental marking even if can_start_incremental_marking is false. It can
+// cause at most X idle GCs.
+// In kDone mode the idle time handler does nothing.
+//
+// The initial mode is kReduceLatency.
+//
+// kReduceLatency => kReduceMemory transition happens if there were Y
+// consecutive long idle notifications without any mutator GC. This is our
+// notion of "mutator is idle".
+//
+// kReduceMemory => kDone transition happens after X idle GCs.

[Erik Corry, 2015/05/05 21:38:52]

Probably not for this CL, but in the longer run I would prefer a different, less
hard-coded rule:
We need to do some minimal number, perhaps 2.  After that I feel we should
continue as long as we manage to free up pages of memory.  That might be 0 more
GCs or a large number if we are gradually compacting large numbers of fragmented
pages.

[ulan, 2015/05/06 09:47:19]

I will follow up with CL that uses the 'reduce_memory' flag to do more
aggressive compaction. Let's check after that CL if 3 GCs are good enough or we
need smarter logic here.

+//
+// kReduceMemory => kReduceLatency transition happens if N mutator GCs

[Erik Corry, 2015/05/05 21:38:52]

Should N be 1?  If any allocation-provoked GCs are happening at all then the
mutator is active and we should move to latency mode.

[ulan, 2015/05/06 09:47:19]

This is to prevent back-and-forth transitions from latency mode to memory mode
if the mutator is almost idle: does a scavenge once in a while.

[Erik Corry, 2015/05/06 09:57:29]

I can see that makes sense.

+// were performed meaning that the mutator is active.
+//
+// kDone => kReduceLatency transition happens if there were M mutator GCs or
+// context was disposed.
+//
+// X = kMaxIdleMarkCompacts
+// Y = kLongIdleNotificationsBeforeMutatorIsIdle
+// N = #(idle GCs)
+// M = kGCsBeforeMutatorIsActive
+GCIdleTimeAction GCIdleTimeHandler::Compute(double idle_time_in_ms,
+                                            HeapState heap_state) {
+  Mode next_mode = NextMode(heap_state);
+
+  if (next_mode != mode_) {
+    mode_ = next_mode;
+    ResetCounters();
+  }
+
+  UpdateCounters(idle_time_in_ms);
+
+  if (mode_ == kDone) {
     return GCIdleTimeAction::Done();
   } else {
-    idle_times_which_made_no_progress_since_last_idle_round_++;
-    return GCIdleTimeAction::Nothing();
+    return Action(idle_time_in_ms, heap_state, mode_ == kReduceMemory);
   }
 }
 
@@ -204,28 +245,23 @@ GCIdleTimeAction GCIdleTimeHandler::NothingOrDone() {
 // a full GC.
 // (2) If the new space is almost full and we can afford a Scavenge or if the
 // next Scavenge will very likely take long, then a Scavenge is performed.
-// (3) If there is currently no MarkCompact idle round going on, we start a
-// new idle round if enough garbage was created. Otherwise we do not perform
-// garbage collection to keep system utilization low.
-// (4) If incremental marking is done, we perform a full garbage collection
+// (3) If incremental marking is done, we perform a full garbage collection
 // if  we are allowed to still do full garbage collections during this idle
 // round or if we are not allowed to start incremental marking. Otherwise we
 // do not perform garbage collection to keep system utilization low.
-// (5) If sweeping is in progress and we received a large enough idle time
+// (4) If sweeping is in progress and we received a large enough idle time
 // request, we finalize sweeping here.
-// (6) If incremental marking is in progress, we perform a marking step. Note,
+// (5) If incremental marking is in progress, we perform a marking step. Note,
 // that this currently may trigger a full garbage collection.
-GCIdleTimeAction GCIdleTimeHandler::Compute(double idle_time_in_ms,
-                                            HeapState heap_state) {
+GCIdleTimeAction GCIdleTimeHandler::Action(double idle_time_in_ms,
+                                           const HeapState& heap_state,
+                                           bool reduce_memory) {
   if (static_cast<int>(idle_time_in_ms) <= 0) {
-    if (heap_state.contexts_disposed > 0) {
-      StartIdleRound();
-    }
     if (heap_state.incremental_marking_stopped) {
       if (ShouldDoContextDisposalMarkCompact(
               heap_state.contexts_disposed,
               heap_state.contexts_disposal_rate)) {
-        return GCIdleTimeAction::FullGC();
+        return GCIdleTimeAction::FullGC(false);
       }
     }
     return GCIdleTimeAction::Nothing();
@@ -239,37 +275,92 @@ GCIdleTimeAction GCIdleTimeHandler::Compute(double idle_time_in_ms,
     return GCIdleTimeAction::Scavenge();
   }
 
-  if (IsMarkCompactIdleRoundFinished()) {
-    if (EnoughGarbageSinceLastIdleRound()) {
-      StartIdleRound();
-    } else {
-      return GCIdleTimeAction::Done();
-    }
-  }
-
-  if (heap_state.incremental_marking_stopped) {
+  if (heap_state.incremental_marking_stopped && reduce_memory) {
     if (ShouldDoMarkCompact(static_cast<size_t>(idle_time_in_ms),
                             heap_state.size_of_objects,
                             heap_state.mark_compact_speed_in_bytes_per_ms)) {
-      return GCIdleTimeAction::FullGC();
+      return GCIdleTimeAction::FullGC(reduce_memory);
     }
   }
+
   if (heap_state.sweeping_in_progress) {
     if (heap_state.sweeping_completed) {
       return GCIdleTimeAction::FinalizeSweeping();
     } else {
-      return NothingOrDone();
+      return GCIdleTimeAction::Nothing();
     }
   }
+
   if (heap_state.incremental_marking_stopped &&
-      !heap_state.can_start_incremental_marking) {
-    return NothingOrDone();
+      !heap_state.can_start_incremental_marking && !reduce_memory) {
+    return GCIdleTimeAction::Nothing();
   }
 
   size_t step_size = EstimateMarkingStepSize(
       static_cast<size_t>(kIncrementalMarkingStepTimeInMs),
       heap_state.incremental_marking_speed_in_bytes_per_ms);
-  return GCIdleTimeAction::IncrementalMarking(step_size);
+  return GCIdleTimeAction::IncrementalMarking(step_size, reduce_memory);
+}
+
+
+void GCIdleTimeHandler::UpdateCounters(double idle_time_in_ms) {
+  if (mode_ == kReduceLatency) {
+    int mutator_gcs = scavenges_ + mark_compacts_ - idle_mark_compacts_;
+    if (mutator_gcs > 0) {
+      ResetCounters();

[Erik Corry, 2015/05/05 21:38:52]

A few words on the intention here?  What I can see is that if scavenges and
total mark-compacts add up to more than just the incremental portion of the
mark-compacts we will zero all three counters, and also zero the
long_idle_notifications_ counter.  Why?  It looks like the scavenges_ count will
be zeroed on every idle notification, even a 1ms notification.

[ulan, 2015/05/06 09:47:19]

I refactored this to be more explicit and added description for
long_idle_notifications_ in the header:

// The number of long idle notifications with no mutator GC happening
// between the notifications.

+    }
+    long_idle_notifications_ += (idle_time_in_ms >= kMaxLongIdleTime)

[Hannes Payer (out of office), 2015/05/05 20:51:16]

This will never trigger, since idle_time_in_ms will always be smaller than
kMAxLongIdleTime because of the deadline based implementation.

[ulan, 2015/05/06 09:47:19]

Changed this to kLargeLongIdleTime.

+                                    ? kLongIdleNotificationsBeforeMutatorIsIdle
+                                    : (idle_time_in_ms >= kMinLongIdleTime);

[Erik Corry, 2015/05/05 21:38:52]

Implicit cast of a bool to 0 or 1 is not good style.  Instead you could wrap the
whole += in an "if" that checks whether the idle time is < kMinLongIdleTime

[ulan, 2015/05/06 09:47:19]

Done.

+  }
+}
+
+
+void GCIdleTimeHandler::ResetCounters() {
+  long_idle_notifications_ = 0;
+  idle_mark_compacts_ = 0;
+  mark_compacts_ = 0;
+  scavenges_ = 0;
+}
+
+
+bool GCIdleTimeHandler::IsMutatorActive(int contexts_disposed, int gcs) {
+  return contexts_disposed > 0 || gcs >= kGCsBeforeMutatorIsActive;
+}
+
+
+bool GCIdleTimeHandler::IsMutatorIdle(int long_idle_notifications, int gcs) {
+  return gcs == 0 &&
+         long_idle_notifications >= kLongIdleNotificationsBeforeMutatorIsIdle;
+}
+
+
+GCIdleTimeHandler::Mode GCIdleTimeHandler::NextMode(
+    const HeapState& heap_state) {
+  DCHECK(mark_compacts_ >= idle_mark_compacts_);
+  int mutator_gcs = scavenges_ + mark_compacts_ - idle_mark_compacts_;
+  switch (mode_) {
+    case kDone:
+      DCHECK(idle_mark_compacts_ == 0);
+      if (IsMutatorActive(heap_state.contexts_disposed, mutator_gcs)) {
+        return kReduceLatency;
+      }
+      break;
+    case kReduceLatency:
+      if (IsMutatorIdle(long_idle_notifications_, mutator_gcs)) {
+        return kReduceMemory;
+      }
+      break;
+    case kReduceMemory:
+      if (idle_mark_compacts_ >= kMaxIdleMarkCompacts) {
+        return kDone;
+      }
+      if (mutator_gcs > idle_mark_compacts_) {
+        return kReduceLatency;
+      }
+      break;
+  }
+  return mode_;
 }
 }
 }