cppgc: Eliminate marking boilerplate

Starting marking required Creating a Marker and calling StartMarking.
StartMarking should always have been called immediately after creating
the marker.
Since markers are not persisted between GC (a marker exists only while
marking is in progress), it makes sense to start marking implicitly when
a marker is created.

Calling StartMarking in MarkerBase ctor is inadvisable since subclasses
might still to initialize fields.
Using MarkerFactory instead guarantees that StartMarking is always
called immediately after creating a Marker.

Bug: chromium:1056170
Change-Id: Icbf11afd848e1618c204ca6bf951600b3ae9fef2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2375199
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69601}

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

@@ -65,7 +65,7 @@ class CppgcPlatformAdapter final : public cppgc::Platform {
 
 class UnifiedHeapMarker final : public cppgc::internal::MarkerBase {
  public:
-  UnifiedHeapMarker(Heap& v8_heap, cppgc::internal::HeapBase& cpp_heap,
+  UnifiedHeapMarker(Key, Heap& v8_heap, cppgc::internal::HeapBase& cpp_heap,
                     cppgc::Platform* platform, MarkingConfig config);
 
   ~UnifiedHeapMarker() final = default;
@@ -87,11 +87,11 @@ class UnifiedHeapMarker final : public cppgc::internal::MarkerBase {
   cppgc::internal::ConservativeMarkingVisitor conservative_marking_visitor_;
 };
 
-UnifiedHeapMarker::UnifiedHeapMarker(Heap& v8_heap,
+UnifiedHeapMarker::UnifiedHeapMarker(Key key, Heap& v8_heap,
                                      cppgc::internal::HeapBase& heap,
                                      cppgc::Platform* platform,
                                      MarkingConfig config)
-    : cppgc::internal::MarkerBase(heap, platform, config),
+    : cppgc::internal::MarkerBase(key, heap, platform, config),
       unified_heap_mutator_marking_state_(v8_heap),
       marking_visitor_(heap, mutator_marking_state_,
                        unified_heap_mutator_marking_state_),
@@ -128,9 +128,9 @@ void CppHeap::TracePrologue(TraceFlags flags) {
       UnifiedHeapMarker::MarkingConfig::CollectionType::kMajor,
       cppgc::Heap::StackState::kNoHeapPointers,
       UnifiedHeapMarker::MarkingConfig::MarkingType::kIncremental};
-  marker_ = std::make_unique<UnifiedHeapMarker>(
-      *isolate_.heap(), AsBase(), platform_.get(), marking_config);
-  marker_->StartMarking();
+  marker_ =
+      cppgc::internal::MarkerFactory::CreateAndStartMarking<UnifiedHeapMarker>(
+          *isolate_.heap(), AsBase(), platform_.get(), marking_config);
   marking_done_ = false;
 }
 

src/heap/cppgc/heap.cc

@@ -143,8 +143,8 @@ void Heap::StartGarbageCollection(Config config) {
 
   const Marker::MarkingConfig marking_config{
       config.collection_type, config.stack_state, config.marking_type};
-  marker_ = std::make_unique<Marker>(AsBase(), platform_.get(), marking_config);
-  marker_->StartMarking();
+  marker_ = MarkerFactory::CreateAndStartMarking<Marker>(
+      AsBase(), platform_.get(), marking_config);
 }
 
 void Heap::FinalizeGarbageCollection(Config::StackState stack_state) {

src/heap/cppgc/marker.cc

@@ -159,7 +159,7 @@ void MarkerBase::IncrementalMarkingTask::Run() {
   }
 }
 
-MarkerBase::MarkerBase(HeapBase& heap, cppgc::Platform* platform,
+MarkerBase::MarkerBase(Key, HeapBase& heap, cppgc::Platform* platform,
                        MarkingConfig config)
     : heap_(heap),
       config_(config),
@@ -380,8 +380,9 @@ void MarkerBase::ClearAllWorklistsForTesting() {
   marking_worklists_.ClearForTesting();
 }
 
-Marker::Marker(HeapBase& heap, cppgc::Platform* platform, MarkingConfig config)
-    : MarkerBase(heap, platform, config),
+Marker::Marker(Key key, HeapBase& heap, cppgc::Platform* platform,
+               MarkingConfig config)
+    : MarkerBase(key, heap, platform, config),
       marking_visitor_(heap, mutator_marking_state_),
       conservative_marking_visitor_(heap, mutator_marking_state_,
                                     marking_visitor_) {}

src/heap/cppgc/marker.h

@@ -22,10 +22,12 @@ namespace cppgc {
 namespace internal {
 
 class HeapBase;
+class MarkerFactory;
 
 // Marking algorithm. Example for a valid call sequence creating the marking
 // phase:
-// 1. StartMarking()
+// 1. StartMarking() [Called implicitly when creating a Marker using
+//                    MarkerFactory]
 // 2. AdvanceMarkingWithDeadline() [Optional, depending on environment.]
 // 3. EnterAtomicPause()
 // 4. AdvanceMarkingWithDeadline()
@@ -58,10 +60,6 @@ class V8_EXPORT_PRIVATE MarkerBase {
   MarkerBase(const MarkerBase&) = delete;
   MarkerBase& operator=(const MarkerBase&) = delete;
 
-  // Initialize marking according to the given config. This method will
-  // trigger incremental/concurrent marking if needed.
-  void StartMarking();
-
   // Signals entering the atomic marking pause. The method
   // - stops incremental/concurrent marking;
   // - flushes back any in-construction worklists if needed;
@@ -122,7 +120,17 @@ class V8_EXPORT_PRIVATE MarkerBase {
       v8::base::TimeDelta::FromMilliseconds(2);
   static constexpr size_t kMinimumMarkedBytesPerIncrementalStep = 64 * kKB;
 
-  MarkerBase(HeapBase&, cppgc::Platform*, MarkingConfig);
+  class Key {
+   private:
+    Key() = default;
+    friend class MarkerFactory;
+  };
+
+  MarkerBase(Key, HeapBase&, cppgc::Platform*, MarkingConfig);
+
+  // Initialize marking according to the given config. This method will
+  // trigger incremental/concurrent marking if needed.
+  void StartMarking();
 
   virtual cppgc::Visitor& visitor() = 0;
   virtual ConservativeTracingVisitor& conservative_visitor() = 0;
@@ -148,11 +156,27 @@ class V8_EXPORT_PRIVATE MarkerBase {
   MarkingWorklists marking_worklists_;
   MarkingState mutator_marking_state_;
   bool is_marking_started_ = false;
+
+  friend class MarkerFactory;
+};
+
+class V8_EXPORT_PRIVATE MarkerFactory {
+ public:
+  template <typename T, typename... Args>
+  static std::unique_ptr<T> CreateAndStartMarking(Args&&... args) {
+    static_assert(std::is_base_of<MarkerBase, T>::value,
+                  "MarkerFactory can only create subclasses of MarkerBase");
+    std::unique_ptr<T> marker =
+        std::make_unique<T>(MarkerBase::Key(), std::forward<Args>(args)...);
+    marker->StartMarking();
+    return marker;
+  }
 };
 
 class V8_EXPORT_PRIVATE Marker final : public MarkerBase {
  public:
-  Marker(HeapBase&, cppgc::Platform*, MarkingConfig = MarkingConfig::Default());
+  Marker(Key, HeapBase&, cppgc::Platform*,
+         MarkingConfig = MarkingConfig::Default());
 
  protected:
   cppgc::Visitor& visitor() final { return marking_visitor_; }

test/unittests/heap/cppgc/marker-unittest.cc

@@ -18,7 +18,6 @@ namespace cppgc {
 namespace internal {
 
 namespace {
-
 class MarkerTest : public testing::TestWithHeap {
  public:
   using MarkingConfig = Marker::MarkingConfig;
@@ -27,14 +26,24 @@ class MarkerTest : public testing::TestWithHeap {
     const MarkingConfig config = {MarkingConfig::CollectionType::kMajor,
                                   stack_state};
     auto* heap = Heap::From(GetHeap());
-    Marker marker(*heap, GetPlatformHandle().get(), config);
-    marker.StartMarking();
-    marker.FinishMarking(stack_state);
-    marker.ProcessWeakness();
+    InitializeMarker(*heap, GetPlatformHandle().get(), config);
+    marker_->FinishMarking(stack_state);
+    marker_->ProcessWeakness();
     // Pretend do finish sweeping as StatsCollector verifies that Notify*
     // methods are called in the right order.
     heap->stats_collector()->NotifySweepingCompleted();
   }
+
+  void InitializeMarker(HeapBase& heap, cppgc::Platform* platform,
+                        MarkingConfig config) {
+    marker_ =
+        MarkerFactory::CreateAndStartMarking<Marker>(heap, platform, config);
+  }
+
+  Marker* marker() const { return marker_.get(); }
+
+ private:
+  std::unique_ptr<Marker> marker_;
 };
 
 class GCed : public GarbageCollected<GCed> {
@@ -219,15 +228,14 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedEmptyStack) {
   static const Marker::MarkingConfig config = {
       MarkingConfig::CollectionType::kMajor,
       MarkingConfig::StackState::kMayContainHeapPointers};
-  Marker marker(*Heap::From(GetHeap()), GetPlatformHandle().get(), config);
-  marker.StartMarking();
+  InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get(), config);
   GCedWithCallback* object = MakeGarbageCollected<GCedWithCallback>(
-      GetAllocationHandle(), [&marker](GCedWithCallback* obj) {
+      GetAllocationHandle(), [marker = marker()](GCedWithCallback* obj) {
         Member<GCedWithCallback> member(obj);
-        marker.VisitorForTesting().Trace(member);
+        marker->VisitorForTesting().Trace(member);
       });
   EXPECT_TRUE(HeapObjectHeader::FromPayload(object).IsMarked());
-  marker.FinishMarking(MarkingConfig::StackState::kMayContainHeapPointers);
+  marker()->FinishMarking(MarkingConfig::StackState::kMayContainHeapPointers);
   EXPECT_TRUE(HeapObjectHeader::FromPayload(object).IsMarked());
 }
 
@@ -235,14 +243,13 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedNonEmptyStack) {
   static const Marker::MarkingConfig config = {
       MarkingConfig::CollectionType::kMajor,
       MarkingConfig::StackState::kMayContainHeapPointers};
-  Marker marker(*Heap::From(GetHeap()), GetPlatformHandle().get(), config);
-  marker.StartMarking();
+  InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get(), config);
   MakeGarbageCollected<GCedWithCallback>(
-      GetAllocationHandle(), [&marker](GCedWithCallback* obj) {
+      GetAllocationHandle(), [marker = marker()](GCedWithCallback* obj) {
         Member<GCedWithCallback> member(obj);
-        marker.VisitorForTesting().Trace(member);
+        marker->VisitorForTesting().Trace(member);
         EXPECT_TRUE(HeapObjectHeader::FromPayload(obj).IsMarked());
-        marker.FinishMarking(
+        marker->FinishMarking(
             MarkingConfig::StackState::kMayContainHeapPointers);
         EXPECT_TRUE(HeapObjectHeader::FromPayload(obj).IsMarked());
       });
@@ -252,14 +259,13 @@ TEST_F(MarkerTest, SentinelNotClearedOnWeakPersistentHandling) {
   static const Marker::MarkingConfig config = {
       MarkingConfig::CollectionType::kMajor,
       MarkingConfig::StackState::kNoHeapPointers};
-  Marker marker(*Heap::From(GetHeap()), GetPlatformHandle().get(), config);
+  InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get(), config);
   Persistent<GCed> root = MakeGarbageCollected<GCed>(GetAllocationHandle());
   auto* tmp = MakeGarbageCollected<GCed>(GetAllocationHandle());
   root->SetWeakChild(tmp);
-  marker.StartMarking();
-  marker.FinishMarking(MarkingConfig::StackState::kNoHeapPointers);
+  marker()->FinishMarking(MarkingConfig::StackState::kNoHeapPointers);
   root->SetWeakChild(kSentinelPointer);
-  marker.ProcessWeakness();
+  marker()->ProcessWeakness();
   EXPECT_EQ(kSentinelPointer, root->weak_child());
 }
 
@@ -278,37 +284,47 @@ class IncrementalMarkingTest : public testing::TestWithHeap {
       MarkingConfig::StackState::kMayContainHeapPointers,
       MarkingConfig::MarkingType::kIncremental};
 
-  void FinishSteps(Marker& marker, MarkingConfig::StackState stack_state) {
-    SingleStep(marker, stack_state, std::numeric_limits<size_t>::max());
+  void FinishSteps(MarkingConfig::StackState stack_state) {
+    SingleStep(stack_state, std::numeric_limits<size_t>::max());
   }
 
-  void FinishMarking(Marker& marker) {
-    marker.FinishMarking(MarkingConfig::StackState::kMayContainHeapPointers);
-    marker.ProcessWeakness();
+  void FinishMarking() {
+    marker_->FinishMarking(MarkingConfig::StackState::kMayContainHeapPointers);
+    marker_->ProcessWeakness();
     // Pretend do finish sweeping as StatsCollector verifies that Notify*
     // methods are called in the right order.
     Heap::From(GetHeap())->stats_collector()->NotifySweepingCompleted();
   }
 
+  void InitializeMarker(HeapBase& heap, cppgc::Platform* platform,
+                        MarkingConfig config) {
+    marker_ =
+        MarkerFactory::CreateAndStartMarking<Marker>(heap, platform, config);
+  }
+
+  Marker* marker() const { return marker_.get(); }
+
  private:
-  bool SingleStep(Marker& marker, MarkingConfig::StackState stack_state,
-                  size_t bytes_to_mark) {
-    return marker.IncrementalMarkingStepForTesting(stack_state, bytes_to_mark);
+  bool SingleStep(MarkingConfig::StackState stack_state, size_t bytes_to_mark) {
+    return marker_->IncrementalMarkingStepForTesting(stack_state,
+                                                     bytes_to_mark);
   }
+
+  std::unique_ptr<Marker> marker_;
 };
+
 constexpr IncrementalMarkingTest::MarkingConfig
     IncrementalMarkingTest::IncrementalPreciseMarkingConfig;
 constexpr IncrementalMarkingTest::MarkingConfig
     IncrementalMarkingTest::IncrementalConservativeMarkingConfig;
 
-TEST_F(IncrementalMarkingTest, RootIsMarkedAfterStartMarking) {
+TEST_F(IncrementalMarkingTest, RootIsMarkedAfterMarkingStarted) {
   Persistent<GCed> root = MakeGarbageCollected<GCed>(GetAllocationHandle());
   EXPECT_FALSE(HeapObjectHeader::FromPayload(root).IsMarked());
-  Marker marker(*Heap::From(GetHeap()), GetPlatformHandle().get(),
-                IncrementalPreciseMarkingConfig);
-  marker.StartMarking();
+  InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get(),
+                   IncrementalPreciseMarkingConfig);
   EXPECT_TRUE(HeapObjectHeader::FromPayload(root).IsMarked());
-  FinishMarking(marker);
+  FinishMarking();
 }
 
 TEST_F(IncrementalMarkingTest, MemberIsMarkedAfterMarkingSteps) {
@@ -316,26 +332,24 @@ TEST_F(IncrementalMarkingTest, MemberIsMarkedAfterMarkingSteps) {
   root->SetChild(MakeGarbageCollected<GCed>(GetAllocationHandle()));
   HeapObjectHeader& header = HeapObjectHeader::FromPayload(root->child());
   EXPECT_FALSE(header.IsMarked());
-  Marker marker(*Heap::From(GetHeap()), GetPlatformHandle().get(),
-                IncrementalPreciseMarkingConfig);
-  marker.StartMarking();
-  FinishSteps(marker, MarkingConfig::StackState::kNoHeapPointers);
+  InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get(),
+                   IncrementalPreciseMarkingConfig);
+  FinishSteps(MarkingConfig::StackState::kNoHeapPointers);
   EXPECT_TRUE(header.IsMarked());
-  FinishMarking(marker);
+  FinishMarking();
 }
 
 TEST_F(IncrementalMarkingTest,
        MemberWithWriteBarrierIsMarkedAfterMarkingSteps) {
   Persistent<GCed> root = MakeGarbageCollected<GCed>(GetAllocationHandle());
-  Marker marker(*Heap::From(GetHeap()), GetPlatformHandle().get(),
-                IncrementalPreciseMarkingConfig);
-  marker.StartMarking();
+  InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get(),
+                   IncrementalPreciseMarkingConfig);
   root->SetChild(MakeGarbageCollected<GCed>(GetAllocationHandle()));
   HeapObjectHeader& header = HeapObjectHeader::FromPayload(root->child());
   EXPECT_FALSE(header.IsMarked());
-  FinishSteps(marker, MarkingConfig::StackState::kNoHeapPointers);
+  FinishSteps(MarkingConfig::StackState::kNoHeapPointers);
   EXPECT_TRUE(header.IsMarked());
-  FinishMarking(marker);
+  FinishMarking();
 }
 
 namespace {
@@ -350,22 +364,20 @@ class Holder : public GarbageCollected<Holder> {
 TEST_F(IncrementalMarkingTest, IncrementalStepDuringAllocation) {
   Persistent<Holder> holder =
       MakeGarbageCollected<Holder>(GetAllocationHandle());
-  Marker marker(*Heap::From(GetHeap()), GetPlatformHandle().get(),
-                IncrementalPreciseMarkingConfig);
-  marker.StartMarking();
+  InitializeMarker(*Heap::From(GetHeap()), GetPlatformHandle().get(),
+                   IncrementalPreciseMarkingConfig);
   const HeapObjectHeader* header;
   MakeGarbageCollected<GCedWithCallback>(
-      GetAllocationHandle(),
-      [this, &holder, &header, &marker](GCedWithCallback* obj) {
+      GetAllocationHandle(), [this, &holder, &header](GCedWithCallback* obj) {
         header = &HeapObjectHeader::FromPayload(obj);
         holder->member_ = obj;
         EXPECT_FALSE(header->IsMarked());
-        FinishSteps(marker, MarkingConfig::StackState::kMayContainHeapPointers);
+        FinishSteps(MarkingConfig::StackState::kMayContainHeapPointers);
         EXPECT_TRUE(header->IsMarked());
       });
-  FinishSteps(marker, MarkingConfig::StackState::kNoHeapPointers);
+  FinishSteps(MarkingConfig::StackState::kNoHeapPointers);
   EXPECT_TRUE(header->IsMarked());
-  FinishMarking(marker);
+  FinishMarking();
 }
 
 }  // namespace internal

test/unittests/heap/cppgc/marking-visitor-unittest.cc

@@ -23,8 +23,8 @@ namespace {
 class MarkingVisitorTest : public testing::TestWithHeap {
  public:
   MarkingVisitorTest()
-      : marker_(std::make_unique<Marker>(*Heap::From(GetHeap()),
-                                         GetPlatformHandle().get())) {}
+      : marker_(MarkerFactory::CreateAndStartMarking<Marker>(
+            *Heap::From(GetHeap()), GetPlatformHandle().get())) {}
   ~MarkingVisitorTest() override { marker_->ClearAllWorklistsForTesting(); }
 
   Marker* GetMarker() { return marker_.get(); }

test/unittests/heap/cppgc/write-barrier-unittest.cc

@@ -21,9 +21,7 @@ namespace {
 
 class IncrementalMarkingScope {
  public:
-  explicit IncrementalMarkingScope(MarkerBase* marker) : marker_(marker) {
-    marker_->StartMarking();
-  }
+  explicit IncrementalMarkingScope(MarkerBase* marker) : marker_(marker) {}
 
   ~IncrementalMarkingScope() V8_NOEXCEPT {
     marker_->FinishMarking(kIncrementalConfig.stack_state);
@@ -149,9 +147,9 @@ class GCed : public GarbageCollected<GCed> {
 class WriteBarrierTest : public testing::TestWithHeap {
  public:
   WriteBarrierTest() : internal_heap_(Heap::From(GetHeap())) {
-    GetMarkerRef() =
-        std::make_unique<Marker>(*internal_heap_, GetPlatformHandle().get(),
-                                 IncrementalMarkingScope::kIncrementalConfig);
+    GetMarkerRef() = MarkerFactory::CreateAndStartMarking<Marker>(
+        *internal_heap_, GetPlatformHandle().get(),
+        IncrementalMarkingScope::kIncrementalConfig);
     marker_ = GetMarkerRef().get();
   }
 
@@ -167,6 +165,8 @@ class WriteBarrierTest : public testing::TestWithHeap {
   MarkerBase* marker_;
 };
 
+class NoWriteBarrierTest : public testing::TestWithHeap {};
+
 // =============================================================================
 // Basic support. ==============================================================
 // =============================================================================
@@ -189,7 +189,7 @@ TEST_F(WriteBarrierTest, TriggersWhenMarkingIsOn) {
   }
 }
 
-TEST_F(WriteBarrierTest, BailoutWhenMarkingIsOff) {
+TEST_F(NoWriteBarrierTest, BailoutWhenMarkingIsOff) {
   auto* object1 = MakeGarbageCollected<GCed>(GetAllocationHandle());
   auto* object2 = MakeGarbageCollected<GCed>(GetAllocationHandle());
   EXPECT_FALSE(object1->IsMarked());