Start incremental marking on idle notification.

src/heap.cc

Index: src/heap.cc
diff --git a/src/heap.cc b/src/heap.cc
index 4da83e859e3ddff3f2988c00f066ab63ddf58c04..8e4c833a26cf3610bdee79e272ef54ad8ac2443f 100644
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -143,6 +143,7 @@ Heap::Heap()
       number_idle_notifications_(0),
       last_idle_notification_gc_count_(0),
       last_idle_notification_gc_count_init_(false),
+      idle_notification_will_schedule_next_gc_(false),
       configured_(false),
       chunks_queued_for_free_(NULL) {
   // Allow build-time customization of the max semispace size. Building
@@ -513,7 +514,8 @@ bool Heap::CollectGarbage(AllocationSpace space, GarbageCollector collector) {
   }
 
   ASSERT(collector == SCAVENGER || incremental_marking()->IsStopped());
-  if (incremental_marking()->IsStopped()) {
+  if (incremental_marking()->IsStopped() &&
+      !idle_notification_will_schedule_next_gc()) {

[Erik Corry, 2011/11/10 15:06:55]

I think if the heuristics say we need to start a GC then we should start a GC. 
Expecting the idle notification to schedule a GC is not reliable, because the
idle notifications may not arrive.

[ulan, 2011/11/11 13:27:26]

Removed the flag.

     if (incremental_marking()->WorthActivating() && NextGCIsLikelyToBeFull()) {
       incremental_marking()->Start();
     }
@@ -4461,17 +4463,88 @@ void Heap::EnsureHeapIsIterable() {
 
 
 bool Heap::IdleNotification() {

[Erik Corry, 2011/11/10 15:06:55]

A comment here to say what the return value means would be helpful.

[ulan, 2011/11/11 13:27:26]

This function is documented in V8 API. Added a comment saying that in header
file.

-  static const int kIdlesBeforeScavenge = 4;
-  static const int kIdlesBeforeMarkSweep = 7;
-  static const int kIdlesBeforeMarkCompact = 8;
-  static const int kMaxIdleCount = kIdlesBeforeMarkCompact + 1;
-  static const unsigned int kGCsBetweenCleanup = 4;
-
   if (!last_idle_notification_gc_count_init_) {
     last_idle_notification_gc_count_ = gc_count_;
     last_idle_notification_gc_count_init_ = true;
+    last_idle_notification_timestamp_ = OS::TimeCurrentMillis();
+  }
+
+  if (!FLAG_incremental_marking) return IdleGlobalGC();
+
+  // The goal is to perform kMaxIdleCount incremental GC cycles and then
+  // wait until the mutator creates more garbage.

[Erik Corry, 2011/11/10 15:06:55]

"Creates more garbage" is rather vague.  If this means that a certain number of
scavenges have taken place then we should write that.

[ulan, 2011/11/11 13:27:26]

Rephrased it to "enough garbage to justify a new round". Extracted this
condition into a separate predicate function. 

+  // A GC cycle consists of:
+  // 1. many incremental marking steps,
+  // 2. one old space mark-sweep-compact,
+  // 3. many lazy sweep steps.
+  // The counters from IdleGlobalGC are reused, but have different meaning:

[Erik Corry, 2011/11/10 15:06:55]

This seems unnecessarily complicated.  Counters are not expensive, lets give
them just one meaning.

[ulan, 2011/11/11 13:27:26]

Done.

+  // - number_idle_notifications_ counts the GC cycles.

[Erik Corry, 2011/11/10 15:06:55]

I do not like the phrase "GC cycles".  If we use the following phrases it is
clearer:

Scavenge
Full GC
Incremental GC (always full)
Nonincremental GC (always full)

[ulan, 2011/11/11 13:27:26]

Done.

+  // - last_idle_notification_gc_count_ stores the gc_count_ after the last
+  //   old space mark-sweep-compact.
+
+  if (number_idle_notifications_ >= kMaxIdleCount &&
+      gc_count_ - last_idle_notification_gc_count_ >= kGCsBetweenCleanup) {
+    // The mutator created more garbage, start new round of GC cycles.
+    number_idle_notifications_ = 0;
   }
 
+  intptr_t step_size = IncrementalMarking::kStepFakeAllocatedBytes;
+
+  double delay = OS::TimeCurrentMillis() - last_idle_notification_timestamp_;
+  last_idle_notification_timestamp_ += delay;
+
+  if (delay > 400) {
+    // Speed up if idle notifications are rare.

[Erik Corry, 2011/11/10 15:06:55]

Time-based things make debugging difficult, and in this case I cannot see that
it makes sense.  We should not try to autodetect the version of Chrome based on
the interval between the idle notifications, which is what this looks like.

[ulan, 2011/11/11 13:27:26]

As discussed offline, added a "hint" argument to IdleNotification and now using
it to compute the right size.

+    step_size *= 10;
+  }
+
+  if (incremental_marking()->IsStopped()) {
+    if (!old_data_space()->IsSweepingComplete() ||
+        !old_pointer_space()->IsSweepingComplete()) {

[Erik Corry, 2011/11/10 15:06:55]

It is a little dangerous to name individual spaces here.  If we later have
another space that is swept lazily then we will have to remember to update this
place.

[ulan, 2011/11/11 13:27:26]

Done.

+      bool sweeping_complete = old_data_space()->AdvanceSweeper(step_size);
+      sweeping_complete &= old_pointer_space()->AdvanceSweeper(step_size);
+      if (!sweeping_complete) {
+        return false;
+      }
+    }
+    if (!WorthStartingGCWhenIdle()) {
+      return true;
+    }
+    incremental_marking()->Start();
+  }
+
+  // This flag prevents incremental marking from requesting GC via stack guard
+  // and also prevents GC from starting a new incremental marking.
+  idle_notification_will_schedule_next_gc_ = true;
+
+  incremental_marking()->Step(step_size);

[Erik Corry, 2011/11/10 15:06:55]

It seems that we do a step without checking that we are currently in the middle
of incremental marking?

[ulan, 2011/11/11 13:27:26]

Line 4513 starts marking if it is not started already.

+
+  if (incremental_marking()->IsComplete()) {
+    bool uncommit = false;
+    if (last_idle_notification_gc_count_ - gc_count_ < kGCsBetweenCleanup / 2) {
+      // Mutator was idle since the the last GC caused by IdleNotification.
+      isolate_->compilation_cache()->Clear();

[Erik Corry, 2011/11/10 15:06:55]

We seem to be duplicating a lot of the logic of incremental GC here.  Clearing
specific caches, but leaving others alone, and calling CollectAllGarbage in
order to finish the incremental GC.  Can we factor that out into a common
routine with the other place(s) where we discover that incremental GC is nearly
done?

[ulan, 2011/11/11 13:27:26]

We do it in LowMemoryNotification and in old version of IdleNotification, but
they are not related to incremental marking.

+      uncommit = true;
+    }
+    CollectAllGarbage(kNoGCFlags);
+    if (uncommit) {
+      new_space_.Shrink();
+      UncommitFromSpace();
+    }
+    last_idle_notification_gc_count_ = gc_count_;
+    number_idle_notifications_++;
+  }
+
+  idle_notification_will_schedule_next_gc_ = false;
+
+  return !WorthStartingGCWhenIdle() &&

[Erik Corry, 2011/11/10 15:06:55]

If it is worth starting, should we not just start it, then the next line would
take care of this situation?

[ulan, 2011/11/11 13:27:26]

Now I am simply returning false, so the next notification will check the
conditions properly and start marking if needed.

+         incremental_marking()->IsStopped() &&
+         old_data_space()->IsSweepingComplete() &&

[Erik Corry, 2011/11/10 15:06:55]

Listing the lazily swept pages is fragile.

[ulan, 2011/11/11 13:27:26]

Done.

+         old_pointer_space()->IsSweepingComplete();
+}
+
+
+bool Heap::IdleGlobalGC() {
   bool uncommit = true;
   bool finished = false;