heap: Report full GC cycles when sweeping finishes

This CL moves the call to GCTracer::StopCycle for the full GC from
Heap::CompleteSweeping full, which is called to force sweeping to
finish, to GCTracer::NotifySweepingComplete, which is called as soon
as sweeping finishes --- and symmetrically to a new method
GCTracer::NotifyCppGCCompleted, which is called as soon as sweeping
of the managed C++ heap finishes. In this way, a full GC cycle is
reported as soon as sweeping is finished both for the V8 and the C++
managed heap.

The changes introduced in this CL are essentially a partial revert of
https://crrev.com/c/3456563, fixed in such a way that when the full
GC cycle is reported, the current tracer event will be the correct
one corresponding to that cycle.

Bug: v8:12503
Bug: chromium:1154636
Change-Id: Icea07cf35a9565994e798b0500e9da72cd95f9ac
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3497318Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Nikolaos Papaspyrou <nikolaos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79470}

src/heap/cppgc-js/cpp-heap.cc

@@ -301,6 +301,7 @@ void CppHeap::MetricRecorderAdapter::AddMainThreadEvent(
     const FullCycle& cppgc_event) {
   DCHECK(!last_full_gc_event_.has_value());
   last_full_gc_event_ = cppgc_event;
+  GetIsolate()->heap()->tracer()->NotifyCppGCCompleted();
 }
 
 void CppHeap::MetricRecorderAdapter::AddMainThreadEvent(

src/heap/gc-tracer.cc

@@ -273,6 +273,8 @@ void GCTracer::ResetForTesting() {
   current_.end_time = MonotonicallyIncreasingTimeInMs();
   previous_ = current_;
   start_of_observable_pause_ = 0.0;
+  notified_sweeping_completed_ = false;
+  notified_cppgc_completed_ = false;
   young_gc_while_full_gc_ = false;
   ResetIncrementalMarkingCounters();
   allocation_time_ms_ = 0.0;
@@ -548,12 +550,29 @@ void GCTracer::StopCycle(GarbageCollector collector) {
   }
 }
 
-void GCTracer::StopCycleIfSweeping() {
+void GCTracer::StopCycleIfNeeded() {
   if (current_.state != Event::State::SWEEPING) return;
+  if (!notified_sweeping_completed_) return;
+  if (heap_->cpp_heap() && !notified_cppgc_completed_) return;
   StopCycle(GarbageCollector::MARK_COMPACTOR);
+  notified_sweeping_completed_ = false;
+  notified_cppgc_completed_ = false;
 }
 
 void GCTracer::NotifySweepingCompleted() {
+#ifdef VERIFY_HEAP
+  // If heap verification is enabled, sweeping finalization can also be
+  // triggered from inside a full GC cycle's atomic pause.
+  DCHECK((current_.type == Event::MARK_COMPACTOR ||
+          current_.type == Event::INCREMENTAL_MARK_COMPACTOR) &&
+         (current_.state == Event::State::SWEEPING ||
+          (FLAG_verify_heap && current_.state == Event::State::ATOMIC)));
+#else
+  DCHECK(IsSweepingInProgress());
+#endif
+
+  // Stop a full GC cycle only when both v8 and cppgc (if available) GCs have
+  // finished sweeping. This method is invoked by v8.
   if (FLAG_trace_gc_freelists) {
     PrintIsolate(heap_->isolate(),
                  "FreeLists statistics after sweeping completed:\n");
@@ -565,6 +584,21 @@ void GCTracer::NotifySweepingCompleted() {
     heap_->code_space()->PrintAllocationsOrigins();
     heap_->map_space()->PrintAllocationsOrigins();
   }
+  DCHECK(!notified_sweeping_completed_);
+  notified_sweeping_completed_ = true;
+  StopCycleIfNeeded();
+}
+
+void GCTracer::NotifyCppGCCompleted() {
+  // Stop a full GC cycle only when both v8 and cppgc (if available) GCs have
+  // finished sweeping. This method is invoked by cppgc.
+  DCHECK(heap_->cpp_heap());
+  DCHECK(CppHeap::From(heap_->cpp_heap())
+             ->GetMetricRecorder()
+             ->MetricsReportPending());
+  DCHECK(!notified_cppgc_completed_);
+  notified_cppgc_completed_ = true;
+  StopCycleIfNeeded();
 }
 
 void GCTracer::SampleAllocation(double current_ms,

src/heap/gc-tracer.h

@@ -269,7 +269,7 @@ class V8_EXPORT_PRIVATE GCTracer {
   void StartCycle(GarbageCollector collector, GarbageCollectionReason gc_reason,
                   const char* collector_reason, MarkingType marking);
   void StopCycle(GarbageCollector collector);
-  void StopCycleIfSweeping();
+  void StopCycleIfNeeded();
 
   // Start and stop a cycle's atomic pause.
   void StartAtomicPause();
@@ -279,6 +279,7 @@ class V8_EXPORT_PRIVATE GCTracer {
   void StopInSafepoint();
 
   void NotifySweepingCompleted();
+  void NotifyCppGCCompleted();
 
   void NotifyYoungGenerationHandling(
       YoungGenerationHandling young_generation_handling);
@@ -296,6 +297,14 @@ class V8_EXPORT_PRIVATE GCTracer {
             (current_.type == Event::MARK_COMPACTOR ||
              current_.type == Event::INCREMENTAL_MARK_COMPACTOR));
   }
+
+  // Checks if the current event corresponds to a full GC cycle whose sweeping
+  // has not finalized yet.
+  bool IsSweepingInProgress() const {
+    return (current_.type == Event::MARK_COMPACTOR ||
+            current_.type == Event::INCREMENTAL_MARK_COMPACTOR) &&
+           current_.state == Event::State::SWEEPING;
+  }
 #endif
 
   // Sample and accumulate bytes allocated since the last GC.
@@ -568,6 +577,11 @@ class V8_EXPORT_PRIVATE GCTracer {
   base::RingBuffer<BytesAndDuration> recorded_embedder_generation_allocations_;
   base::RingBuffer<double> recorded_survival_ratios_;
 
+  // A full GC cycle stops only when both v8 and cppgc (if available) GCs have
+  // finished sweeping.
+  bool notified_sweeping_completed_ = false;
+  bool notified_cppgc_completed_ = false;
+
   // When a full GC cycle is interrupted by a young generation GC cycle, the
   // |previous_| event is used as temporary storage for the |current_| event
   // that corresponded to the full GC cycle, and this field is set to true.

src/heap/heap.cc

@@ -1910,6 +1910,8 @@ bool Heap::CollectGarbage(AllocationSpace space,
     // interrupted full cycle.
     if (IsYoungGenerationCollector(collector)) {
       tracer()->StopCycle(collector);
+    } else {
+      tracer()->StopCycleIfNeeded();
     }
   }
 
@@ -2019,8 +2021,11 @@ void Heap::CompleteSweepingFull() {
   array_buffer_sweeper()->EnsureFinished();
   mark_compact_collector()->EnsureSweepingCompleted(
       MarkCompactCollector::SweepingForcedFinalizationMode::kUnifiedHeap);
+
   DCHECK(!mark_compact_collector()->sweeping_in_progress());
-  tracer()->StopCycleIfSweeping();
+  DCHECK_IMPLIES(cpp_heap(),
+                 !CppHeap::From(cpp_heap())->sweeper().IsSweepingInProgress());
+  DCHECK(!tracer()->IsSweepingInProgress());
 }
 
 void Heap::StartIncrementalMarkingIfAllocationLimitIsReached(
@@ -2220,6 +2225,15 @@ size_t Heap::PerformGarbageCollection(
     const char* collector_reason, const v8::GCCallbackFlags gc_callback_flags) {
   DisallowJavascriptExecution no_js(isolate());
 
+#ifdef VERIFY_HEAP
+  if (FLAG_verify_heap) {
+    // We don't really perform a GC here but need this scope for the nested
+    // SafepointScope inside Verify().
+    AllowGarbageCollection allow_gc;
+    Verify();
+  }
+#endif  // VERIFY_HEAP
+
   if (IsYoungGenerationCollector(collector)) {
     CompleteSweepingYoung(collector);
     tracer()->StartCycle(collector, gc_reason, collector_reason,
@@ -2254,15 +2268,6 @@ size_t Heap::PerformGarbageCollection(
 
   collection_barrier_->StopTimeToCollectionTimer();
 
-#ifdef VERIFY_HEAP
-  if (FLAG_verify_heap) {
-    // We don't really perform a GC here but need this scope for the nested
-    // SafepointScope inside Verify().
-    AllowGarbageCollection allow_gc;
-    Verify();
-  }
-#endif
-
   tracer()->StartInSafepoint();
 
   GarbageCollectionPrologueInSafepoint();
@@ -2338,7 +2343,7 @@ size_t Heap::PerformGarbageCollection(
     AllowGarbageCollection allow_gc;
     Verify();
   }
-#endif
+#endif  // VERIFY_HEAP
 
   RecomputeLimits(collector);
 
@@ -2398,6 +2403,7 @@ void Heap::PerformSharedGarbageCollection(Isolate* initiator,
   tracer()->StopAtomicPause();
   tracer()->StopObservablePause();
   tracer()->UpdateStatistics(collector);
+  tracer()->StopCycleIfNeeded();
 }
 
 void Heap::CompleteSweepingYoung(GarbageCollector collector) {

src/heap/mark-compact.cc

@@ -721,7 +721,13 @@ void MarkCompactCollector::EnsureSweepingCompleted(
     // Ensure that sweeping is also completed for the C++ managed heap, if one
     // exists.
     CppHeap::From(heap()->cpp_heap())->FinishSweepingIfRunning();
+    DCHECK(
+        !CppHeap::From(heap()->cpp_heap())->sweeper().IsSweepingInProgress());
   }
+
+  DCHECK_IMPLIES(mode == SweepingForcedFinalizationMode::kUnifiedHeap ||
+                     !heap()->cpp_heap(),
+                 !heap()->tracer()->IsSweepingInProgress());
 }
 
 void MarkCompactCollector::EnsurePageIsSwept(Page* page) {

src/heap/scavenger.cc

@@ -296,7 +296,7 @@ void ScavengerCollector::CollectGarbage() {
     Sweeper* sweeper = heap_->mark_compact_collector()->sweeper();
 
     // Pause the concurrent sweeper.
-    Sweeper::PauseOrCompleteScope pause_scope(sweeper);
+    Sweeper::PauseScope pause_scope(sweeper);
     // Filter out pages from the sweeper that need to be processed for old to
     // new slots by the Scavenger. After processing, the Scavenger adds back
     // pages that are still unsweeped. This way the Scavenger has exclusive

src/heap/sweeper.cc

@@ -28,37 +28,27 @@ Sweeper::Sweeper(Heap* heap, MajorNonAtomicMarkingState* marking_state)
       iterability_task_started_(false),
       should_reduce_memory_(false) {}
 
-Sweeper::PauseOrCompleteScope::PauseOrCompleteScope(Sweeper* sweeper)
-    : sweeper_(sweeper) {
+Sweeper::PauseScope::PauseScope(Sweeper* sweeper) : sweeper_(sweeper) {
   if (!sweeper_->sweeping_in_progress()) return;
 
   if (sweeper_->job_handle_ && sweeper_->job_handle_->IsValid())
     sweeper_->job_handle_->Cancel();
-
-  // Complete sweeping if there's nothing more to do.
-  if (sweeper_->IsDoneSweeping()) {
-    sweeper_->heap_->mark_compact_collector()->EnsureSweepingCompleted(
-        MarkCompactCollector::SweepingForcedFinalizationMode::kV8Only);
-    DCHECK(!sweeper_->sweeping_in_progress());
-  } else {
-    // Unless sweeping is complete the flag still indicates that the sweeper
-    // is enabled. It just cannot use tasks anymore.
-    DCHECK(sweeper_->sweeping_in_progress());
-  }
 }
 
-Sweeper::PauseOrCompleteScope::~PauseOrCompleteScope() {
+Sweeper::PauseScope::~PauseScope() {
   if (!sweeper_->sweeping_in_progress()) return;
 
   sweeper_->StartSweeperTasks();
 }
 
 Sweeper::FilterSweepingPagesScope::FilterSweepingPagesScope(
-    Sweeper* sweeper, const PauseOrCompleteScope& pause_or_complete_scope)
+    Sweeper* sweeper, const PauseScope& pause_scope)
     : sweeper_(sweeper),
-      pause_or_complete_scope_(pause_or_complete_scope),
       sweeping_in_progress_(sweeper_->sweeping_in_progress()) {
-  USE(pause_or_complete_scope_);
+  // The PauseScope here only serves as a witness that concurrent sweeping has
+  // been paused.
+  USE(pause_scope);
+
   if (!sweeping_in_progress_) return;
 
   int old_space_index = GetSweepSpaceIndex(OLD_SPACE);

src/heap/sweeper.h

@@ -31,11 +31,11 @@ class Sweeper {
   using SweptList = std::vector<Page*>;
   using FreeRangesMap = std::map<uint32_t, uint32_t>;
 
-  // Pauses the sweeper tasks or completes sweeping.
-  class V8_NODISCARD PauseOrCompleteScope final {
+  // Pauses the sweeper tasks.
+  class V8_NODISCARD PauseScope final {
    public:
-    explicit PauseOrCompleteScope(Sweeper* sweeper);
-    ~PauseOrCompleteScope();
+    explicit PauseScope(Sweeper* sweeper);
+    ~PauseScope();
 
    private:
     Sweeper* const sweeper_;
@@ -47,8 +47,7 @@ class Sweeper {
   // after exiting this scope.
   class V8_NODISCARD FilterSweepingPagesScope final {
    public:
-    FilterSweepingPagesScope(
-        Sweeper* sweeper, const PauseOrCompleteScope& pause_or_complete_scope);
+    FilterSweepingPagesScope(Sweeper* sweeper, const PauseScope& pause_scope);
     ~FilterSweepingPagesScope();
 
     template <typename Callback>
@@ -69,7 +68,6 @@ class Sweeper {
    private:
     Sweeper* const sweeper_;
     SweepingList old_space_sweeping_list_;
-    const PauseOrCompleteScope& pause_or_complete_scope_;
     bool sweeping_in_progress_;
   };
 

test/cctest/heap/test-embedder-tracing.cc

@@ -278,7 +278,7 @@ TEST(FinalizeTracingWhenMarking) {
     heap->mark_compact_collector()->EnsureSweepingCompleted(
         MarkCompactCollector::SweepingForcedFinalizationMode::kV8Only);
   }
-  heap->tracer()->StopCycleIfSweeping();
+  heap->tracer()->StopCycleIfNeeded();
   CHECK(heap->incremental_marking()->IsStopped());
 
   i::IncrementalMarking* marking = heap->incremental_marking();

test/cctest/heap/test-heap.cc

@@ -6401,7 +6401,7 @@ HEAP_TEST(Regress670675) {
     collector->EnsureSweepingCompleted(
         MarkCompactCollector::SweepingForcedFinalizationMode::kV8Only);
   }
-  heap->tracer()->StopCycleIfSweeping();
+  heap->tracer()->StopCycleIfNeeded();
   i::IncrementalMarking* marking = CcTest::heap()->incremental_marking();
   if (marking->IsStopped()) {
     SafepointScope safepoint_scope(heap);

test/unittests/heap/gc-tracer-unittest.cc

@@ -101,10 +101,11 @@ void StopTracing(GCTracer* tracer, GarbageCollector collector) {
   tracer->StopAtomicPause();
   tracer->StopObservablePause();
   tracer->UpdateStatistics(collector);
-  if (Heap::IsYoungGenerationCollector(collector))
+  if (Heap::IsYoungGenerationCollector(collector)) {
     tracer->StopCycle(collector);
-  else
-    tracer->StopCycleIfSweeping();
+  } else {
+    tracer->NotifySweepingCompleted();
+  }
 }
 
 }  // namespace