[heap] Simplify idle notification handler

This merges the "do-nothing" case with the "done" case as the former
is no longer useful. This also fixes a bug where the idle time handler
would not make progress by always returning "do-nothing".

Change-Id: Ibdd3189e4fd35acc5405aa82a13ea8ee2fd74cc6
Reviewed-on: https://chromium-review.googlesource.com/c/1478695
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59758}

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

@@ -17,27 +17,6 @@ const double GCIdleTimeHandler::kHighContextDisposalRate = 100;
 const size_t GCIdleTimeHandler::kMinTimeForOverApproximatingWeakClosureInMs = 1;
 
 
-void GCIdleTimeAction::Print() {
-  switch (type) {
-    case DONE:
-      PrintF("done");
-      break;
-    case DO_NOTHING:
-      PrintF("no action");
-      break;
-    case DO_INCREMENTAL_STEP:
-      PrintF("incremental step");
-      if (additional_work) {
-        PrintF("; finalized marking");
-      }
-      break;
-    case DO_FULL_GC:
-      PrintF("full GC");
-      break;
-  }
-}
-
-
 void GCIdleTimeHeapState::Print() {
   PrintF("contexts_disposed=%d ", contexts_disposed);
   PrintF("contexts_disposal_rate=%f ", contexts_disposal_rate);
@@ -96,19 +75,6 @@ bool GCIdleTimeHandler::ShouldDoOverApproximateWeakClosure(
 }
 
 
-GCIdleTimeAction GCIdleTimeHandler::NothingOrDone(double idle_time_in_ms) {
-  if (idle_time_in_ms >= kMinBackgroundIdleTime) {
-    return GCIdleTimeAction::Nothing();
-  }
-  if (idle_times_which_made_no_progress_ >= kMaxNoProgressIdleTimes) {
-    return GCIdleTimeAction::Done();
-  } else {
-    idle_times_which_made_no_progress_++;
-    return GCIdleTimeAction::Nothing();
-  }
-}
-
-
 // The following logic is implemented by the controller:
 // (1) If we don't have any idle time, do nothing, unless a context was
 // disposed, incremental marking is stopped, and the heap is small. Then do
@@ -128,25 +94,17 @@ GCIdleTimeAction GCIdleTimeHandler::Compute(double idle_time_in_ms,
       if (ShouldDoContextDisposalMarkCompact(heap_state.contexts_disposed,
                                              heap_state.contexts_disposal_rate,
                                              heap_state.size_of_objects)) {
-        return GCIdleTimeAction::FullGC();
+        return GCIdleTimeAction::kFullGC;
       }
     }
-    return GCIdleTimeAction::Nothing();
-  }
-
-  // We are in a context disposal GC scenario. Don't do anything if we do not
-  // get the right idle signal.
-  if (ShouldDoContextDisposalMarkCompact(heap_state.contexts_disposed,
-                                         heap_state.contexts_disposal_rate,
-                                         heap_state.size_of_objects)) {
-    return NothingOrDone(idle_time_in_ms);
+    return GCIdleTimeAction::kDone;
   }
 
-  if (!FLAG_incremental_marking || heap_state.incremental_marking_stopped) {
-    return GCIdleTimeAction::Done();
+  if (FLAG_incremental_marking && !heap_state.incremental_marking_stopped) {
+    return GCIdleTimeAction::kIncrementalStep;
   }
 
-  return GCIdleTimeAction::IncrementalStep();
+  return GCIdleTimeAction::kDone;
 }
 
 bool GCIdleTimeHandler::Enabled() { return FLAG_incremental_marking; }

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

