heap: Move call to ReportFullCycleToRecorder

This CL simplifies the reporting of full GC cycles and the connection
between the GC of the managed C++ heap and the managed Javascript heap.
It moves the call to GCTracer::RecordFullCycleToRecorder to be part of
GCTracer::StopCycle.

Bug: v8:12503
Bug: chromium:1154636
Change-Id: I332dbcd81d2e5bdda83f3353c6526fc18e23ebd5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3456563Reviewed-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@{#79075}

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

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

src/heap/gc-tracer.cc

@@ -501,6 +501,8 @@ void GCTracer::StopCycle(GarbageCollector collector) {
       young_gc_while_full_gc_ = false;
     }
   } else {
+    ReportFullCycleToRecorder();
+
     counters->mark_compact_reason()->AddSample(static_cast<int>(gc_reason));
 
     if (FLAG_trace_gc_freelists) {
@@ -528,8 +530,6 @@ void GCTracer::NotifySweepingCompleted() {
     heap_->code_space()->PrintAllocationsOrigins();
     heap_->map_space()->PrintAllocationsOrigins();
   }
-  metrics_report_pending_ = true;
-  NotifyGCCompleted();
 }
 
 void GCTracer::SampleAllocation(double current_ms,
@@ -1380,22 +1380,6 @@ void GCTracer::RecordGCSumCounters(double atomic_pause_duration) {
                        "background_duration", marking_background_duration);
 }
 
-void GCTracer::NotifyGCCompleted() {
-  // Report full GC cycle metric to recorder only when both v8 and cppgc (if
-  // available) GCs have finished. This method is invoked by both v8 and cppgc.
-  if (!metrics_report_pending_) {
-    // V8 sweeping is not done yet.
-    return;
-  }
-  const auto* cpp_heap = heap_->cpp_heap();
-  if (cpp_heap &&
-      !CppHeap::From(cpp_heap)->GetMetricRecorder()->MetricsReportPending()) {
-    // Cppgc sweeping is not done yet.
-    return;
-  }
-  ReportFullCycleToRecorder();
-}
-
 namespace {
 
 void CopyTimeMetrics(
@@ -1462,20 +1446,18 @@ void FlushBatchedIncrementalEvents(
 }  // namespace
 
 void GCTracer::ReportFullCycleToRecorder() {
-  // TODO(chromium:1154636): The following check should be added. It is now
-  // failing because this method is called during young generation GC cycles
-  // that finalize sweeping. This should be fixed.
-  // DCHECK(!Event::IsYoungGenerationEvent(current_.type));
+  DCHECK(!Event::IsYoungGenerationEvent(current_.type));
+  DCHECK_EQ(Event::State::NOT_RUNNING, current_.state);
+  auto* cpp_heap = v8::internal::CppHeap::From(heap_->cpp_heap());
+  DCHECK_IMPLIES(cpp_heap,
+                 cpp_heap->GetMetricRecorder()->MetricsReportPending());
   const std::shared_ptr<metrics::Recorder>& recorder =
       heap_->isolate()->metrics_recorder();
   DCHECK_NOT_NULL(recorder);
-  metrics_report_pending_ = false;
   if (!recorder->HasEmbedderRecorder()) {
     incremental_mark_batched_events_.events.clear();
-    if (heap_->cpp_heap()) {
-      v8::internal::CppHeap::From(heap_->cpp_heap())
-          ->GetMetricRecorder()
-          ->ClearCachedEvents();
+    if (cpp_heap) {
+      cpp_heap->GetMetricRecorder()->ClearCachedEvents();
     }
     return;
   }
@@ -1484,8 +1466,7 @@ void GCTracer::ReportFullCycleToRecorder() {
                                   heap_->isolate());
   }
   v8::metrics::GarbageCollectionFullCycle event;
-  if (heap_->cpp_heap()) {
-    auto* cpp_heap = v8::internal::CppHeap::From(heap_->cpp_heap());
+  if (cpp_heap) {
     cpp_heap->GetMetricRecorder()->FlushBatchedIncrementalEvents();
     const base::Optional<cppgc::internal::MetricRecorder::FullCycle>
         optional_cppgc_event =
@@ -1546,6 +1527,7 @@ void GCTracer::ReportIncrementalMarkingStepToRecorder() {
 
 void GCTracer::ReportYoungCycleToRecorder() {
   DCHECK(Event::IsYoungGenerationEvent(current_.type));
+  DCHECK_EQ(Event::State::NOT_RUNNING, current_.state);
   const std::shared_ptr<metrics::Recorder>& recorder =
       heap_->isolate()->metrics_recorder();
   DCHECK_NOT_NULL(recorder);

src/heap/gc-tracer.h

@@ -262,8 +262,6 @@ class V8_EXPORT_PRIVATE GCTracer {
 
   void NotifySweepingCompleted();
 
-  void NotifyGCCompleted();
-
   void NotifyYoungGenerationHandling(
       YoungGenerationHandling young_generation_handling);
 
@@ -520,7 +518,6 @@ class V8_EXPORT_PRIVATE GCTracer {
   IncrementalMarkingInfos
       incremental_marking_scopes_[Scope::NUMBER_OF_INCREMENTAL_SCOPES];
 
-
   // Timestamp and allocation counter at the last sampled allocation event.
   double allocation_time_ms_;
   size_t new_space_allocation_counter_bytes_;
@@ -554,8 +551,6 @@ class V8_EXPORT_PRIVATE GCTracer {
   base::RingBuffer<BytesAndDuration> recorded_embedder_generation_allocations_;
   base::RingBuffer<double> recorded_survival_ratios_;
 
-  bool metrics_report_pending_ = 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.

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

@@ -325,12 +325,12 @@ TEST_F(GCTracerTest, IncrementalMarkingSpeed) {
   EXPECT_EQ(3000000u, tracer->incremental_marking_bytes_);
   EXPECT_EQ(1000000 / 100,
             tracer->IncrementalMarkingSpeedInBytesPerMillisecond());
-  StartTracing(tracer, GarbageCollector::MARK_COMPACTOR,
-               StartTracingMode::kIncrementalEnterPause);
   // 1000000 bytes in 100ms.
   tracer->AddIncrementalMarkingStep(100, 1000000);
   EXPECT_EQ(400, tracer->incremental_marking_duration_);
   EXPECT_EQ(4000000u, tracer->incremental_marking_bytes_);
+  StartTracing(tracer, GarbageCollector::MARK_COMPACTOR,
+               StartTracingMode::kIncrementalEnterPause);
   StopTracing(tracer, GarbageCollector::MARK_COMPACTOR);
   EXPECT_EQ(400, tracer->current_.incremental_marking_duration);
   EXPECT_EQ(4000000u, tracer->current_.incremental_marking_bytes);