cppgc: young-gen: Enable concurrent sweeping for minor GCs

Similar to full GCs, the GC defers metric reporting until sweeping is
finished.

Bug: chromium:1029379
Change-Id: Ib06adb3be691c1ad2bd530eb77fc01cc22537338
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3576130Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Nikolaos Papaspyrou <nikolaos@chromium.org>
Commit-Queue: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79891}

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

@@ -145,15 +145,13 @@ void CppHeap::EnableDetachedGarbageCollectionsForTesting() {
 
 void CppHeap::CollectGarbageForTesting(cppgc::EmbedderStackState stack_state) {
   return internal::CppHeap::From(this)->CollectGarbageForTesting(
-      cppgc::internal::GarbageCollector::Config::CollectionType::kMajor,
-      stack_state);
+      internal::CppHeap::CollectionType::kMajor, stack_state);
 }
 
 void CppHeap::CollectGarbageInYoungGenerationForTesting(
     cppgc::EmbedderStackState stack_state) {
   return internal::CppHeap::From(this)->CollectGarbageForTesting(
-      cppgc::internal::GarbageCollector::Config::CollectionType::kMinor,
-      stack_state);
+      internal::CppHeap::CollectionType::kMinor, stack_state);
 }
 
 namespace internal {
@@ -277,8 +275,7 @@ UnifiedHeapMarker::UnifiedHeapMarker(Heap* v8_heap,
     : cppgc::internal::MarkerBase(heap, platform, config),
       unified_heap_marking_state_(v8_heap),
       marking_visitor_(
-          config.collection_type == cppgc::internal::GarbageCollector::Config::
-                                        CollectionType::kMajor
+          config.collection_type == CppHeap::CollectionType::kMajor
               ? std::make_unique<MutatorUnifiedHeapMarkingVisitor>(
                     heap, mutator_marking_state_, unified_heap_marking_state_)
               : std::make_unique<MutatorMinorGCMarkingVisitor>(
@@ -297,17 +294,16 @@ void UnifiedHeapMarker::AddObject(void* object) {
 
 void CppHeap::MetricRecorderAdapter::AddMainThreadEvent(
     const GCCycle& cppgc_event) {
+  auto* tracer = GetIsolate()->heap()->tracer();
   if (cppgc_event.type == MetricRecorder::GCCycle::Type::kMinor) {
     DCHECK(!last_young_gc_event_);
     last_young_gc_event_ = cppgc_event;
+    tracer->NotifyYoungCppGCCompleted();
   } else {
     DCHECK(!last_full_gc_event_);
     last_full_gc_event_ = cppgc_event;
+    tracer->NotifyFullCppGCCompleted();
   }
-  GetIsolate()->heap()->tracer()->NotifyCppGCCompleted(
-      cppgc_event.type == MetricRecorder::GCCycle::Type::kMinor
-          ? GCTracer::CppType::kMinor
-          : GCTracer::CppType::kMajor);
 }
 
 void CppHeap::MetricRecorderAdapter::AddMainThreadEvent(
@@ -513,6 +509,9 @@ bool ShouldReduceMemory(CppHeap::GarbageCollectionFlags flags) {
 }  // namespace
 
 CppHeap::MarkingType CppHeap::SelectMarkingType() const {
+  // For now, force atomic marking for minor collections.
+  if (*collection_type_ == CollectionType::kMinor) return MarkingType::kAtomic;
+
   if (IsForceGC(current_gc_flags_) && !force_incremental_marking_for_testing_)
     return MarkingType::kAtomic;
 
@@ -525,16 +524,14 @@ CppHeap::SweepingType CppHeap::SelectSweepingType() const {
   return sweeping_support();
 }
 
-void CppHeap::InitializeTracing(
-    cppgc::internal::GarbageCollector::Config::CollectionType collection_type,
+void CppHeap::InitializeTracing(CollectionType collection_type,
                                 GarbageCollectionFlags gc_flags) {
   CHECK(!sweeper_.IsSweepingInProgress());
 
   // Check that previous cycle metrics for the same collection type have been
   // reported.
   if (GetMetricRecorder()) {
-    if (collection_type ==
-        cppgc::internal::GarbageCollector::Config::CollectionType::kMajor)
+    if (collection_type == CollectionType::kMajor)
       DCHECK(!GetMetricRecorder()->FullGCMetricsReportPending());
     else
       DCHECK(!GetMetricRecorder()->YoungGCMetricsReportPending());
@@ -544,16 +541,14 @@ void CppHeap::InitializeTracing(
   collection_type_ = collection_type;
 
 #if defined(CPPGC_YOUNG_GENERATION)
-  if (*collection_type_ ==
-      cppgc::internal::GarbageCollector::Config::CollectionType::kMajor)
+  if (*collection_type_ == CollectionType::kMajor)
     cppgc::internal::SequentialUnmarker unmarker(raw_heap());
 #endif  // defined(CPPGC_YOUNG_GENERATION)
 
   current_gc_flags_ = gc_flags;
 
   const UnifiedHeapMarker::MarkingConfig marking_config{
-      *collection_type_, cppgc::Heap::StackState::kNoHeapPointers,
-      SelectMarkingType(),
+      *collection_type_, StackState::kNoHeapPointers, SelectMarkingType(),
       IsForceGC(current_gc_flags_)
           ? UnifiedHeapMarker::MarkingConfig::IsForcedGC::kForced
           : UnifiedHeapMarker::MarkingConfig::IsForcedGC::kNotForced};
@@ -602,9 +597,7 @@ void CppHeap::EnterFinalPause(cppgc::EmbedderStackState stack_state) {
   CHECK(!in_disallow_gc_scope());
   in_atomic_pause_ = true;
   marker_->EnterAtomicPause(stack_state);
-  if (isolate_ &&
-      *collection_type_ ==
-          cppgc::internal::GarbageCollector::Config::CollectionType::kMinor) {
+  if (isolate_ && *collection_type_ == CollectionType::kMinor) {
     // Visit V8 -> cppgc references.
     TraceV8ToCppGCReferences(isolate_,
                              static_cast<UnifiedHeapMarker*>(marker_.get())
@@ -665,26 +658,29 @@ void CppHeap::TraceEpilogue() {
   sweeper().NotifyDoneIfNeeded();
 }
 
-void CppHeap::RunMinorGC() {
-#if defined(CPPGC_YOUNG_GENERATION)
+void CppHeap::RunMinorGC(StackState stack_state) {
+  DCHECK(!sweeper_.IsSweepingInProgress());
+
   if (in_no_gc_scope()) return;
   // Minor GC does not support nesting in full GCs.
   if (IsMarking()) return;
-  // Finish sweeping in case it is still running.
-  sweeper().FinishIfRunning();
+  // Minor GCs with the stack are currently not supported.
+  if (stack_state == StackState::kMayContainHeapPointers) return;
+
+  // Notify GC tracer that CppGC started young GC cycle.
+  isolate_->heap()->tracer()->NotifyYoungCppGCRunning();
 
   SetStackEndOfCurrentGC(v8::base::Stack::GetCurrentStackPosition());
 
   // Perform an atomic GC, with starting incremental/concurrent marking and
   // immediately finalizing the garbage collection.
-  InitializeTracing(
-      cppgc::internal::GarbageCollector::Config::CollectionType::kMinor,
-      GarbageCollectionFlagValues::kForced);
+  InitializeTracing(CollectionType::kMinor,
+                    GarbageCollectionFlagValues::kNoFlags);
   StartTracing();
+  // TODO(chromium:1029379): Should be safe to run without stack.
   EnterFinalPause(cppgc::EmbedderStackState::kMayContainHeapPointers);
   AdvanceTracing(std::numeric_limits<double>::infinity());
   TraceEpilogue();
-#endif  // defined(CPPGC_YOUNG_GENERATION)
 }
 
 void CppHeap::AllocatedObjectSizeIncreased(size_t bytes) {
@@ -721,9 +717,8 @@ void CppHeap::ReportBufferedAllocationSizeIfPossible() {
   }
 }
 
-void CppHeap::CollectGarbageForTesting(
-    cppgc::internal::GarbageCollector::Config::CollectionType collection_type,
-    cppgc::internal::GarbageCollector::Config::StackState stack_state) {
+void CppHeap::CollectGarbageForTesting(CollectionType collection_type,
+                                       StackState stack_state) {
   if (in_no_gc_scope()) return;
 
   // Finish sweeping in case it is still running.
@@ -762,8 +757,7 @@ void CppHeap::StartIncrementalGarbageCollectionForTesting() {
   DCHECK_NULL(isolate_);
   if (IsMarking()) return;
   force_incremental_marking_for_testing_ = true;
-  InitializeTracing(
-      cppgc::internal::GarbageCollector::Config::CollectionType::kMajor,
+  InitializeTracing(CollectionType::kMajor,
                     GarbageCollectionFlagValues::kForced);
   StartTracing();
   force_incremental_marking_for_testing_ = false;
@@ -775,9 +769,7 @@ void CppHeap::FinalizeIncrementalGarbageCollectionForTesting(
   DCHECK_NULL(isolate_);
   DCHECK(IsMarking());
   if (IsMarking()) {
-    CollectGarbageForTesting(
-        cppgc::internal::GarbageCollector::Config::CollectionType::kMajor,
-        stack_state);
+    CollectGarbageForTesting(CollectionType::kMajor, stack_state);
   }
   sweeper_.FinishIfRunning();
 }

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

@@ -41,6 +41,9 @@ class V8_EXPORT_PRIVATE CppHeap final
   };
 
   using GarbageCollectionFlags = base::Flags<GarbageCollectionFlagValues>;
+  using StackState = cppgc::internal::GarbageCollector::Config::StackState;
+  using CollectionType =
+      cppgc::internal::GarbageCollector::Config::CollectionType;
 
   class MetricRecorderAdapter final : public cppgc::internal::MetricRecorder {
    public:
@@ -114,9 +117,7 @@ class V8_EXPORT_PRIVATE CppHeap final
 
   void EnableDetachedGarbageCollectionsForTesting();
 
-  void CollectGarbageForTesting(
-      cppgc::internal::GarbageCollector::Config::CollectionType,
-      cppgc::internal::GarbageCollector::Config::StackState);
+  void CollectGarbageForTesting(CollectionType, StackState);
 
   void CollectCustomSpaceStatisticsAtLastGC(
       std::vector<cppgc::CustomSpaceIndex>,
@@ -134,7 +135,7 @@ class V8_EXPORT_PRIVATE CppHeap final
   void TraceEpilogue();
   void EnterFinalPause(cppgc::EmbedderStackState stack_state);
 
-  void RunMinorGC();
+  void RunMinorGC(StackState);
 
   // StatsCollector::AllocationObserver interface.
   void AllocatedObjectSizeIncreased(size_t) final;

src/heap/cppgc/marker.cc

@@ -225,6 +225,7 @@ void MarkerBase::StartMarking() {
         incremental_marking_allocation_observer_.get());
   }
 }
+
 void MarkerBase::HandleNotFullyConstructedObjects() {
   if (config_.stack_state == MarkingConfig::StackState::kNoHeapPointers) {
     mutator_marking_state_.FlushNotFullyConstructedObjects();

src/heap/gc-tracer.cc

@@ -274,7 +274,9 @@ void GCTracer::ResetForTesting() {
   previous_ = current_;
   start_of_observable_pause_ = 0.0;
   notified_sweeping_completed_ = false;
-  notified_cppgc_completed_ = false;
+  notified_full_cppgc_completed_ = false;
+  notified_young_cppgc_completed_ = false;
+  notified_young_cppgc_running_ = false;
   young_gc_while_full_gc_ = false;
   ResetIncrementalMarkingCounters();
   allocation_time_ms_ = 0.0;
@@ -550,13 +552,29 @@ void GCTracer::StopCycle(GarbageCollector collector) {
   }
 }
 
-void GCTracer::StopCycleIfNeeded() {
+void GCTracer::StopFullCycleIfNeeded() {
   if (current_.state != Event::State::SWEEPING) return;
   if (!notified_sweeping_completed_) return;
-  if (heap_->cpp_heap() && !notified_cppgc_completed_) return;
+  if (heap_->cpp_heap() && !notified_full_cppgc_completed_) return;
   StopCycle(GarbageCollector::MARK_COMPACTOR);
   notified_sweeping_completed_ = false;
-  notified_cppgc_completed_ = false;
+  notified_full_cppgc_completed_ = false;
+}
+
+void GCTracer::StopYoungCycleIfNeeded() {
+  // We rely here on the fact that young GCs in V8 are atomic and by the time
+  // this is called, the Scavenger or Minor MC has already finished.
+  DCHECK(Event::IsYoungGenerationEvent(current_.type));
+  if (current_.state != Event::State::SWEEPING) return;
+  // Check if young cppgc was scheduled but hasn't completed yet.
+  if (heap_->cpp_heap() && notified_young_cppgc_running_ &&
+      !notified_young_cppgc_completed_)
+    return;
+  StopCycle(current_.type == Event::SCAVENGER
+                ? GarbageCollector::SCAVENGER
+                : GarbageCollector::MINOR_MARK_COMPACTOR);
+  notified_young_cppgc_running_ = false;
+  notified_young_cppgc_completed_ = false;
 }
 
 void GCTracer::NotifySweepingCompleted() {
@@ -586,29 +604,39 @@ void GCTracer::NotifySweepingCompleted() {
   }
   DCHECK(!notified_sweeping_completed_);
   notified_sweeping_completed_ = true;
-  StopCycleIfNeeded();
+  StopFullCycleIfNeeded();
 }
 
-void GCTracer::NotifyCppGCCompleted(CppType collection_type) {
+void GCTracer::NotifyFullCppGCCompleted() {
   // 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());
   const auto* metric_recorder =
       CppHeap::From(heap_->cpp_heap())->GetMetricRecorder();
   USE(metric_recorder);
-  if (collection_type == CppType::kMinor) {
-    DCHECK(metric_recorder->YoungGCMetricsReportPending());
-    // Young generation GCs in Oilpan run together with Scavenger. Check that
-    // collection types are in sync.
-    DCHECK(Event::IsYoungGenerationEvent(current_.type));
-    // Don't stop the cycle (i.e. call StopCycle/StopCycleIfNeeded) for young
-    // generation events - this is performed later in Heap::CollectGarbage().
-    return;
-  }
   DCHECK(metric_recorder->FullGCMetricsReportPending());
-  DCHECK(!notified_cppgc_completed_);
-  notified_cppgc_completed_ = true;
-  StopCycleIfNeeded();
+  DCHECK(!notified_full_cppgc_completed_);
+  notified_full_cppgc_completed_ = true;
+  StopFullCycleIfNeeded();
+}
+
+void GCTracer::NotifyYoungCppGCCompleted() {
+  // Stop a young 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(notified_young_cppgc_running_);
+  const auto* metric_recorder =
+      CppHeap::From(heap_->cpp_heap())->GetMetricRecorder();
+  USE(metric_recorder);
+  DCHECK(metric_recorder->YoungGCMetricsReportPending());
+  DCHECK(!notified_young_cppgc_completed_);
+  notified_young_cppgc_completed_ = true;
+  StopYoungCycleIfNeeded();
+}
+
+void GCTracer::NotifyYoungCppGCRunning() {
+  DCHECK(!notified_young_cppgc_running_);
+  notified_young_cppgc_running_ = true;
 }
 
 void GCTracer::SampleAllocation(double current_ms,

src/heap/gc-tracer.h

@@ -235,11 +235,6 @@ class V8_EXPORT_PRIVATE GCTracer {
     TimedHistogram* type_priority_timer;
   };
 
-  enum class CppType {
-    kMinor,
-    kMajor,
-  };
-
   static const int kThroughputTimeFrameMs = 5000;
   static constexpr double kConservativeSpeedInBytesPerMillisecond = 128 * KB;
 
@@ -271,8 +266,8 @@ class V8_EXPORT_PRIVATE GCTracer {
   // Start and stop a GC cycle (collecting data and reporting results).
   void StartCycle(GarbageCollector collector, GarbageCollectionReason gc_reason,
                   const char* collector_reason, MarkingType marking);
-  void StopCycle(GarbageCollector collector);
-  void StopCycleIfNeeded();
+  void StopYoungCycleIfNeeded();
+  void StopFullCycleIfNeeded();
 
   // Start and stop a cycle's atomic pause.
   void StartAtomicPause();
@@ -282,7 +277,10 @@ class V8_EXPORT_PRIVATE GCTracer {
   void StopInSafepoint();
 
   void NotifySweepingCompleted();
-  void NotifyCppGCCompleted(CppType);
+  void NotifyFullCppGCCompleted();
+
+  void NotifyYoungCppGCRunning();
+  void NotifyYoungCppGCCompleted();
 
   void NotifyYoungGenerationHandling(
       YoungGenerationHandling young_generation_handling);
@@ -462,6 +460,8 @@ class V8_EXPORT_PRIVATE GCTracer {
     double total_duration_ms;
   };
 
+  void StopCycle(GarbageCollector collector);
+
   // Returns the average speed of the events in the buffer.
   // If the buffer is empty, the result is 0.
   // Otherwise, the result is between 1 byte/ms and 1 GB/ms.
@@ -587,7 +587,13 @@ class V8_EXPORT_PRIVATE GCTracer {
   // 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;
+  bool notified_full_cppgc_completed_ = false;
+  // Similar to full GCs, a young GC cycle stops only when both v8 and cppgc GCs
+  // have finished sweeping.
+  bool notified_young_cppgc_completed_ = false;
+  // Keep track whether the young cppgc GC was scheduled (as opposed to full
+  // cycles, for young cycles cppgc is not always scheduled).
+  bool notified_young_cppgc_running_ = 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

src/heap/heap.cc

@@ -1914,9 +1914,9 @@ bool Heap::CollectGarbage(AllocationSpace space,
     // order; the latter may replace the current event with that of an
     // interrupted full cycle.
     if (IsYoungGenerationCollector(collector)) {
-      tracer()->StopCycle(collector);
+      tracer()->StopYoungCycleIfNeeded();
     } else {
-      tracer()->StopCycleIfNeeded();
+      tracer()->StopFullCycleIfNeeded();
     }
   }
 
@@ -2357,13 +2357,17 @@ size_t Heap::PerformGarbageCollection(
     local_embedder_heap_tracer()->TraceEpilogue();
   }
 
+#if defined(CPPGC_YOUNG_GENERATION)
   // Schedule Oilpan's Minor GC. Since the minor GC doesn't support conservative
   // stack scanning, do it only when Scavenger runs from task, which is
   // non-nestable.
-  if (cpp_heap() && collector == GarbageCollector::SCAVENGER &&
-      gc_reason == GarbageCollectionReason::kTask) {
-    CppHeap::From(cpp_heap())->RunMinorGC();
+  if (cpp_heap() && IsYoungGenerationCollector(collector)) {
+    const bool with_stack = (gc_reason != GarbageCollectionReason::kTask);
+    CppHeap::From(cpp_heap())
+        ->RunMinorGC(with_stack ? CppHeap::StackState::kMayContainHeapPointers
+                                : CppHeap::StackState::kNoHeapPointers);
   }
+#endif  // defined(CPPGC_YOUNG_GENERATION)
 
 #ifdef VERIFY_HEAP
   if (FLAG_verify_heap) {
@@ -2432,7 +2436,7 @@ void Heap::PerformSharedGarbageCollection(Isolate* initiator,
   tracer()->StopAtomicPause();
   tracer()->StopObservablePause();
   tracer()->UpdateStatistics(collector);
-  tracer()->StopCycleIfNeeded();
+  tracer()->StopFullCycleIfNeeded();
 }
 
 void Heap::CompleteSweepingYoung(GarbageCollector collector) {
@@ -2458,6 +2462,11 @@ void Heap::CompleteSweepingYoung(GarbageCollector collector) {
   // the sweeping here, to avoid having to pause and resume during the young
   // generation GC.
   mark_compact_collector()->FinishSweepingIfOutOfWork();
+
+#if defined(CPPGC_YOUNG_GENERATION)
+  // Always complete sweeping if young generation is enabled.
+  if (cpp_heap()) CppHeap::From(cpp_heap())->FinishSweepingIfRunning();
+#endif  // defined(CPPGC_YOUNG_GENERATION)
 }
 
 void Heap::EnsureSweepingCompleted(HeapObject object) {

test/cctest/heap/test-heap.cc

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

test/unittests/heap/embedder-tracing-unittest.cc

@@ -502,7 +502,7 @@ TEST_F(EmbedderTracingTest, FinalizeTracingWhenMarking) {
     heap->mark_compact_collector()->EnsureSweepingCompleted(
         MarkCompactCollector::SweepingForcedFinalizationMode::kV8Only);
   }
-  heap->tracer()->StopCycleIfNeeded();
+  heap->tracer()->StopFullCycleIfNeeded();
   EXPECT_TRUE(heap->incremental_marking()->IsStopped());
 
   i::IncrementalMarking* marking = heap->incremental_marking();

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

@@ -102,7 +102,7 @@ void StopTracing(GCTracer* tracer, GarbageCollector collector) {
   tracer->StopObservablePause();
   tracer->UpdateStatistics(collector);
   if (Heap::IsYoungGenerationCollector(collector)) {
-    tracer->StopCycle(collector);
+    tracer->StopYoungCycleIfNeeded();
   } else {
     tracer->NotifySweepingCompleted();
   }