Start incremental marking on idle notification.

src/heap.cc

 // Copyright 2011 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 125 matching lines @@
       max_alive_after_gc_(0),
       min_in_mutator_(kMaxInt),
       alive_after_last_gc_(0),
       last_gc_end_timestamp_(0.0),
       store_buffer_(this),
       marking_(this),
       incremental_marking_(this),
       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
   // V8 with snapshots and a non-default max semispace size is much
   // easier if you can define it as part of the build environment.
 #if defined(V8_MAX_SEMISPACE_SIZE)
   max_semispace_size_ = reserved_semispace_size_ = V8_MAX_SEMISPACE_SIZE;
 #endif
 
   intptr_t max_virtual = OS::MaxVirtualMemory();
@@ skipping 350 matching lines @@
         : isolate_->counters()->gc_compactor();
     rate->Start();
     next_gc_likely_to_collect_more =
         PerformGarbageCollection(collector, &tracer);
     rate->Stop();
 
     GarbageCollectionEpilogue();
   }
 
   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();
     }
   }
 
   return next_gc_likely_to_collect_more;
 }
 
 
 void Heap::PerformScavenge() {
@@ skipping 3926 matching lines @@
 
 void Heap::EnsureHeapIsIterable() {
   ASSERT(IsAllocationAllowed());
   if (!IsHeapIterable()) {
     CollectAllGarbage(kMakeHeapIterableMask);
   }
   ASSERT(IsHeapIterable());
 }
 
 
 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;
 
   // 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_idle_notification_gc_count_ < kGCsBetweenCleanup) {
     number_idle_notifications_ =
         Min(number_idle_notifications_ + 1, kMaxIdleCount);
@@ skipping 1928 matching lines @@
   isolate_->heap()->store_buffer()->Compact();
   isolate_->heap()->store_buffer()->Filter(MemoryChunk::ABOUT_TO_BE_FREED);
   for (chunk = chunks_queued_for_free_; chunk != NULL; chunk = next) {
     next = chunk->next_chunk();
     isolate_->memory_allocator()->Free(chunk);
   }
   chunks_queued_for_free_ = NULL;
 }
 
 } }  // namespace v8::internal