Make idle notification cleanup less aggressive. Do not clean up on...

src/heap.cc

Index: src/heap.cc
===================================================================
--- src/heap.cc	(revision 5979)
+++ src/heap.cc	(working copy)
@@ -3757,19 +3757,31 @@
   static const int kIdlesBeforeScavenge = 4;
   static const int kIdlesBeforeMarkSweep = 7;
   static const int kIdlesBeforeMarkCompact = 8;
+  static const int kMaxIdleCount = 9;

[antonm, 2010/12/13 12:06:20]

do we need it at all?  maybe use something like kIdlesBeforeMarkCompact (+1)
instead?

[Mads Ager (chromium), 2010/12/13 12:12:58]

I'm just being paranoid to avoid overflows. I'll change it to
kIdlesBeforeMarkCompact + 1.

Thanks.

+  static const int kGCsBetweenCleanup = 4;
   static int number_idle_notifications = 0;
   static int last_gc_count = gc_count_;
 
   bool uncommit = true;
   bool finished = false;
 
-  if (last_gc_count == gc_count_) {
-    number_idle_notifications++;
+  // Reset the number of idle notifications received when a number of
+  // GCs have taken place. This allows another round of cleanup based
+  // on idle notifications if enough work has been carried out to
+  // provoke a number of garbage collections.
+  if (gc_count_ < last_gc_count + kGCsBetweenCleanup) {
+    number_idle_notifications =
+        Min(number_idle_notifications + 1, kMaxIdleCount);
   } else {
     number_idle_notifications = 0;
     last_gc_count = gc_count_;
   }
 
+  // If we have received more than kIdlesBeforeMarkCompact idle
+  // notifications we do not perform any cleanup because we don't
+  // expect to gain much by doing so.
+  if (number_idle_notifications > kIdlesBeforeMarkCompact) return true;

[Kasper Lund, 2010/12/13 11:45:44]

Have you consider getting rid of this code and just falling into the
contexts_disposed_ case? Somehow, I would be more comfortable with still getting
GCs on idle if we're actively disposing contexts. Maybe the call to
UncommitFromSpace isn't a bad idea either.

Maybe something like:

  } else if (contexts_disposed_ > 0) {
    ...
  } else {
    finished = true;
  }

would be better.

[Mads Ager (chromium), 2010/12/13 11:58:06]

Good idea. I have moved this to an else-if part that sets finished to true so it
fits the current model better.

+
   if (number_idle_notifications == kIdlesBeforeScavenge) {
     if (contexts_disposed_ > 0) {
       HistogramTimerScope scope(&Counters::gc_context);
@@ -3794,7 +3806,6 @@
     CollectAllGarbage(true);
     new_space_.Shrink();
     last_gc_count = gc_count_;
-    number_idle_notifications = 0;
     finished = true;
 
   } else if (contexts_disposed_ > 0) {