[heap] Remove incremental marking finalization step

Remove finalization step of incremental marking. The step was
historically used to process embedder/weak work on the main thread
before invoking the atomic pause. Remove the infrastructure as the
step is not needed anymore and actually required a safepoint.

Change-Id: I208767bbac3d9a06a0b3c67aa9779f8a5fa07328
Bug: v8:12775
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3702801
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81234}

src/heap/gc-tracer.cc

@@ -907,7 +907,6 @@ void GCTracer::PrintNVP() const {
           "sweep.old=%.1f "
           "incremental=%.1f "
           "incremental.finalize=%.1f "
-          "incremental.finalize.body=%.1f "
           "incremental.finalize.external.prologue=%.1f "
           "incremental.finalize.external.epilogue=%.1f "
           "incremental.layout_change=%.1f "
@@ -916,8 +915,6 @@ void GCTracer::PrintNVP() const {
           "incremental.embedder_prologue=%.1f "
           "incremental.embedder_tracing=%.1f "
           "incremental_wrapper_tracing_longest_step=%.1f "
-          "incremental_finalize_longest_step=%.1f "
-          "incremental_finalize_steps_count=%d "
           "incremental_longest_step=%.1f "
           "incremental_steps_count=%d "
           "incremental_marking_throughput=%.f "
@@ -994,7 +991,6 @@ void GCTracer::PrintNVP() const {
           current_scope(Scope::MC_SWEEP_OLD),
           current_scope(Scope::MC_INCREMENTAL),
           current_scope(Scope::MC_INCREMENTAL_FINALIZE),
-          current_scope(Scope::MC_INCREMENTAL_FINALIZE_BODY),
           current_scope(Scope::MC_INCREMENTAL_EXTERNAL_PROLOGUE),
           current_scope(Scope::MC_INCREMENTAL_EXTERNAL_EPILOGUE),
           current_scope(Scope::MC_INCREMENTAL_LAYOUT_CHANGE),
@@ -1004,8 +1000,6 @@ void GCTracer::PrintNVP() const {
           current_scope(Scope::MC_INCREMENTAL_EMBEDDER_TRACING),
           incremental_scope(Scope::MC_INCREMENTAL_EMBEDDER_TRACING)
               .longest_step,
-          incremental_scope(Scope::MC_INCREMENTAL_FINALIZE_BODY).longest_step,
-          incremental_scope(Scope::MC_INCREMENTAL_FINALIZE_BODY).steps,
           incremental_scope(Scope::MC_INCREMENTAL).longest_step,
           incremental_scope(Scope::MC_INCREMENTAL).steps,
           IncrementalMarkingSpeedInBytesPerMillisecond(),

src/heap/heap.cc

@@ -469,7 +469,7 @@ GarbageCollector Heap::SelectGarbageCollector(AllocationSpace space,
     return GarbageCollector::MARK_COMPACTOR;
   }
 
-  if (incremental_marking()->NeedsFinalization() &&
+  if (incremental_marking()->IsComplete() &&
       AllocationLimitOvershotByLargeMargin()) {
     *reason = "Incremental marking needs finalization";
     return GarbageCollector::MARK_COMPACTOR;
@@ -1529,17 +1529,10 @@ void Heap::HandleGCRequest() {
     CheckMemoryPressure();
   } else if (CollectionRequested()) {
     CheckCollectionRequested();
-  } else if (incremental_marking()->request_type() ==
-             IncrementalMarking::GCRequestType::COMPLETE_MARKING) {
+  } else if (incremental_marking()->IsComplete()) {
     CollectAllGarbage(current_gc_flags_,
                       GarbageCollectionReason::kFinalizeMarkingViaStackGuard,
                       current_gc_callback_flags_);
-  } else if (incremental_marking()->request_type() ==
-                 IncrementalMarking::GCRequestType::FINALIZATION &&
-             incremental_marking()->IsMarking() &&
-             !incremental_marking()->finalize_marking_completed()) {
-    FinalizeIncrementalMarkingIncrementally(
-        GarbageCollectionReason::kFinalizeMarkingViaStackGuard);
   }
 }
 
@@ -3821,17 +3814,10 @@ size_t Heap::NewSpaceCapacity() {
 
 void Heap::FinalizeIncrementalMarkingIfComplete(
     GarbageCollectionReason gc_reason) {
-  if (incremental_marking()->IsMarking() &&
-      (incremental_marking()->IsReadyToOverApproximateWeakClosure() ||
-       (!incremental_marking()->finalize_marking_completed() &&
-        mark_compact_collector()->local_marking_worklists()->IsEmpty() &&
-        local_embedder_heap_tracer()->ShouldFinalizeIncrementalMarking()))) {
-    FinalizeIncrementalMarkingIncrementally(gc_reason);
-  } else if (incremental_marking()->IsComplete() ||
-             (incremental_marking()->IsMarking() &&
-              mark_compact_collector()->local_marking_worklists()->IsEmpty() &&
-              local_embedder_heap_tracer()
-                  ->ShouldFinalizeIncrementalMarking())) {
+  if (incremental_marking()->IsComplete() ||
+      (incremental_marking()->IsMarking() &&
+       mark_compact_collector()->local_marking_worklists()->IsEmpty() &&
+       local_embedder_heap_tracer()->ShouldFinalizeIncrementalMarking())) {
     CollectAllGarbage(current_gc_flags_, gc_reason, current_gc_callback_flags_);
   }
 }
@@ -3864,31 +3850,6 @@ void Heap::InvokeIncrementalMarkingEpilogueCallbacks() {
   }
 }
 
-void Heap::FinalizeIncrementalMarkingIncrementally(
-    GarbageCollectionReason gc_reason) {
-  if (FLAG_trace_incremental_marking) {
-    isolate()->PrintWithTimestamp(
-        "[IncrementalMarking] (%s).\n",
-        Heap::GarbageCollectionReasonToString(gc_reason));
-  }
-
-  DevToolsTraceEventScope devtools_trace_event_scope(
-      this, "MajorGC", "incremental finalization step");
-
-  NestedTimedHistogramScope incremental_marking_scope(
-      isolate()->counters()->gc_incremental_marking_finalize());
-  TRACE_EVENT1(
-      "v8", "V8.GCIncrementalMarkingFinalize", "epoch",
-      tracer()->CurrentEpoch(GCTracer::Scope::MC_INCREMENTAL_FINALIZE));
-  TRACE_GC_EPOCH(tracer(), GCTracer::Scope::MC_INCREMENTAL_FINALIZE,
-                 ThreadKind::kMain);
-
-  IgnoreLocalGCRequests ignore_gc_requests(this);
-  InvokeIncrementalMarkingPrologueCallbacks();
-  incremental_marking()->FinalizeIncrementally();
-  InvokeIncrementalMarkingEpilogueCallbacks();
-}
-
 void Heap::NotifyObjectLayoutChange(
     HeapObject object, const DisallowGarbageCollection&,
     InvalidateRecordedSlots invalidate_recorded_slots, int new_size) {
@@ -5461,7 +5422,7 @@ bool Heap::ShouldExpandOldGenerationOnSlowAllocation(LocalHeap* local_heap) {
 
   if (ShouldOptimizeForLoadTime()) return true;
 
-  if (incremental_marking()->NeedsFinalization()) {
+  if (incremental_marking()->IsComplete()) {
     return !AllocationLimitOvershotByLargeMargin();
   }
 

src/heap/heap.h

@@ -1907,13 +1907,6 @@ class Heap {
 
   void ComputeFastPromotionMode();
 
-  // Attempt to over-approximate the weak closure by marking object groups and
-  // implicit references from global handles, but don't atomically complete
-  // marking. If we continue to mark incrementally, we might have marked
-  // objects that die later.
-  void FinalizeIncrementalMarkingIncrementally(
-      GarbageCollectionReason gc_reason);
-
   void InvokeIncrementalMarkingPrologueCallbacks();
   void InvokeIncrementalMarkingEpilogueCallbacks();
 

src/heap/incremental-marking-job.cc

@@ -129,7 +129,7 @@ void IncrementalMarkingJob::Task::RunInternal() {
     StepResult step_result = Step(heap);
     if (!incremental_marking->IsStopped()) {
       const TaskType task_type =
-          incremental_marking->finalize_marking_completed() ||
+          incremental_marking->IsComplete() ||
                   step_result != StepResult::kNoImmediateWork
               ? TaskType::kNormal
               : TaskType::kDelayed;

src/heap/incremental-marking.cc

@@ -148,8 +148,8 @@ void IncrementalMarking::Start(GarbageCollectionReason gc_reason) {
                                          : global_limit_mb - global_size_mb);
   }
   DCHECK(FLAG_incremental_marking);
-  DCHECK(state_ == STOPPED);
-  DCHECK(heap_->gc_state() == Heap::NOT_IN_GC);
+  DCHECK_EQ(state_, STOPPED);
+  DCHECK_EQ(heap_->gc_state(), Heap::NOT_IN_GC);
   DCHECK(!heap_->isolate()->serializer_enabled());
 
   Counters* counters = heap_->isolate()->counters();
@@ -363,20 +363,6 @@ void IncrementalMarking::EnsureBlackAllocated(Address allocated, size_t size) {
   }
 }
 
-void IncrementalMarking::FinalizeIncrementally() {
-  TRACE_GC(heap()->tracer(), GCTracer::Scope::MC_INCREMENTAL_FINALIZE_BODY);
-  DCHECK(!finalize_marking_completed_);
-  DCHECK(IsMarking());
-
-  // TODO(v8:12775): Remove the finalization step.
-  finalize_marking_completed_ = true;
-
-  if (FLAG_trace_incremental_marking) {
-    heap()->isolate()->PrintWithTimestamp(
-        "[IncrementalMarking] Finalize incrementally.\n");
-  }
-}
-
 void IncrementalMarking::UpdateMarkingWorklistAfterYoungGenGC() {
   if (!IsMarking()) return;
 
@@ -556,19 +542,6 @@ bool IncrementalMarking::Stop() {
   return true;
 }
 
-void IncrementalMarking::FinalizeMarking(CompletionAction action) {
-  DCHECK(!finalize_marking_completed_);
-  if (FLAG_trace_incremental_marking) {
-    heap()->isolate()->PrintWithTimestamp(
-        "[IncrementalMarking] requesting finalization of incremental "
-        "marking.\n");
-  }
-  request_type_ = GCRequestType::FINALIZATION;
-  if (action == CompletionAction::kGcViaStackGuard) {
-    heap_->isolate()->stack_guard()->RequestGC();
-  }
-}
-
 double IncrementalMarking::CurrentTimeToMarkingTask() const {
   const double recorded_time_to_marking_task =
       heap_->tracer()->AverageTimeToIncrementalMarkingTask();
@@ -633,7 +606,7 @@ void IncrementalMarking::MarkingComplete(CompletionAction action) {
     heap()->isolate()->PrintWithTimestamp(
         "[IncrementalMarking] Complete (normal).\n");
   }
-  request_type_ = GCRequestType::COMPLETE_MARKING;
+
   if (action == CompletionAction::kGcViaStackGuard) {
     heap_->isolate()->stack_guard()->RequestGC();
   }
@@ -643,8 +616,6 @@ void IncrementalMarking::Epilogue() {
   DCHECK(IsStopped());
 
   was_activated_ = false;
-  finalize_marking_completed_ = false;
-  request_type_ = GCRequestType::NONE;
 }
 
 bool IncrementalMarking::ShouldDoEmbedderStep() {
@@ -885,15 +856,12 @@ StepResult IncrementalMarking::Step(double max_step_size_in_ms,
     combined_result = CombineStepResults(v8_result, embedder_result);
 
     if (combined_result == StepResult::kNoImmediateWork) {
-      if (!finalize_marking_completed_) {
-        FinalizeMarking(action);
+      if (!IsComplete()) {
+        // TODO(v8:12775): Try to remove.
         FastForwardSchedule();
-        combined_result = StepResult::kWaitingForFinalization;
-        incremental_marking_job()->Start(heap_);
-      } else {
-        MarkingComplete(action);
-        combined_result = StepResult::kWaitingForFinalization;
       }
+      MarkingComplete(action);
+      combined_result = StepResult::kWaitingForFinalization;
     }
     if (FLAG_concurrent_marking) {
       local_marking_worklists()->ShareWork();

src/heap/incremental-marking.h

@@ -5,6 +5,7 @@
 #ifndef V8_HEAP_INCREMENTAL_MARKING_H_
 #define V8_HEAP_INCREMENTAL_MARKING_H_
 
+#include "src/base/logging.h"
 #include "src/base/platform/mutex.h"
 #include "src/heap/heap.h"
 #include "src/heap/incremental-marking-job.h"
@@ -38,8 +39,6 @@ class V8_EXPORT_PRIVATE IncrementalMarking final {
   //   is triggered via stack guard.
   enum class CompletionAction { kGcViaStackGuard, kGCViaTask };
 
-  enum class GCRequestType { NONE, COMPLETE_MARKING, FINALIZATION };
-
   using MarkingState = MarkCompactCollector::MarkingState;
   using AtomicMarkingState = MarkCompactCollector::AtomicMarkingState;
   using NonAtomicMarkingState = MarkCompactCollector::NonAtomicMarkingState;
@@ -102,30 +101,10 @@ class V8_EXPORT_PRIVATE IncrementalMarking final {
 
   V8_INLINE void TransferColor(HeapObject from, HeapObject to);
 
-  bool finalize_marking_completed() const {
-    return finalize_marking_completed_;
-  }
-
-  void SetWeakClosureWasOverApproximatedForTesting(bool val) {
-    finalize_marking_completed_ = val;
-  }
-
   bool IsStopped() const { return state() == STOPPED; }
   bool IsMarking() const { return state() >= MARKING; }
   bool IsComplete() const { return state() == COMPLETE; }
 
-  inline bool IsReadyToOverApproximateWeakClosure() const {
-    return request_type_ == GCRequestType::FINALIZATION &&
-           !finalize_marking_completed_;
-  }
-
-  inline bool NeedsFinalization() {
-    return IsMarking() && (request_type_ == GCRequestType::FINALIZATION ||
-                           request_type_ == GCRequestType::COMPLETE_MARKING);
-  }
-
-  GCRequestType request_type() const { return request_type_; }
-
   bool CanBeActivated();
   bool WasActivated();
 
@@ -133,13 +112,9 @@ class V8_EXPORT_PRIVATE IncrementalMarking final {
   // Returns true if incremental marking was running and false otherwise.
   bool Stop();
 
-  void FinalizeIncrementally();
-
   void UpdateMarkingWorklistAfterYoungGenGC();
   void UpdateMarkedBytesAfterScavenge(size_t dead_bytes_in_new_space);
 
-  void FinalizeMarking(CompletionAction action);
-
   void MarkingComplete(CompletionAction action);
 
   void Epilogue();
@@ -294,11 +269,8 @@ class V8_EXPORT_PRIVATE IncrementalMarking final {
   bool is_compacting_ = false;
   bool was_activated_ = false;
   bool black_allocation_ = false;
-  bool finalize_marking_completed_ = false;
   IncrementalMarkingJob incremental_marking_job_;
 
-  std::atomic<GCRequestType> request_type_{GCRequestType::NONE};
-
   Observer new_generation_observer_;
   Observer old_generation_observer_;
 

src/init/heap-symbols.h

@@ -519,7 +519,6 @@
   F(MC_INCREMENTAL_EXTERNAL_EPILOGUE)                              \
   F(MC_INCREMENTAL_EXTERNAL_PROLOGUE)                              \
   F(MC_INCREMENTAL_FINALIZE)                                       \
-  F(MC_INCREMENTAL_FINALIZE_BODY)                                  \
   F(MC_INCREMENTAL_LAYOUT_CHANGE)                                  \
   F(MC_INCREMENTAL_START)                                          \
   F(MC_INCREMENTAL_SWEEPING)

test/cctest/heap/heap-utils.cc

@@ -195,16 +195,14 @@ void SimulateIncrementalMarking(i::Heap* heap, bool force_completion) {
   CHECK(marking->IsMarking() || marking->IsComplete());
   if (!force_completion) return;
 
+  SafepointScope scope(heap);
+  MarkingBarrier::PublishAll(heap);
+  marking->MarkRootsForTesting();
+
   while (!marking->IsComplete()) {
     marking->Step(kStepSizeInMs,
                   i::IncrementalMarking::CompletionAction::kGCViaTask,
                   i::StepOrigin::kV8);
-    if (marking->IsReadyToOverApproximateWeakClosure()) {
-      SafepointScope scope(heap);
-      MarkingBarrier::PublishAll(heap);
-      marking->MarkRootsForTesting();
-      marking->FinalizeIncrementally();
-    }
   }
   CHECK(marking->IsComplete());
 }

test/cctest/heap/test-heap.cc

@@ -2479,8 +2479,6 @@ TEST(IdleNotificationFinishMarking) {
                 ->local_marking_worklists()
                 ->IsEmpty());
 
-  marking->SetWeakClosureWasOverApproximatedForTesting(true);
-
   // The next idle notification has to finish incremental marking.
   const double kLongIdleTime = 1000.0;
   CcTest::isolate()->IdleNotificationDeadline(
@@ -3912,8 +3910,7 @@ TEST(IncrementalMarkingStepMakesBigProgressWithLargeObjects) {
         i::Heap::kNoGCFlags, i::GarbageCollectionReason::kTesting);
   }
   heap::SimulateIncrementalMarking(CcTest::heap());
-  CHECK(marking->IsComplete() ||
-        marking->IsReadyToOverApproximateWeakClosure());
+  CHECK(marking->IsComplete());
 }
 
 
@@ -5735,17 +5732,15 @@ TEST(Regress598319) {
     }
   }
 
+  SafepointScope safepoint_scope(heap);
+  MarkingBarrier::PublishAll(heap);
+
   // Finish marking with bigger steps to speed up test.
   const double kLargeStepSizeInMs = 1000;
-  while (!marking->IsComplete()) {
-    marking->Step(kLargeStepSizeInMs,
-                  i::IncrementalMarking::CompletionAction::kGCViaTask,
-                  StepOrigin::kV8);
-    if (marking->IsReadyToOverApproximateWeakClosure()) {
-      SafepointScope safepoint_scope(heap);
-      MarkingBarrier::PublishAll(heap);
-      marking->FinalizeIncrementally();
-    }
+  while (marking->Step(kLargeStepSizeInMs,
+                       i::IncrementalMarking::CompletionAction::kGCViaTask,
+                       StepOrigin::kV8) !=
+         StepResult::kWaitingForFinalization) {
   }
   CHECK(marking->IsComplete());
 
@@ -5836,10 +5831,6 @@ TEST(Regress615489) {
     marking->Step(kStepSizeInMs,
                   i::IncrementalMarking::CompletionAction::kGCViaTask,
                   StepOrigin::kV8);
-    if (marking->IsReadyToOverApproximateWeakClosure()) {
-      SafepointScope safepoint_scope(heap);
-      marking->FinalizeIncrementally();
-    }
   }
   CHECK(marking->IsComplete());
   intptr_t size_before = heap->SizeOfObjects();
@@ -5900,10 +5891,6 @@ TEST(Regress631969) {
     marking->Step(kStepSizeInMs,
                   i::IncrementalMarking::CompletionAction::kGCViaTask,
                   StepOrigin::kV8);
-    if (marking->IsReadyToOverApproximateWeakClosure()) {
-      SafepointScope safepoint_scope(heap);
-      marking->FinalizeIncrementally();
-    }
   }
 
   {

test/unittests/heap/heap-utils.cc

@@ -34,10 +34,6 @@ void HeapInternalsBase::SimulateIncrementalMarking(Heap* heap,
     marking->Step(kStepSizeInMs,
                   i::IncrementalMarking::CompletionAction::kGCViaTask,
                   i::StepOrigin::kV8);
-    if (marking->IsReadyToOverApproximateWeakClosure()) {
-      SafepointScope scope(heap);
-      marking->FinalizeIncrementally();
-    }
   }
   CHECK(marking->IsComplete());
 }