Check if there is still time before finalizing an incremental collection.

BUG=
R=erik.corry@gmail.com, ulan@chromium.org

Review URL: https://codereview.chromium.org/629903003

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@24567 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

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

@@ -63,6 +63,9 @@ size_t GCIdleTimeHandler::EstimateMarkingStepSize(
 
 size_t GCIdleTimeHandler::EstimateMarkCompactTime(
     size_t size_of_objects, size_t mark_compact_speed_in_bytes_per_ms) {
+  // TODO(hpayer): Be more precise about the type of mark-compact event. It
+  // makes a huge difference if it is incremental or non-incremental and if
+  // compaction is happening.
   if (mark_compact_speed_in_bytes_per_ms == 0) {
     mark_compact_speed_in_bytes_per_ms = kInitialConservativeMarkCompactSpeed;
   }
@@ -71,7 +74,7 @@ size_t GCIdleTimeHandler::EstimateMarkCompactTime(
 }
 
 
-bool GCIdleTimeHandler::DoScavenge(
+bool GCIdleTimeHandler::ShouldDoScavenge(
     size_t idle_time_in_ms, size_t new_space_size, size_t used_new_space_size,
     size_t scavenge_speed_in_bytes_per_ms,
     size_t new_space_allocation_throughput_in_bytes_per_ms) {
@@ -110,6 +113,15 @@ bool GCIdleTimeHandler::DoScavenge(
 }
 
 
+bool GCIdleTimeHandler::ShouldDoMarkCompact(
+    size_t idle_time_in_ms, size_t size_of_objects,
+    size_t mark_compact_speed_in_bytes_per_ms) {
+  return idle_time_in_ms >=
+         EstimateMarkCompactTime(size_of_objects,
+                                 mark_compact_speed_in_bytes_per_ms);
+}
+
+
 // The following logic is implemented by the controller:
 // (1) If the new space is almost full and we can affort a Scavenge or if the
 // next Scavenge will very likely take long, then a Scavenge is performed.
@@ -128,10 +140,11 @@ bool GCIdleTimeHandler::DoScavenge(
 // that this currently may trigger a full garbage collection.
 GCIdleTimeAction GCIdleTimeHandler::Compute(size_t idle_time_in_ms,
                                             HeapState heap_state) {
-  if (DoScavenge(idle_time_in_ms, heap_state.new_space_capacity,
-                 heap_state.used_new_space_size,
-                 heap_state.scavenge_speed_in_bytes_per_ms,
-                 heap_state.new_space_allocation_throughput_in_bytes_per_ms)) {
+  if (ShouldDoScavenge(
+          idle_time_in_ms, heap_state.new_space_capacity,
+          heap_state.used_new_space_size,
+          heap_state.scavenge_speed_in_bytes_per_ms,
+          heap_state.new_space_allocation_throughput_in_bytes_per_ms)) {
     return GCIdleTimeAction::Scavenge();
   }
 
@@ -148,10 +161,8 @@ GCIdleTimeAction GCIdleTimeHandler::Compute(size_t idle_time_in_ms,
   }
 
   if (heap_state.incremental_marking_stopped) {
-    size_t estimated_time_in_ms =
-        EstimateMarkCompactTime(heap_state.size_of_objects,
-                                heap_state.mark_compact_speed_in_bytes_per_ms);
-    if (idle_time_in_ms >= estimated_time_in_ms ||
+    if (ShouldDoMarkCompact(idle_time_in_ms, heap_state.size_of_objects,
+                            heap_state.mark_compact_speed_in_bytes_per_ms) ||
         (heap_state.size_of_objects < kSmallHeapSize &&
          heap_state.contexts_disposed > 0)) {
       // If there are no more than two GCs left in this idle round and we are

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

@@ -155,7 +155,11 @@ class GCIdleTimeHandler {
   static size_t EstimateMarkCompactTime(
       size_t size_of_objects, size_t mark_compact_speed_in_bytes_per_ms);
 
-  static bool DoScavenge(
+  static bool ShouldDoMarkCompact(size_t idle_time_in_ms,
+                                  size_t size_of_objects,
+                                  size_t mark_compact_speed_in_bytes_per_ms);
+
+  static bool ShouldDoScavenge(
       size_t idle_time_in_ms, size_t new_space_size, size_t used_new_space_size,
       size_t scavenger_speed_in_bytes_per_ms,
       size_t new_space_allocation_throughput_in_bytes_per_ms);

src/heap/heap.cc

@@ -4268,11 +4268,14 @@ void Heap::MakeHeapIterable() {
 }
 
 
-void Heap::AdvanceIdleIncrementalMarking(intptr_t step_size) {
-  incremental_marking()->Step(step_size,
-                              IncrementalMarking::NO_GC_VIA_STACK_GUARD, true);
-
-  if (incremental_marking()->IsComplete()) {
+void Heap::TryFinalizeIdleIncrementalMarking(
+    size_t idle_time_in_ms, size_t size_of_objects,
+    size_t mark_compact_speed_in_bytes_per_ms) {
+  if (incremental_marking()->IsComplete() ||
+      (mark_compact_collector()->IsMarkingDequeEmpty() &&
+       gc_idle_time_handler_.ShouldDoMarkCompact(
+           idle_time_in_ms, size_of_objects,
+           mark_compact_speed_in_bytes_per_ms))) {
     bool uncommit = false;
     if (gc_count_at_last_idle_gc_ == gc_count_) {
       // No GC since the last full GC, the mutator is probably not active.
@@ -4332,16 +4335,28 @@ bool Heap::IdleNotification(int idle_time_in_ms) {
       gc_idle_time_handler_.Compute(idle_time_in_ms, heap_state);
 
   bool result = false;
+  int actual_time_in_ms = 0;
   switch (action.type) {
     case DONE:
       result = true;
       break;
-    case DO_INCREMENTAL_MARKING:
+    case DO_INCREMENTAL_MARKING: {
       if (incremental_marking()->IsStopped()) {
         incremental_marking()->Start();
       }
-      AdvanceIdleIncrementalMarking(action.parameter);
+      incremental_marking()->Step(action.parameter,
+                                  IncrementalMarking::NO_GC_VIA_STACK_GUARD,
+                                  IncrementalMarking::FORCE_MARKING,
+                                  IncrementalMarking::DO_NOT_FORCE_COMPLETION);
+      actual_time_in_ms = static_cast<int>(timer.Elapsed().InMilliseconds());
+      int remaining_idle_time_in_ms = idle_time_in_ms - actual_time_in_ms;
+      if (remaining_idle_time_in_ms > 0) {
+        TryFinalizeIdleIncrementalMarking(
+            remaining_idle_time_in_ms, heap_state.size_of_objects,
+            heap_state.mark_compact_speed_in_bytes_per_ms);
+      }
       break;
+    }
     case DO_FULL_GC: {
       HistogramTimerScope scope(isolate_->counters()->gc_context());
       const char* message = contexts_disposed_
@@ -4361,20 +4376,20 @@ bool Heap::IdleNotification(int idle_time_in_ms) {
       break;
   }
 
-  int actual_time_ms = static_cast<int>(timer.Elapsed().InMilliseconds());
-  if (actual_time_ms <= idle_time_in_ms) {
+  actual_time_in_ms = static_cast<int>(timer.Elapsed().InMilliseconds());
+  if (actual_time_in_ms <= idle_time_in_ms) {
     if (action.type != DONE && action.type != DO_NOTHING) {
       isolate()->counters()->gc_idle_time_limit_undershot()->AddSample(
-          idle_time_in_ms - actual_time_ms);
+          idle_time_in_ms - actual_time_in_ms);
     }
   } else {
     isolate()->counters()->gc_idle_time_limit_overshot()->AddSample(
-        actual_time_ms - idle_time_in_ms);
+        actual_time_in_ms - idle_time_in_ms);
   }
 
   if (FLAG_trace_idle_notification) {
     PrintF("Idle notification: requested idle time %d ms, actual time %d ms [",
-           idle_time_in_ms, actual_time_ms);
+           idle_time_in_ms, actual_time_in_ms);
     action.Print();
     PrintF("]\n");
   }

src/heap/heap.h

@@ -1949,7 +1949,9 @@ class Heap {
 
   void SelectScavengingVisitorsTable();
 
-  void AdvanceIdleIncrementalMarking(intptr_t step_size);
+  void TryFinalizeIdleIncrementalMarking(
+      size_t idle_time_in_ms, size_t size_of_objects,
+      size_t mark_compact_speed_in_bytes_per_ms);
 
   bool WorthActivatingIncrementalMarking();
 

src/heap/incremental-marking.cc

@@ -27,6 +27,7 @@ IncrementalMarking::IncrementalMarking(Heap* heap)
       should_hurry_(false),
       marking_speed_(0),
       allocated_(0),
+      idle_marking_delay_counter_(0),
       no_marking_scope_depth_(0),
       unscanned_bytes_of_large_object_(0) {}
 
@@ -891,24 +892,27 @@ void IncrementalMarking::SpeedUp() {
 }
 
 
-void IncrementalMarking::Step(intptr_t allocated_bytes, CompletionAction action,
-                              bool force_marking) {
+intptr_t IncrementalMarking::Step(intptr_t allocated_bytes,
+                                  CompletionAction action,
+                                  ForceMarkingAction marking,
+                                  ForceCompletionAction completion) {
   if (heap_->gc_state() != Heap::NOT_IN_GC || !FLAG_incremental_marking ||
       !FLAG_incremental_marking_steps ||
       (state_ != SWEEPING && state_ != MARKING)) {
-    return;
+    return 0;
   }
 
   allocated_ += allocated_bytes;
 
-  if (!force_marking && allocated_ < kAllocatedThreshold &&
+  if (marking == DO_NOT_FORCE_MARKING && allocated_ < kAllocatedThreshold &&
       write_barriers_invoked_since_last_step_ <
           kWriteBarriersInvokedThreshold) {
-    return;
+    return 0;
   }
 
-  if (state_ == MARKING && no_marking_scope_depth_ > 0) return;
+  if (state_ == MARKING && no_marking_scope_depth_ > 0) return 0;
 
+  intptr_t bytes_processed = 0;
   {
     HistogramTimerScope incremental_marking_scope(
         heap_->isolate()->counters()->gc_incremental_marking());
@@ -929,7 +933,6 @@ void IncrementalMarking::Step(intptr_t allocated_bytes, CompletionAction action,
     write_barriers_invoked_since_last_step_ = 0;
 
     bytes_scanned_ += bytes_to_process;
-    intptr_t bytes_processed = 0;
 
     if (state_ == SWEEPING) {
       if (heap_->mark_compact_collector()->sweeping_in_progress() &&
@@ -942,7 +945,14 @@ void IncrementalMarking::Step(intptr_t allocated_bytes, CompletionAction action,
       }
     } else if (state_ == MARKING) {
       bytes_processed = ProcessMarkingDeque(bytes_to_process);
-      if (marking_deque_.IsEmpty()) MarkingComplete(action);
+      if (marking_deque_.IsEmpty()) {
+        if (completion == FORCE_COMPLETION ||
+            IsIdleMarkingDelayCounterLimitReached()) {
+          MarkingComplete(action);
+        } else {
+          IncrementIdleMarkingDelayCounter();
+        }
+      }
     }
 
     steps_count_++;
@@ -958,6 +968,7 @@ void IncrementalMarking::Step(intptr_t allocated_bytes, CompletionAction action,
     // process the marking deque.
     heap_->tracer()->AddIncrementalMarkingStep(duration, bytes_processed);
   }
+  return bytes_processed;
 }
 
 
@@ -977,5 +988,20 @@ void IncrementalMarking::ResetStepCounters() {
 int64_t IncrementalMarking::SpaceLeftInOldSpace() {
   return heap_->MaxOldGenerationSize() - heap_->PromotedSpaceSizeOfObjects();
 }
+
+
+bool IncrementalMarking::IsIdleMarkingDelayCounterLimitReached() {
+  return idle_marking_delay_counter_ > kMaxIdleMarkingDelayCounter;
+}
+
+
+void IncrementalMarking::IncrementIdleMarkingDelayCounter() {
+  idle_marking_delay_counter_++;
+}
+
+
+void IncrementalMarking::ClearIdleMarkingDelayCounter() {
+  idle_marking_delay_counter_ = 0;
+}
 }
 }  // namespace v8::internal

src/heap/incremental-marking.h

@@ -20,6 +20,10 @@ class IncrementalMarking {
 
   enum CompletionAction { GC_VIA_STACK_GUARD, NO_GC_VIA_STACK_GUARD };
 
+  enum ForceMarkingAction { FORCE_MARKING, DO_NOT_FORCE_MARKING };
+
+  enum ForceCompletionAction { FORCE_COMPLETION, DO_NOT_FORCE_COMPLETION };
+
   explicit IncrementalMarking(Heap* heap);
 
   static void Initialize();
@@ -83,10 +87,15 @@ class IncrementalMarking {
   static const intptr_t kMarkingSpeedAccelleration = 2;
   static const intptr_t kMaxMarkingSpeed = 1000;
 
+  // This is the upper bound for how many times we allow finalization of
+  // incremental marking to be postponed.
+  static const size_t kMaxIdleMarkingDelayCounter = 3;
+
   void OldSpaceStep(intptr_t allocated);
 
-  void Step(intptr_t allocated, CompletionAction action,
-            bool force_marking = false);
+  intptr_t Step(intptr_t allocated, CompletionAction action,
+                ForceMarkingAction marking = DO_NOT_FORCE_MARKING,
+                ForceCompletionAction completion = FORCE_COMPLETION);
 
   inline void RestartIfNotMarking() {
     if (state_ == COMPLETE) {
@@ -165,6 +174,10 @@ class IncrementalMarking {
     unscanned_bytes_of_large_object_ = unscanned_bytes;
   }
 
+  void ClearIdleMarkingDelayCounter();
+
+  bool IsIdleMarkingDelayCounterLimitReached();
+
  private:
   int64_t SpaceLeftInOldSpace();
 
@@ -195,6 +208,8 @@ class IncrementalMarking {
 
   INLINE(void VisitObject(Map* map, HeapObject* obj, int size));
 
+  void IncrementIdleMarkingDelayCounter();
+
   Heap* heap_;
 
   State state_;
@@ -213,6 +228,7 @@ class IncrementalMarking {
   intptr_t bytes_scanned_;
   intptr_t allocated_;
   intptr_t write_barriers_invoked_since_last_step_;
+  size_t idle_marking_delay_counter_;
 
   int no_marking_scope_depth_;
 

src/heap/mark-compact.cc

@@ -862,6 +862,8 @@ void MarkCompactCollector::Finish() {
     Deoptimizer::DeoptimizeMarkedCode(isolate());
     have_code_to_deoptimize_ = false;
   }
+
+  heap_->incremental_marking()->ClearIdleMarkingDelayCounter();
 }
 
 
@@ -1937,6 +1939,11 @@ void MarkCompactCollector::MarkAllocationSite(AllocationSite* site) {
 }
 
 
+bool MarkCompactCollector::IsMarkingDequeEmpty() {
+  return marking_deque_.IsEmpty();
+}
+
+
 void MarkCompactCollector::MarkRoots(RootMarkingVisitor* visitor) {
   // Mark the heap roots including global variables, stack variables,
   // etc., and all objects reachable from them.

src/heap/mark-compact.h

@@ -657,6 +657,8 @@ class MarkCompactCollector {
   // to artificially keep AllocationSites alive for a time.
   void MarkAllocationSite(AllocationSite* site);
 
+  bool IsMarkingDequeEmpty();
+
  private:
   class SweeperTask;
 

test/cctest/test-heap.cc

@@ -2183,6 +2183,48 @@ TEST(ResetSharedFunctionInfoCountersDuringMarkSweep) {
 }
 
 
+TEST(IdleNotificationFinishMarking) {
+  i::FLAG_allow_natives_syntax = true;
+  CcTest::InitializeVM();
+  SimulateFullSpace(CcTest::heap()->old_pointer_space());
+  IncrementalMarking* marking = CcTest::heap()->incremental_marking();
+  marking->Abort();
+  marking->Start();
+
+  CHECK_EQ(CcTest::heap()->gc_count(), 0);
+
+  // TODO(hpayer): We cannot write proper unit test right now for heap.
+  // The ideal test would call kMaxIdleMarkingDelayCounter to test the
+  // marking delay counter.
+
+  // Perform a huge incremental marking step but don't complete marking.
+  intptr_t bytes_processed = 0;
+  do {
+    bytes_processed =
+        marking->Step(1 * MB, IncrementalMarking::NO_GC_VIA_STACK_GUARD,
+                      IncrementalMarking::FORCE_MARKING,
+                      IncrementalMarking::DO_NOT_FORCE_COMPLETION);
+    CHECK(!marking->IsIdleMarkingDelayCounterLimitReached());
+  } while (bytes_processed);
+
+  // The next invocations of incremental marking are not going to complete
+  // marking
+  // since the completion threshold is not reached
+  for (size_t i = 0; i < IncrementalMarking::kMaxIdleMarkingDelayCounter - 2;
+       i++) {
+    marking->Step(1 * MB, IncrementalMarking::NO_GC_VIA_STACK_GUARD,
+                  IncrementalMarking::FORCE_MARKING,
+                  IncrementalMarking::DO_NOT_FORCE_COMPLETION);
+    CHECK(!marking->IsIdleMarkingDelayCounterLimitReached());
+  }
+
+  // The next idle notification has to finish incremental marking.
+  const int kShortIdleTimeInMs = 1;
+  CcTest::isolate()->IdleNotification(kShortIdleTimeInMs);
+  CHECK_EQ(CcTest::heap()->gc_count(), 1);
+}
+
+
 // Test that HAllocateObject will always return an object in new-space.
 TEST(OptimizedAllocationAlwaysInNewSpace) {
   i::FLAG_allow_natives_syntax = true;

test/unittests/heap/gc-idle-time-handler-unittest.cc

@@ -112,7 +112,7 @@ TEST(GCIdleTimeHandler, EstimateMarkCompactTimeMax) {
 TEST_F(GCIdleTimeHandlerTest, DoScavengeEmptyNewSpace) {
   GCIdleTimeHandler::HeapState heap_state = DefaultHeapState();
   int idle_time_in_ms = 16;
-  EXPECT_FALSE(GCIdleTimeHandler::DoScavenge(
+  EXPECT_FALSE(GCIdleTimeHandler::ShouldDoScavenge(
       idle_time_in_ms, heap_state.new_space_capacity,
       heap_state.used_new_space_size, heap_state.scavenge_speed_in_bytes_per_ms,
       heap_state.new_space_allocation_throughput_in_bytes_per_ms));
@@ -123,7 +123,7 @@ TEST_F(GCIdleTimeHandlerTest, DoScavengeFullNewSpace) {
   GCIdleTimeHandler::HeapState heap_state = DefaultHeapState();
   heap_state.used_new_space_size = kNewSpaceCapacity;
   int idle_time_in_ms = 16;
-  EXPECT_TRUE(GCIdleTimeHandler::DoScavenge(
+  EXPECT_TRUE(GCIdleTimeHandler::ShouldDoScavenge(
       idle_time_in_ms, heap_state.new_space_capacity,
       heap_state.used_new_space_size, heap_state.scavenge_speed_in_bytes_per_ms,
       heap_state.new_space_allocation_throughput_in_bytes_per_ms));
@@ -135,7 +135,7 @@ TEST_F(GCIdleTimeHandlerTest, DoScavengeUnknownScavengeSpeed) {
   heap_state.used_new_space_size = kNewSpaceCapacity;
   heap_state.scavenge_speed_in_bytes_per_ms = 0;
   int idle_time_in_ms = 16;
-  EXPECT_FALSE(GCIdleTimeHandler::DoScavenge(
+  EXPECT_FALSE(GCIdleTimeHandler::ShouldDoScavenge(
       idle_time_in_ms, heap_state.new_space_capacity,
       heap_state.used_new_space_size, heap_state.scavenge_speed_in_bytes_per_ms,
       heap_state.new_space_allocation_throughput_in_bytes_per_ms));
@@ -147,7 +147,7 @@ TEST_F(GCIdleTimeHandlerTest, DoScavengeLowScavengeSpeed) {
   heap_state.used_new_space_size = kNewSpaceCapacity;
   heap_state.scavenge_speed_in_bytes_per_ms = 1 * KB;
   int idle_time_in_ms = 16;
-  EXPECT_FALSE(GCIdleTimeHandler::DoScavenge(
+  EXPECT_FALSE(GCIdleTimeHandler::ShouldDoScavenge(
       idle_time_in_ms, heap_state.new_space_capacity,
       heap_state.used_new_space_size, heap_state.scavenge_speed_in_bytes_per_ms,
       heap_state.new_space_allocation_throughput_in_bytes_per_ms));
@@ -159,13 +159,26 @@ TEST_F(GCIdleTimeHandlerTest, DoScavengeHighScavengeSpeed) {
   heap_state.used_new_space_size = kNewSpaceCapacity;
   heap_state.scavenge_speed_in_bytes_per_ms = kNewSpaceCapacity;
   int idle_time_in_ms = 16;
-  EXPECT_TRUE(GCIdleTimeHandler::DoScavenge(
+  EXPECT_TRUE(GCIdleTimeHandler::ShouldDoScavenge(
       idle_time_in_ms, heap_state.new_space_capacity,
       heap_state.used_new_space_size, heap_state.scavenge_speed_in_bytes_per_ms,
       heap_state.new_space_allocation_throughput_in_bytes_per_ms));
 }
 
 
+TEST_F(GCIdleTimeHandlerTest, ShouldDoMarkCompact) {
+  size_t idle_time_in_ms = 16;
+  EXPECT_TRUE(GCIdleTimeHandler::ShouldDoMarkCompact(idle_time_in_ms, 0, 0));
+}
+
+
+TEST_F(GCIdleTimeHandlerTest, DontDoMarkCompact) {
+  size_t idle_time_in_ms = 1;
+  EXPECT_FALSE(GCIdleTimeHandler::ShouldDoMarkCompact(
+      idle_time_in_ms, kSizeOfObjects, kMarkingSpeed));
+}
+
+
 TEST_F(GCIdleTimeHandlerTest, AfterContextDisposeLargeIdleTime) {
   GCIdleTimeHandler::HeapState heap_state = DefaultHeapState();
   heap_state.contexts_disposed = 1;