Reland "cppgc: Parallel marking in atomic pause"

This is a reland of 6747144c82

Original change's description:
> cppgc: Parallel marking in atomic pause
>
> Bug: v8:12424
> Change-Id: I0633e1bd8c890c14ce2c5519253a5e6eb7592f04
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3295580
> Commit-Queue: Omer Katz <omerkatz@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#78081}

Bug: v8:12424
Change-Id: I66a030b4e66647a76bbe3d114785d3052358b403
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3301477
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78092}

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

@@ -331,7 +331,9 @@ CppHeap::CppHeap(
     : cppgc::internal::HeapBase(
           std::make_shared<CppgcPlatformAdapter>(platform), custom_spaces,
           cppgc::internal::HeapBase::StackSupport::
-              kSupportsConservativeStackScan),
+              kSupportsConservativeStackScan,
+          cppgc::internal::HeapBase::MarkingType::kIncrementalAndConcurrent,
+          cppgc::internal::HeapBase::SweepingType::kIncrementalAndConcurrent),
       wrapper_descriptor_(wrapper_descriptor) {
   CHECK_NE(WrapperDescriptor::kUnknownEmbedderId,
            wrapper_descriptor_.embedder_id_for_garbage_collected);
@@ -480,10 +482,8 @@ void CppHeap::EnterFinalPause(EmbedderStackState stack_state) {
     stack_state = *override_stack_state_;
   }
   marker_->EnterAtomicPause(stack_state);
-  if (compactor_.CancelIfShouldNotCompact(cppgc::Heap::MarkingType::kAtomic,
-                                          stack_state)) {
-    marker_->NotifyCompactionCancelled();
-  }
+  compactor_.CancelIfShouldNotCompact(cppgc::Heap::MarkingType::kAtomic,
+                                      stack_state);
 }
 
 void CppHeap::TraceEpilogue(TraceSummary* trace_summary) {

src/heap/cppgc/compactor.cc

@@ -474,6 +474,7 @@ void Compactor::InitializeIfShouldCompact(
   compaction_worklists_ = std::make_unique<CompactionWorklists>();
 
   is_enabled_ = true;
+  is_cancelled_ = false;
 }
 
 bool Compactor::CancelIfShouldNotCompact(
@@ -481,15 +482,16 @@ bool Compactor::CancelIfShouldNotCompact(
     GarbageCollector::Config::StackState stack_state) {
   if (!is_enabled_ || ShouldCompact(marking_type, stack_state)) return false;
 
-  DCHECK_NOT_NULL(compaction_worklists_);
-  compaction_worklists_->movable_slots_worklist()->Clear();
-  compaction_worklists_.reset();
-
+  is_cancelled_ = true;
   is_enabled_ = false;
   return true;
 }
 
 Compactor::CompactableSpaceHandling Compactor::CompactSpacesIfEnabled() {
+  if (is_cancelled_ && compaction_worklists_) {
+    compaction_worklists_->movable_slots_worklist()->Clear();
+    compaction_worklists_.reset();
+  }
   if (!is_enabled_) return CompactableSpaceHandling::kSweep;
 
   StatsCollector::EnabledScope stats_scope(heap_.heap()->stats_collector(),

src/heap/cppgc/compactor.h

@@ -48,6 +48,7 @@ class V8_EXPORT_PRIVATE Compactor final {
   std::unique_ptr<CompactionWorklists> compaction_worklists_;
 
   bool is_enabled_ = false;
+  bool is_cancelled_ = false;
   bool enable_for_next_gc_for_testing_ = false;
 };
 

src/heap/cppgc/heap-base.cc

@@ -53,7 +53,8 @@ class ObjectSizeCounter : private HeapVisitor<ObjectSizeCounter> {
 HeapBase::HeapBase(
     std::shared_ptr<cppgc::Platform> platform,
     const std::vector<std::unique_ptr<CustomSpaceBase>>& custom_spaces,
-    StackSupport stack_support)
+    StackSupport stack_support, MarkingType marking_support,
+    SweepingType sweeping_support)
     : raw_heap_(this, custom_spaces),
       platform_(std::move(platform)),
       oom_handler_(std::make_unique<FatalOutOfMemoryHandler>(this)),
@@ -81,7 +82,9 @@ HeapBase::HeapBase(
       weak_persistent_region_(*oom_handler_.get()),
       strong_cross_thread_persistent_region_(*oom_handler_.get()),
       weak_cross_thread_persistent_region_(*oom_handler_.get()),
-      stack_support_(stack_support) {
+      stack_support_(stack_support),
+      marking_support_(marking_support),
+      sweeping_support_(sweeping_support) {
   stats_collector_->RegisterObserver(
       &allocation_observer_for_PROCESS_HEAP_STATISTICS_);
 }

src/heap/cppgc/heap-base.h

@@ -71,6 +71,8 @@ class StatsCollector;
 class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
  public:
   using StackSupport = cppgc::Heap::StackSupport;
+  using MarkingType = cppgc::Heap::MarkingType;
+  using SweepingType = cppgc::Heap::SweepingType;
 
   static HeapBase& From(cppgc::HeapHandle& heap_handle) {
     return static_cast<HeapBase&>(heap_handle);
@@ -81,7 +83,8 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
 
   HeapBase(std::shared_ptr<cppgc::Platform> platform,
            const std::vector<std::unique_ptr<CustomSpaceBase>>& custom_spaces,
-           StackSupport stack_support);
+           StackSupport stack_support, MarkingType marking_support,
+           SweepingType sweeping_support);
   virtual ~HeapBase();
 
   HeapBase(const HeapBase&) = delete;
@@ -203,6 +206,8 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
 
   int GetCreationThreadId() const { return creation_thread_id_; }
 
+  MarkingType marking_support() const { return marking_support_; }
+
  protected:
   // Used by the incremental scheduler to finalize a GC if supported.
   virtual void FinalizeIncrementalGarbageCollectionIfNeeded(
@@ -273,6 +278,9 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
 
   int creation_thread_id_ = v8::base::OS::GetCurrentThreadId();
 
+  const MarkingType marking_support_;
+  const SweepingType sweeping_support_;
+
   friend class MarkerBase::IncrementalMarkingTask;
   friend class cppgc::subtle::DisallowGarbageCollectionScope;
   friend class cppgc::subtle::NoGarbageCollectionScope;

src/heap/cppgc/heap.cc

@@ -62,8 +62,8 @@ namespace internal {
 
 namespace {
 
-void CheckConfig(Heap::Config config, Heap::MarkingType marking_support,
-                 Heap::SweepingType sweeping_support) {
+void CheckConfig(Heap::Config config, HeapBase::MarkingType marking_support,
+                 HeapBase::SweepingType sweeping_support) {
   CHECK_WITH_MSG(
       (config.collection_type != Heap::Config::CollectionType::kMinor) ||
           (config.stack_state == Heap::Config::StackState::kNoHeapPointers),
@@ -78,13 +78,12 @@ void CheckConfig(Heap::Config config, Heap::MarkingType marking_support,
 
 Heap::Heap(std::shared_ptr<cppgc::Platform> platform,
            cppgc::Heap::HeapOptions options)
-    : HeapBase(platform, options.custom_spaces, options.stack_support),
+    : HeapBase(platform, options.custom_spaces, options.stack_support,
+               options.marking_support, options.sweeping_support),
       gc_invoker_(this, platform_.get(), options.stack_support),
       growing_(&gc_invoker_, stats_collector_.get(),
                options.resource_constraints, options.marking_support,
-               options.sweeping_support),
-      marking_support_(options.marking_support),
-      sweeping_support_(options.sweeping_support) {
+               options.sweeping_support) {
   CHECK_IMPLIES(options.marking_support != MarkingType::kAtomic,
                 platform_->GetForegroundTaskRunner());
   CHECK_IMPLIES(options.sweeping_support != SweepingType::kAtomic,

src/heap/cppgc/heap.h

@@ -53,9 +53,6 @@ class V8_EXPORT_PRIVATE Heap final : public HeapBase,
   GCInvoker gc_invoker_;
   HeapGrowing growing_;
 
-  const MarkingType marking_support_;
-  const SweepingType sweeping_support_;
-
   size_t epoch_ = 0;
 };
 

src/heap/cppgc/incremental-marking-schedule.cc

@@ -30,7 +30,6 @@ void IncrementalMarkingSchedule::UpdateMutatorThreadMarkedBytes(
 
 void IncrementalMarkingSchedule::AddConcurrentlyMarkedBytes(
     size_t marked_bytes) {
-  DCHECK(!incremental_marking_start_time_.IsNull());
   concurrently_marked_bytes_.fetch_add(marked_bytes, std::memory_order_relaxed);
 }
 

src/heap/cppgc/marker.cc

@@ -240,6 +240,7 @@ void MarkerBase::StartMarking() {
         MarkingConfig::MarkingType::kIncrementalAndConcurrent) {
       mutator_marking_state_.Publish();
       concurrent_marker_->Start();
+      concurrent_marking_active_ = true;
     }
     incremental_marking_allocation_observer_ =
         std::make_unique<IncrementalMarkingAllocationObserver>(*this);
@@ -255,8 +256,9 @@ void MarkerBase::EnterAtomicPause(MarkingConfig::StackState stack_state) {
                                            StatsCollector::kMarkAtomicPrologue);
 
   if (ExitIncrementalMarkingIfNeeded(config_, heap())) {
-    // Cancel remaining concurrent/incremental tasks.
-    concurrent_marker_->Cancel();
+    // Cancel remaining incremental tasks. Concurrent marking jobs are left to
+    // run in parallel with the atomic pause until the mutator thread runs out
+    // of work.
     incremental_marking_handle_.Cancel();
     heap().stats_collector()->UnregisterObserver(
         incremental_marking_allocation_observer_.get());
@@ -276,6 +278,17 @@ void MarkerBase::EnterAtomicPause(MarkingConfig::StackState stack_state) {
       MarkNotFullyConstructedObjects();
     }
   }
+  if (heap().marking_support() ==
+      MarkingConfig::MarkingType::kIncrementalAndConcurrent) {
+    // Start parallel marking.
+    mutator_marking_state_.Publish();
+    if (concurrent_marking_active_) {
+      concurrent_marker_->NotifyIncrementalMutatorStepCompleted();
+    } else {
+      concurrent_marker_->Start();
+      concurrent_marking_active_ = true;
+    }
+  }
 }
 
 void MarkerBase::LeaveAtomicPause() {
@@ -414,6 +427,16 @@ void MarkerBase::AdvanceMarkingOnAllocation() {
   }
 }
 
+bool MarkerBase::CancelConcurrentMarkingIfNeeded() {
+  if (config_.marking_type != MarkingConfig::MarkingType::kAtomic ||
+      !concurrent_marking_active_)
+    return false;
+
+  concurrent_marker_->Cancel();
+  concurrent_marking_active_ = false;
+  return true;
+}
+
 bool MarkerBase::AdvanceMarkingWithLimits(v8::base::TimeDelta max_duration,
                                           size_t marked_bytes_limit) {
   bool is_done = false;
@@ -433,6 +456,9 @@ bool MarkerBase::AdvanceMarkingWithLimits(v8::base::TimeDelta max_duration,
       // adjustment.
       is_done = ProcessWorklistsWithDeadline(marked_bytes_limit, deadline);
     }
+    if (is_done && CancelConcurrentMarkingIfNeeded()) {
+      is_done = ProcessWorklistsWithDeadline(marked_bytes_limit, deadline);
+    }
     schedule_.UpdateMutatorThreadMarkedBytes(
         mutator_marking_state_.marked_bytes());
   }
@@ -592,13 +618,6 @@ void MarkerBase::WaitForConcurrentMarkingForTesting() {
   concurrent_marker_->JoinForTesting();
 }
 
-void MarkerBase::NotifyCompactionCancelled() {
-  // Compaction cannot be cancelled while concurrent marking is active.
-  DCHECK_EQ(MarkingConfig::MarkingType::kAtomic, config_.marking_type);
-  DCHECK_IMPLIES(concurrent_marker_, !concurrent_marker_->IsActive());
-  mutator_marking_state_.NotifyCompactionCancelled();
-}
-
 Marker::Marker(Key key, HeapBase& heap, cppgc::Platform* platform,
                MarkingConfig config)
     : MarkerBase(key, heap, platform, config),

src/heap/cppgc/marker.h

@@ -133,8 +133,6 @@ class V8_EXPORT_PRIVATE MarkerBase {
 
   void WaitForConcurrentMarkingForTesting();
 
-  void NotifyCompactionCancelled();
-
   bool IsMarking() const { return is_marking_; }
 
  protected:
@@ -173,6 +171,8 @@ class V8_EXPORT_PRIVATE MarkerBase {
 
   void AdvanceMarkingOnAllocation();
 
+  bool CancelConcurrentMarkingIfNeeded();
+
   HeapBase& heap_;
   MarkingConfig config_ = MarkingConfig::Default();
 
@@ -189,6 +189,7 @@ class V8_EXPORT_PRIVATE MarkerBase {
   IncrementalMarkingSchedule schedule_;
 
   std::unique_ptr<ConcurrentMarkerBase> concurrent_marker_{nullptr};
+  bool concurrent_marking_active_ = false;
 
   bool main_marking_disabled_for_testing_{false};
   bool visited_cross_thread_persistents_in_atomic_pause_{false};

src/heap/cppgc/marking-state.h

@@ -110,12 +110,6 @@ class MarkingStateBase {
     return movable_slots_worklist_.get();
   }
 
-  void NotifyCompactionCancelled() {
-    DCHECK(IsCompactionEnabled());
-    movable_slots_worklist_->Clear();
-    movable_slots_worklist_.reset();
-  }
-
   bool DidDiscoverNewEphemeronPairs() const {
     return discovered_new_ephemeron_pairs_;
   }
@@ -415,15 +409,17 @@ void MutatorMarkingState::InvokeWeakRootsCallbackIfNeeded(
 #if DEBUG
   const HeapObjectHeader& header =
       HeapObjectHeader::FromObject(desc.base_object_payload);
-  DCHECK_IMPLIES(header.IsInConstruction(), header.IsMarked());
+  DCHECK_IMPLIES(header.IsInConstruction(),
+                 header.IsMarked<AccessMode::kAtomic>());
 #endif  // DEBUG
   weak_callback(LivenessBrokerFactory::Create(), parameter);
 }
 
 bool MutatorMarkingState::IsMarkedWeakContainer(HeapObjectHeader& header) {
-  const bool result = weak_containers_worklist_.Contains(&header) &&
-                      !recently_retraced_weak_containers_.Contains(&header);
-  DCHECK_IMPLIES(result, header.IsMarked());
+  const bool result =
+      weak_containers_worklist_.Contains<AccessMode::kAtomic>(&header) &&
+      !recently_retraced_weak_containers_.Contains(&header);
+  DCHECK_IMPLIES(result, header.IsMarked<AccessMode::kAtomic>());
   DCHECK_IMPLIES(result, !header.IsInConstruction());
   return result;
 }
@@ -493,7 +489,7 @@ template <AccessMode mode>
 void DynamicallyTraceMarkedObject(Visitor& visitor,
                                   const HeapObjectHeader& header) {
   DCHECK(!header.IsInConstruction<mode>());
-  DCHECK(header.IsMarked<mode>());
+  DCHECK(header.IsMarked<AccessMode::kAtomic>());
   header.Trace<mode>(&visitor);
 }
 

src/heap/cppgc/marking-visitor.cc

@@ -4,6 +4,7 @@
 
 #include "src/heap/cppgc/marking-visitor.h"
 
+#include "src/heap/cppgc/globals.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/marking-state.h"
 
@@ -54,7 +55,7 @@ ConservativeMarkingVisitor::ConservativeMarkingVisitor(
 
 void ConservativeMarkingVisitor::VisitFullyConstructedConservatively(
     HeapObjectHeader& header) {
-  if (header.IsMarked()) {
+  if (header.IsMarked<AccessMode::kAtomic>()) {
     if (marking_state_.IsMarkedWeakContainer(header))
       marking_state_.ReTraceMarkedWeakContainer(visitor_, header);
     return;

test/unittests/heap/cppgc/tests.h

@@ -87,8 +87,8 @@ class TestWithHeap : public TestWithPlatform {
   void ConservativeMemoryDiscardingGC() {
     internal::Heap::From(GetHeap())->CollectGarbage(
         {GarbageCollector::Config::CollectionType::kMajor,
-         Heap::StackState::kMayContainHeapPointers, Heap::MarkingType::kAtomic,
-         Heap::SweepingType::kAtomic,
+         Heap::StackState::kMayContainHeapPointers,
+         cppgc::Heap::MarkingType::kAtomic, cppgc::Heap::SweepingType::kAtomic,
          GarbageCollector::Config::FreeMemoryHandling::kDiscardWherePossible});
   }