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

src/heap.cc

 // Copyright 2010 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 3739 matching lines @@
   }
   Struct::cast(result)->InitializeBody(size);
   return result;
 }
 
 
 bool Heap::IdleNotification() {
   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);
       CollectAllGarbage(false);
     } else {
       CollectGarbage(NEW_SPACE);
     }
     new_space_.Shrink();
     last_gc_count = gc_count_;
 
   } else if (number_idle_notifications == kIdlesBeforeMarkSweep) {
     // Before doing the mark-sweep collections we clear the
     // compilation cache to avoid hanging on to source code and
     // generated code for cached functions.
     CompilationCache::Clear();
 
     CollectAllGarbage(false);
     new_space_.Shrink();
     last_gc_count = gc_count_;
 
   } else if (number_idle_notifications == kIdlesBeforeMarkCompact) {
     CollectAllGarbage(true);
     new_space_.Shrink();
     last_gc_count = gc_count_;
-    number_idle_notifications = 0;
     finished = true;
 
   } else if (contexts_disposed_ > 0) {
     if (FLAG_expose_gc) {
       contexts_disposed_ = 0;
     } else {
       HistogramTimerScope scope(&Counters::gc_context);
       CollectAllGarbage(false);
       last_gc_count = gc_count_;
     }
@@ skipping 1630 matching lines @@
 void ExternalStringTable::TearDown() {
   new_space_strings_.Free();
   old_space_strings_.Free();
 }
 
 
 List<Object*> ExternalStringTable::new_space_strings_;
 List<Object*> ExternalStringTable::old_space_strings_;
 
 } }  // namespace v8::internal