@@ -10,51 +10,12 @@
 namespace v8 {
 namespace internal {
 
-enum GCIdleTimeActionType {
-  DONE,
-  DO_NOTHING,
-  DO_INCREMENTAL_STEP,
-  DO_FULL_GC,
+enum class GCIdleTimeAction : uint8_t {
+  kDone,
+  kIncrementalStep,
+  kFullGC,
 };
 
-
-class GCIdleTimeAction {
- public:
-  static GCIdleTimeAction Done() {
-    GCIdleTimeAction result;
-    result.type = DONE;
-    result.additional_work = false;
-    return result;
-  }
-
-  static GCIdleTimeAction Nothing() {
-    GCIdleTimeAction result;
-    result.type = DO_NOTHING;
-    result.additional_work = false;
-    return result;
-  }
-
-  static GCIdleTimeAction IncrementalStep() {
-    GCIdleTimeAction result;
-    result.type = DO_INCREMENTAL_STEP;
-    result.additional_work = false;
-    return result;
-  }
-
-  static GCIdleTimeAction FullGC() {
-    GCIdleTimeAction result;
-    result.type = DO_FULL_GC;
-    result.additional_work = false;
-    return result;
-  }
-
-  void Print();
-
-  GCIdleTimeActionType type;
-  bool additional_work;
-};
-
-
 class GCIdleTimeHeapState {
  public:
   void Print();
@@ -117,20 +78,13 @@ class V8_EXPORT_PRIVATE GCIdleTimeHandler {
 
   static const size_t kMinTimeForOverApproximatingWeakClosureInMs;
 
-  // Number of times we will return a Nothing action in the current mode
-  // despite having idle time available before we returning a Done action to
-  // ensure we don't keep scheduling idle tasks and making no progress.
-  static const int kMaxNoProgressIdleTimes = 10;
-
-  GCIdleTimeHandler() : idle_times_which_made_no_progress_(0) {}
+  GCIdleTimeHandler() = default;
 
   GCIdleTimeAction Compute(double idle_time_in_ms,
                            GCIdleTimeHeapState heap_state);
 
   bool Enabled();
 
-  void ResetNoProgressCounter() { idle_times_which_made_no_progress_ = 0; }
-
   static size_t EstimateMarkingStepSize(double idle_time_in_ms,
                                         double marking_speed_in_bytes_per_ms);
 
@@ -148,11 +102,6 @@ class V8_EXPORT_PRIVATE GCIdleTimeHandler {
   static bool ShouldDoOverApproximateWeakClosure(double idle_time_in_ms);
 
  private:
-  GCIdleTimeAction NothingOrDone(double idle_time_in_ms);
-
-  // Idle notifications with no progress.
-  int idle_times_which_made_no_progress_;
-
   DISALLOW_COPY_AND_ASSIGN(GCIdleTimeHandler);
 };
 

src/heap/heap.cc

@@ -1489,7 +1489,6 @@ void Heap::StartIncrementalMarkingIfAllocationLimitIsReached(
 void Heap::StartIdleIncrementalMarking(
     GarbageCollectionReason gc_reason,
     const GCCallbackFlags gc_callback_flags) {
-  gc_idle_time_handler_->ResetNoProgressCounter();
   StartIncrementalMarking(kReduceMemoryFootprintMask, gc_reason,
                           gc_callback_flags);
 }
@@ -3189,11 +3188,11 @@ bool Heap::PerformIdleTimeAction(GCIdleTimeAction action,
                                  GCIdleTimeHeapState heap_state,
                                  double deadline_in_ms) {
   bool result = false;
-  switch (action.type) {
-    case DONE:
+  switch (action) {
+    case GCIdleTimeAction::kDone:
       result = true;
       break;
-    case DO_INCREMENTAL_STEP: {
+    case GCIdleTimeAction::kIncrementalStep: {
       incremental_marking()->AdvanceWithDeadline(
           deadline_in_ms, IncrementalMarking::NO_GC_VIA_STACK_GUARD,
           StepOrigin::kTask);
@@ -3202,15 +3201,13 @@ bool Heap::PerformIdleTimeAction(GCIdleTimeAction action,
       result = incremental_marking()->IsStopped();
       break;
     }
-    case DO_FULL_GC: {
+    case GCIdleTimeAction::kFullGC: {
       DCHECK_LT(0, contexts_disposed_);
       HistogramTimerScope scope(isolate_->counters()->gc_context());
       TRACE_EVENT0("v8", "V8.GCContext");
       CollectAllGarbage(kNoGCFlags, GarbageCollectionReason::kContextDisposal);
       break;
     }
-    case DO_NOTHING:
-      break;
   }
 
   return result;
@@ -3226,14 +3223,23 @@ void Heap::IdleNotificationEpilogue(GCIdleTimeAction action,
 
   contexts_disposed_ = 0;
 
-  if ((FLAG_trace_idle_notification && action.type > DO_NOTHING) ||
-      FLAG_trace_idle_notification_verbose) {
+  if (FLAG_trace_idle_notification) {
     isolate_->PrintWithTimestamp(
         "Idle notification: requested idle time %.2f ms, used idle time %.2f "
         "ms, deadline usage %.2f ms [",
         idle_time_in_ms, idle_time_in_ms - deadline_difference,
         deadline_difference);
-    action.Print();
+    switch (action) {
+      case GCIdleTimeAction::kDone:
+        PrintF("done");
+        break;
+      case GCIdleTimeAction::kIncrementalStep:
+        PrintF("incremental step");
+        break;
+      case GCIdleTimeAction::kFullGC:
+        PrintF("full GC");
+        break;
+    }
     PrintF("]");
     if (FLAG_trace_idle_notification_verbose) {
       PrintF("[");

src/heap/heap.h

@@ -58,7 +58,6 @@ class ArrayBufferCollector;
 class ArrayBufferTracker;
 class CodeLargeObjectSpace;
 class ConcurrentMarking;
-class GCIdleTimeAction;
 class GCIdleTimeHandler;
 class GCIdleTimeHeapState;
 class GCTracer;
@@ -145,6 +144,8 @@ enum class YoungGenerationHandling {
   // Also update src/tools/metrics/histograms/histograms.xml in chromium.
 };
 
+enum class GCIdleTimeAction : uint8_t;
+
 class AllocationResult {
  public:
   static inline AllocationResult Retry(AllocationSpace space = NEW_SPACE) {

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

@@ -31,7 +31,6 @@ class GCIdleTimeHandlerTest : public ::testing::Test {
   static const size_t kSizeOfObjects = 100 * MB;
   static const size_t kMarkCompactSpeed = 200 * KB;
   static const size_t kMarkingSpeed = 200 * KB;
-  static const int kMaxNotifications = 100;
 
  private:
   GCIdleTimeHandler handler_;
@@ -95,8 +94,8 @@ TEST_F(GCIdleTimeHandlerTest, ContextDisposeLowRate) {
   heap_state.contexts_disposed = 1;
   heap_state.incremental_marking_stopped = true;
   double idle_time_ms = 0;
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DO_NOTHING, action.type);
+  EXPECT_EQ(GCIdleTimeAction::kDone,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 
@@ -108,8 +107,8 @@ TEST_F(GCIdleTimeHandlerTest, ContextDisposeHighRate) {
       GCIdleTimeHandler::kHighContextDisposalRate - 1;
   heap_state.incremental_marking_stopped = true;
   double idle_time_ms = 0;
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DO_FULL_GC, action.type);
+  EXPECT_EQ(GCIdleTimeAction::kFullGC,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 
@@ -120,8 +119,8 @@ TEST_F(GCIdleTimeHandlerTest, AfterContextDisposeZeroIdleTime) {
   heap_state.contexts_disposal_rate = 1.0;
   heap_state.incremental_marking_stopped = true;
   double idle_time_ms = 0;
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DO_FULL_GC, action.type);
+  EXPECT_EQ(GCIdleTimeAction::kFullGC,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 
@@ -133,8 +132,8 @@ TEST_F(GCIdleTimeHandlerTest, AfterContextDisposeSmallIdleTime1) {
       GCIdleTimeHandler::kHighContextDisposalRate;
   size_t speed = kMarkCompactSpeed;
   double idle_time_ms = static_cast<double>(kSizeOfObjects / speed - 1);
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DO_INCREMENTAL_STEP, action.type);
+  EXPECT_EQ(GCIdleTimeAction::kIncrementalStep,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 
@@ -146,8 +145,8 @@ TEST_F(GCIdleTimeHandlerTest, AfterContextDisposeSmallIdleTime2) {
       GCIdleTimeHandler::kHighContextDisposalRate;
   size_t speed = kMarkCompactSpeed;
   double idle_time_ms = static_cast<double>(kSizeOfObjects / speed - 1);
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DO_INCREMENTAL_STEP, action.type);
+  EXPECT_EQ(GCIdleTimeAction::kIncrementalStep,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 TEST_F(GCIdleTimeHandlerTest, AfterContextDisposeLargeHeap) {
@@ -158,16 +157,16 @@ TEST_F(GCIdleTimeHandlerTest, AfterContextDisposeLargeHeap) {
   heap_state.incremental_marking_stopped = true;
   heap_state.size_of_objects = 101 * MB;
   double idle_time_ms = 0;
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DO_NOTHING, action.type);
+  EXPECT_EQ(GCIdleTimeAction::kDone,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 TEST_F(GCIdleTimeHandlerTest, IncrementalMarking1) {
   if (!handler()->Enabled()) return;
   GCIdleTimeHeapState heap_state = DefaultHeapState();
   double idle_time_ms = 10;
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DO_INCREMENTAL_STEP, action.type);
+  EXPECT_EQ(GCIdleTimeAction::kIncrementalStep,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 
@@ -177,8 +176,8 @@ TEST_F(GCIdleTimeHandlerTest, NotEnoughTime) {
   heap_state.incremental_marking_stopped = true;
   size_t speed = kMarkCompactSpeed;
   double idle_time_ms = static_cast<double>(kSizeOfObjects / speed - 1);
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DONE, action.type);
+  EXPECT_EQ(GCIdleTimeAction::kDone,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 
@@ -187,8 +186,8 @@ TEST_F(GCIdleTimeHandlerTest, DoNotStartIncrementalMarking) {
   GCIdleTimeHeapState heap_state = DefaultHeapState();
   heap_state.incremental_marking_stopped = true;
   double idle_time_ms = 10.0;
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DONE, action.type);
+  EXPECT_EQ(GCIdleTimeAction::kDone,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 
@@ -197,32 +196,11 @@ TEST_F(GCIdleTimeHandlerTest, ContinueAfterStop) {
   GCIdleTimeHeapState heap_state = DefaultHeapState();
   heap_state.incremental_marking_stopped = true;
   double idle_time_ms = 10.0;
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DONE, action.type);
+  EXPECT_EQ(GCIdleTimeAction::kDone,
+            handler()->Compute(idle_time_ms, heap_state));
   heap_state.incremental_marking_stopped = false;
-  action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DO_INCREMENTAL_STEP, action.type);
-}
-
-
-TEST_F(GCIdleTimeHandlerTest, ZeroIdleTimeNothingToDo) {
-  if (!handler()->Enabled()) return;
-  GCIdleTimeHeapState heap_state = DefaultHeapState();
-  for (int i = 0; i < kMaxNotifications; i++) {
-    GCIdleTimeAction action = handler()->Compute(0, heap_state);
-    EXPECT_EQ(DO_NOTHING, action.type);
-  }
-}
-
-
-TEST_F(GCIdleTimeHandlerTest, SmallIdleTimeNothingToDo) {
-  if (!handler()->Enabled()) return;
-  GCIdleTimeHeapState heap_state = DefaultHeapState();
-  heap_state.incremental_marking_stopped = true;
-  for (int i = 0; i < kMaxNotifications; i++) {
-    GCIdleTimeAction action = handler()->Compute(10, heap_state);
-    EXPECT_TRUE(DO_NOTHING == action.type || DONE == action.type);
-  }
+  EXPECT_EQ(GCIdleTimeAction::kIncrementalStep,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 
@@ -235,9 +213,9 @@ TEST_F(GCIdleTimeHandlerTest, DoneIfNotMakingProgressOnIncrementalMarking) {
   // Simulate incremental marking stopped and not eligible to start.
   heap_state.incremental_marking_stopped = true;
   double idle_time_ms = 10.0;
-  // We should return DONE if we cannot start incremental marking.
-  GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
-  EXPECT_EQ(DONE, action.type);
+  // We should return kDone if we cannot start incremental marking.
+  EXPECT_EQ(GCIdleTimeAction::kDone,
+            handler()->Compute(idle_time_ms, heap_state));
 }
 
 }  // namespace internal