cppgc: Split of roots visitation from regular Visitor

Introduce RootVisitor and related class hierarchy to just handle
roots. This avoids the awkard definitions for roots visiation in all
the cases they are not needed.

Change-Id: Ib0912e4bf543db2ecf68caead6929c68d6afdda6
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3782794Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82107}

include/cppgc/cross-thread-persistent.h

@@ -120,7 +120,7 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
     if (!IsValid(raw)) return;
     PersistentRegionLock guard;
     CrossThreadPersistentRegion& region = this->GetPersistentRegion(raw);
-    SetNode(region.AllocateNode(this, &Trace));
+    SetNode(region.AllocateNode(this, &TraceAsRoot));
     this->CheckPointer(raw);
   }
 
@@ -138,7 +138,7 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
       : CrossThreadPersistentBase(raw), LocationPolicy(loc) {
     if (!IsValid(raw)) return;
     CrossThreadPersistentRegion& region = this->GetPersistentRegion(raw);
-    SetNode(region.AllocateNode(this, &Trace));
+    SetNode(region.AllocateNode(this, &TraceAsRoot));
     this->CheckPointer(raw);
   }
 
@@ -349,9 +349,8 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
     return ptr && ptr != kSentinelPointer;
   }
 
-  static void Trace(Visitor* v, const void* ptr) {
-    const auto* handle = static_cast<const BasicCrossThreadPersistent*>(ptr);
-    v->TraceRoot(*handle, handle->Location());
+  static void TraceAsRoot(RootVisitor& root_visitor, const void* ptr) {
+    root_visitor.Trace(*static_cast<const BasicCrossThreadPersistent*>(ptr));
   }
 
   void AssignUnsafe(T* ptr) {
@@ -378,7 +377,7 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
     SetValue(ptr);
     if (!IsValid(ptr)) return;
     PersistentRegionLock guard;
-    SetNode(this->GetPersistentRegion(ptr).AllocateNode(this, &Trace));
+    SetNode(this->GetPersistentRegion(ptr).AllocateNode(this, &TraceAsRoot));
     this->CheckPointer(ptr);
   }
 
@@ -398,7 +397,7 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
     }
     SetValue(ptr);
     if (!IsValid(ptr)) return;
-    SetNode(this->GetPersistentRegion(ptr).AllocateNode(this, &Trace));
+    SetNode(this->GetPersistentRegion(ptr).AllocateNode(this, &TraceAsRoot));
     this->CheckPointer(ptr);
   }
 
@@ -416,7 +415,7 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
     return static_cast<T*>(const_cast<void*>(GetValueFromGC()));
   }
 
-  friend class cppgc::Visitor;
+  friend class internal::RootVisitor;
 };
 
 template <typename T, typename LocationPolicy, typename CheckingPolicy>

include/cppgc/internal/persistent-node.h

@@ -14,13 +14,11 @@
 #include "v8config.h"  // NOLINT(build/include_directory)
 
 namespace cppgc {
-
-class Visitor;
-
 namespace internal {
 
 class CrossThreadPersistentRegion;
 class FatalOutOfMemoryHandler;
+class RootVisitor;
 
 // PersistentNode represents a variant of two states:
 // 1) traceable node with a back pointer to the Persistent object;
@@ -32,7 +30,7 @@ class PersistentNode final {
   PersistentNode(const PersistentNode&) = delete;
   PersistentNode& operator=(const PersistentNode&) = delete;
 
-  void InitializeAsUsedNode(void* owner, TraceCallback trace) {
+  void InitializeAsUsedNode(void* owner, TraceRootCallback trace) {
     CPPGC_DCHECK(trace);
     owner_ = owner;
     trace_ = trace;
@@ -53,9 +51,9 @@ class PersistentNode final {
     return next_;
   }
 
-  void Trace(Visitor* visitor) const {
+  void Trace(RootVisitor& root_visitor) const {
     CPPGC_DCHECK(IsUsed());
-    trace_(visitor, owner_);
+    trace_(root_visitor, owner_);
   }
 
   bool IsUsed() const { return trace_; }
@@ -73,7 +71,7 @@ class PersistentNode final {
     void* owner_ = nullptr;
     PersistentNode* next_;
   };
-  TraceCallback trace_ = nullptr;
+  TraceRootCallback trace_ = nullptr;
 };
 
 class V8_EXPORT PersistentRegionBase {
@@ -86,7 +84,7 @@ class V8_EXPORT PersistentRegionBase {
   PersistentRegionBase(const PersistentRegionBase&) = delete;
   PersistentRegionBase& operator=(const PersistentRegionBase&) = delete;
 
-  void Trace(Visitor*);
+  void Iterate(RootVisitor&);
 
   size_t NodesInUse() const;
 
@@ -96,7 +94,7 @@ class V8_EXPORT PersistentRegionBase {
   explicit PersistentRegionBase(const FatalOutOfMemoryHandler& oom_handler);
 
   PersistentNode* TryAllocateNodeFromFreeList(void* owner,
-                                              TraceCallback trace) {
+                                              TraceRootCallback trace) {
     PersistentNode* node = nullptr;
     if (V8_LIKELY(free_list_head_)) {
       node = free_list_head_;
@@ -118,7 +116,7 @@ class V8_EXPORT PersistentRegionBase {
   }
 
   PersistentNode* RefillFreeListAndAllocateNode(void* owner,
-                                                TraceCallback trace);
+                                                TraceRootCallback trace);
 
  private:
   template <typename PersistentBaseClass>
@@ -145,7 +143,7 @@ class V8_EXPORT PersistentRegion final : public PersistentRegionBase {
   PersistentRegion(const PersistentRegion&) = delete;
   PersistentRegion& operator=(const PersistentRegion&) = delete;
 
-  V8_INLINE PersistentNode* AllocateNode(void* owner, TraceCallback trace) {
+  V8_INLINE PersistentNode* AllocateNode(void* owner, TraceRootCallback trace) {
     CPPGC_DCHECK(IsCreationThread());
     auto* node = TryAllocateNodeFromFreeList(owner, trace);
     if (V8_LIKELY(node)) return node;
@@ -189,7 +187,7 @@ class V8_EXPORT CrossThreadPersistentRegion final
   CrossThreadPersistentRegion& operator=(const CrossThreadPersistentRegion&) =
       delete;
 
-  V8_INLINE PersistentNode* AllocateNode(void* owner, TraceCallback trace) {
+  V8_INLINE PersistentNode* AllocateNode(void* owner, TraceRootCallback trace) {
     PersistentRegionLock::AssertLocked();
     auto* node = TryAllocateNodeFromFreeList(owner, trace);
     if (V8_LIKELY(node)) return node;
@@ -202,7 +200,7 @@ class V8_EXPORT CrossThreadPersistentRegion final
     PersistentRegionBase::FreeNode(node);
   }
 
-  void Trace(Visitor*);
+  void Iterate(RootVisitor&);
 
   size_t NodesInUse() const;
 

include/cppgc/persistent.h

@@ -16,9 +16,6 @@
 #include "v8config.h"  // NOLINT(build/include_directory)
 
 namespace cppgc {
-
-class Visitor;
-
 namespace internal {
 
 // PersistentBase always refers to the object as const object and defers to
@@ -78,7 +75,7 @@ class BasicPersistent final : public PersistentBase,
       : PersistentBase(raw), LocationPolicy(loc) {
     if (!IsValid()) return;
     SetNode(WeaknessPolicy::GetPersistentRegion(GetValue())
-                .AllocateNode(this, &BasicPersistent::Trace));
+                .AllocateNode(this, &TraceAsRoot));
     this->CheckPointer(Get());
   }
 
@@ -221,9 +218,8 @@ class BasicPersistent final : public PersistentBase,
   }
 
  private:
-  static void Trace(Visitor* v, const void* ptr) {
-    const auto* persistent = static_cast<const BasicPersistent*>(ptr);
-    v->TraceRoot(*persistent, persistent->Location());
+  static void TraceAsRoot(RootVisitor& root_visitor, const void* ptr) {
+    root_visitor.Trace(*static_cast<const BasicPersistent*>(ptr));
   }
 
   bool IsValid() const {
@@ -247,7 +243,7 @@ class BasicPersistent final : public PersistentBase,
     SetValue(ptr);
     if (!IsValid()) return;
     SetNode(WeaknessPolicy::GetPersistentRegion(GetValue())
-                .AllocateNode(this, &BasicPersistent::Trace));
+                .AllocateNode(this, &TraceAsRoot));
     this->CheckPointer(Get());
   }
 
@@ -264,7 +260,7 @@ class BasicPersistent final : public PersistentBase,
     return static_cast<T*>(const_cast<void*>(GetValue()));
   }
 
-  friend class cppgc::Visitor;
+  friend class internal::RootVisitor;
 };
 
 template <typename T1, typename WeaknessPolicy1, typename LocationPolicy1,

include/cppgc/trace-trait.h

@@ -16,6 +16,10 @@ class Visitor;
 
 namespace internal {
 
+class RootVisitor;
+
+using TraceRootCallback = void (*)(RootVisitor&, const void* object);
+
 // Implementation of the default TraceTrait handling GarbageCollected and
 // GarbageCollectedMixin.
 template <typename T,

include/cppgc/visitor.h

@@ -303,9 +303,6 @@ class V8_EXPORT Visitor {
   virtual void Visit(const void* self, TraceDescriptor) {}
   virtual void VisitWeak(const void* self, TraceDescriptor, WeakCallback,
                          const void* weak_member) {}
-  virtual void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) {}
-  virtual void VisitWeakRoot(const void* self, TraceDescriptor, WeakCallback,
-                             const void* weak_root, const SourceLocation&) {}
   virtual void VisitEphemeron(const void* key, const void* value,
                               TraceDescriptor value_desc) {}
   virtual void VisitWeakContainer(const void* self, TraceDescriptor strong_desc,
@@ -326,45 +323,11 @@ class V8_EXPORT Visitor {
   static void HandleWeak(const LivenessBroker& info, const void* object) {
     const PointerType* weak = static_cast<const PointerType*>(object);
     auto* raw_ptr = weak->GetFromGC();
-    // Sentinel values are preserved for weak pointers.
     if (!info.IsHeapObjectAlive(raw_ptr)) {
       weak->ClearFromGC();
     }
   }
 
-  template <typename Persistent,
-            std::enable_if_t<Persistent::IsStrongPersistent::value>* = nullptr>
-  void TraceRoot(const Persistent& p, const SourceLocation& loc) {
-    using PointeeType = typename Persistent::PointeeType;
-    static_assert(sizeof(PointeeType),
-                  "Persistent's pointee type must be fully defined");
-    static_assert(internal::IsGarbageCollectedOrMixinType<PointeeType>::value,
-                  "Persistent's pointee type must be GarbageCollected or "
-                  "GarbageCollectedMixin");
-    auto* ptr = p.GetFromGC();
-    if (!ptr) {
-      return;
-    }
-    VisitRoot(ptr, TraceTrait<PointeeType>::GetTraceDescriptor(ptr), loc);
-  }
-
-  template <
-      typename WeakPersistent,
-      std::enable_if_t<!WeakPersistent::IsStrongPersistent::value>* = nullptr>
-  void TraceRoot(const WeakPersistent& p, const SourceLocation& loc) {
-    using PointeeType = typename WeakPersistent::PointeeType;
-    static_assert(sizeof(PointeeType),
-                  "Persistent's pointee type must be fully defined");
-    static_assert(internal::IsGarbageCollectedOrMixinType<PointeeType>::value,
-                  "Persistent's pointee type must be GarbageCollected or "
-                  "GarbageCollectedMixin");
-    static_assert(!internal::IsAllocatedOnCompactableSpace<PointeeType>::value,
-                  "Weak references to compactable objects are not allowed");
-    auto* ptr = p.GetFromGC();
-    VisitWeakRoot(ptr, TraceTrait<PointeeType>::GetTraceDescriptor(ptr),
-                  &HandleWeak<WeakPersistent>, &p, loc);
-  }
-
   template <typename T>
   void TraceImpl(const T* t) {
     static_assert(sizeof(T), "Pointee type must be fully defined.");
@@ -390,6 +353,70 @@ class V8_EXPORT Visitor {
   friend class internal::VisitorBase;
 };
 
+namespace internal {
+
+class V8_EXPORT RootVisitor {
+ public:
+  explicit RootVisitor(Visitor::Key) {}
+
+  virtual ~RootVisitor() = default;
+
+  template <typename AnyStrongPersistentType,
+            std::enable_if_t<
+                AnyStrongPersistentType::IsStrongPersistent::value>* = nullptr>
+  void Trace(const AnyStrongPersistentType& p) {
+    using PointeeType = typename AnyStrongPersistentType::PointeeType;
+    const void* object = Extract(p);
+    if (!object) {
+      return;
+    }
+    VisitRoot(object, TraceTrait<PointeeType>::GetTraceDescriptor(object),
+              p.Location());
+  }
+
+  template <typename AnyWeakPersistentType,
+            std::enable_if_t<
+                !AnyWeakPersistentType::IsStrongPersistent::value>* = nullptr>
+  void Trace(const AnyWeakPersistentType& p) {
+    using PointeeType = typename AnyWeakPersistentType::PointeeType;
+    static_assert(!internal::IsAllocatedOnCompactableSpace<PointeeType>::value,
+                  "Weak references to compactable objects are not allowed");
+    const void* object = Extract(p);
+    if (!object) {
+      return;
+    }
+    VisitWeakRoot(object, TraceTrait<PointeeType>::GetTraceDescriptor(object),
+                  &HandleWeak<AnyWeakPersistentType>, &p, p.Location());
+  }
+
+ protected:
+  virtual void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) {}
+  virtual void VisitWeakRoot(const void* self, TraceDescriptor, WeakCallback,
+                             const void* weak_root, const SourceLocation&) {}
+
+ private:
+  template <typename AnyPersistentType>
+  static const void* Extract(AnyPersistentType& p) {
+    using PointeeType = typename AnyPersistentType::PointeeType;
+    static_assert(sizeof(PointeeType),
+                  "Persistent's pointee type must be fully defined");
+    static_assert(internal::IsGarbageCollectedOrMixinType<PointeeType>::value,
+                  "Persistent's pointee type must be GarbageCollected or "
+                  "GarbageCollectedMixin");
+    return p.GetFromGC();
+  }
+
+  template <typename PointerType>
+  static void HandleWeak(const LivenessBroker& info, const void* object) {
+    const PointerType* weak = static_cast<const PointerType*>(object);
+    auto* raw_ptr = weak->GetFromGC();
+    if (!info.IsHeapObjectAlive(raw_ptr)) {
+      weak->ClearFromGC();
+    }
+  }
+};
+
+}  // namespace internal
 }  // namespace cppgc
 
 #endif  // INCLUDE_CPPGC_VISITOR_H_

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

@@ -16,6 +16,7 @@
 #include "src/heap/cppgc-js/cpp-heap.h"
 #include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/heap-visitor.h"
+#include "src/heap/cppgc/visitor.h"
 #include "src/heap/embedder-tracing.h"
 #include "src/heap/mark-compact.h"
 #include "src/objects/js-objects.h"
@@ -655,10 +656,6 @@ class VisiblityVisitor final : public WeakVisitor {
         &parent_scope_.ParentAsRegularState(),
         HeapObjectHeader::FromObject(desc.base_object_payload));
   }
-  void VisitRoot(const void*, cppgc::TraceDescriptor,
-                 const cppgc::SourceLocation&) final {}
-  void VisitWeakRoot(const void*, cppgc::TraceDescriptor, cppgc::WeakCallback,
-                     const void*, const cppgc::SourceLocation&) final {}
 
   // JS handling.
   void Visit(const TracedReferenceBase& ref) final {
@@ -670,6 +667,24 @@ class VisiblityVisitor final : public WeakVisitor {
   const ParentScope& parent_scope_;
 };
 
+class GraphBuildingRootVisitor final : public cppgc::internal::RootVisitorBase {
+ public:
+  GraphBuildingRootVisitor(CppGraphBuilderImpl& graph_builder,
+                           const ParentScope& parent_scope)
+      : graph_builder_(graph_builder), parent_scope_(parent_scope) {}
+
+  void VisitRoot(const void*, cppgc::TraceDescriptor desc,
+                 const cppgc::SourceLocation& loc) final {
+    graph_builder_.VisitRootForGraphBuilding(
+        parent_scope_.ParentAsRootState(),
+        HeapObjectHeader::FromObject(desc.base_object_payload), loc);
+  }
+
+ private:
+  CppGraphBuilderImpl& graph_builder_;
+  const ParentScope& parent_scope_;
+};
+
 class GraphBuildingVisitor final : public JSVisitor {
  public:
   GraphBuildingVisitor(CppGraphBuilderImpl& graph_builder,
@@ -695,14 +710,7 @@ class GraphBuildingVisitor final : public JSVisitor {
         HeapObjectHeader::FromObject(strong_desc.base_object_payload),
         edge_name_);
   }
-  void VisitRoot(const void*, cppgc::TraceDescriptor desc,
-                 const cppgc::SourceLocation& loc) final {
-    graph_builder_.VisitRootForGraphBuilding(
-        parent_scope_.ParentAsRootState(),
-        HeapObjectHeader::FromObject(desc.base_object_payload), loc);
-  }
-  void VisitWeakRoot(const void*, cppgc::TraceDescriptor, cppgc::WeakCallback,
-                     const void*, const cppgc::SourceLocation&) final {}
+
   // JS handling.
   void Visit(const TracedReferenceBase& ref) final {
     graph_builder_.AddEdge(parent_scope_.ParentAsRegularState(), ref,
@@ -888,15 +896,16 @@ void CppGraphBuilderImpl::Run() {
   // Add roots.
   {
     ParentScope parent_scope(states_.CreateRootState(AddRootNode("C++ roots")));
-    GraphBuildingVisitor object_visitor(*this, parent_scope);
-    cpp_heap_.GetStrongPersistentRegion().Trace(&object_visitor);
+    GraphBuildingRootVisitor root_object_visitor(*this, parent_scope);
+    cpp_heap_.GetStrongPersistentRegion().Iterate(root_object_visitor);
   }
   {
     ParentScope parent_scope(
         states_.CreateRootState(AddRootNode("C++ cross-thread roots")));
-    GraphBuildingVisitor object_visitor(*this, parent_scope);
+    GraphBuildingRootVisitor root_object_visitor(*this, parent_scope);
     cppgc::internal::PersistentRegionLock guard;
-    cpp_heap_.GetStrongCrossThreadPersistentRegion().Trace(&object_visitor);
+    cpp_heap_.GetStrongCrossThreadPersistentRegion().Iterate(
+        root_object_visitor);
   }
 }
 

src/heap/cppgc-js/unified-heap-marking-visitor.cc

@@ -65,21 +65,6 @@ MutatorUnifiedHeapMarkingVisitor::MutatorUnifiedHeapMarkingVisitor(
     : UnifiedHeapMarkingVisitorBase(heap, marking_state,
                                     unified_heap_marking_state) {}
 
-void MutatorUnifiedHeapMarkingVisitor::VisitRoot(const void* object,
-                                                 TraceDescriptor desc,
-                                                 const SourceLocation&) {
-  this->Visit(object, desc);
-}
-
-void MutatorUnifiedHeapMarkingVisitor::VisitWeakRoot(const void* object,
-                                                     TraceDescriptor desc,
-                                                     WeakCallback weak_callback,
-                                                     const void* weak_root,
-                                                     const SourceLocation&) {
-  static_cast<MutatorMarkingState&>(marking_state_)
-      .InvokeWeakRootsCallbackIfNeeded(object, desc, weak_callback, weak_root);
-}
-
 ConcurrentUnifiedHeapMarkingVisitor::ConcurrentUnifiedHeapMarkingVisitor(
     HeapBase& heap, Heap* v8_heap,
     cppgc::internal::ConcurrentMarkingState& marking_state)

src/heap/cppgc-js/unified-heap-marking-visitor.h

@@ -65,11 +65,6 @@ class V8_EXPORT_PRIVATE MutatorUnifiedHeapMarkingVisitor
   MutatorUnifiedHeapMarkingVisitor(HeapBase&, MutatorMarkingState&,
                                    UnifiedHeapMarkingState&);
   ~MutatorUnifiedHeapMarkingVisitor() override = default;
-
- protected:
-  void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) final;
-  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback, const void*,
-                     const SourceLocation&) final;
 };
 
 class V8_EXPORT_PRIVATE MutatorMinorGCMarkingVisitor final
@@ -92,14 +87,6 @@ class V8_EXPORT_PRIVATE ConcurrentUnifiedHeapMarkingVisitor
   ~ConcurrentUnifiedHeapMarkingVisitor() override;
 
  protected:
-  void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) final {
-    UNREACHABLE();
-  }
-  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback, const void*,
-                     const SourceLocation&) final {
-    UNREACHABLE();
-  }
-
   bool DeferTraceToMutatorThreadIfConcurrent(const void*, cppgc::TraceCallback,
                                              size_t) final;
 

src/heap/cppgc/marker.cc

@@ -372,11 +372,12 @@ void MarkerBase::ProcessWeakness() {
             broker));
   }
 
-  heap().GetWeakPersistentRegion().Trace(&visitor());
+  RootMarkingVisitor root_marking_visitor(mutator_marking_state_);
+  heap().GetWeakPersistentRegion().Iterate(root_marking_visitor);
   // Processing cross-thread handles requires taking the process lock.
   g_process_mutex.Get().AssertHeld();
   CHECK(visited_cross_thread_persistents_in_atomic_pause_);
-  heap().GetWeakCrossThreadPersistentRegion().Trace(&visitor());
+  heap().GetWeakCrossThreadPersistentRegion().Iterate(root_marking_visitor);
 
   // Call weak callbacks on objects that may now be pointing to dead objects.
 #if defined(CPPGC_YOUNG_GENERATION)
@@ -436,7 +437,8 @@ void MarkerBase::VisitRoots(MarkingConfig::StackState stack_state) {
     {
       StatsCollector::DisabledScope inner_stats_scope(
           heap().stats_collector(), StatsCollector::kMarkVisitPersistents);
-      heap().GetStrongPersistentRegion().Trace(&visitor());
+      RootMarkingVisitor root_marking_visitor(mutator_marking_state_);
+      heap().GetStrongPersistentRegion().Iterate(root_marking_visitor);
     }
   }
 
@@ -467,7 +469,8 @@ bool MarkerBase::VisitCrossThreadPersistentsIfNeeded() {
   // converted into a CrossThreadPersistent which requires that the handle
   // is either cleared or the object is retained.
   g_process_mutex.Pointer()->Lock();
-  heap().GetStrongCrossThreadPersistentRegion().Trace(&visitor());
+  RootMarkingVisitor root_marking_visitor(mutator_marking_state_);
+  heap().GetStrongCrossThreadPersistentRegion().Iterate(root_marking_visitor);
   visited_cross_thread_persistents_in_atomic_pause_ = true;
   return (heap().GetStrongCrossThreadPersistentRegion().NodesInUse() > 0);
 }

src/heap/cppgc/marking-visitor.cc

@@ -77,18 +77,20 @@ MutatorMarkingVisitor::MutatorMarkingVisitor(HeapBase& heap,
                                              MutatorMarkingState& marking_state)
     : MarkingVisitorBase(heap, marking_state) {}
 
-void MutatorMarkingVisitor::VisitRoot(const void* object, TraceDescriptor desc,
-                                      const SourceLocation&) {
-  Visit(object, desc);
+RootMarkingVisitor::RootMarkingVisitor(MutatorMarkingState& marking_state)
+    : mutator_marking_state_(marking_state) {}
+
+void RootMarkingVisitor::VisitRoot(const void* object, TraceDescriptor desc,
+                                   const SourceLocation&) {
+  mutator_marking_state_.MarkAndPush(object, desc);
 }
 
-void MutatorMarkingVisitor::VisitWeakRoot(const void* object,
-                                          TraceDescriptor desc,
-                                          WeakCallback weak_callback,
-                                          const void* weak_root,
-                                          const SourceLocation&) {
-  static_cast<MutatorMarkingState&>(marking_state_)
-      .InvokeWeakRootsCallbackIfNeeded(object, desc, weak_callback, weak_root);
+void RootMarkingVisitor::VisitWeakRoot(const void* object, TraceDescriptor desc,
+                                       WeakCallback weak_callback,
+                                       const void* weak_root,
+                                       const SourceLocation&) {
+  mutator_marking_state_.InvokeWeakRootsCallbackIfNeeded(
+      object, desc, weak_callback, weak_root);
 }
 
 ConcurrentMarkingVisitor::ConcurrentMarkingVisitor(

src/heap/cppgc/marking-visitor.h

@@ -42,11 +42,6 @@ class V8_EXPORT_PRIVATE MutatorMarkingVisitor : public MarkingVisitorBase {
  public:
   MutatorMarkingVisitor(HeapBase&, MutatorMarkingState&);
   ~MutatorMarkingVisitor() override = default;
-
- protected:
-  void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) final;
-  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback, const void*,
-                     const SourceLocation&) final;
 };
 
 class V8_EXPORT_PRIVATE ConcurrentMarkingVisitor final
@@ -56,18 +51,23 @@ class V8_EXPORT_PRIVATE ConcurrentMarkingVisitor final
   ~ConcurrentMarkingVisitor() override = default;
 
  protected:
-  void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) final {
-    UNREACHABLE();
-  }
-  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback, const void*,
-                     const SourceLocation&) final {
-    UNREACHABLE();
-  }
-
   bool DeferTraceToMutatorThreadIfConcurrent(const void*, TraceCallback,
                                              size_t) final;
 };
 
+class V8_EXPORT_PRIVATE RootMarkingVisitor : public RootVisitorBase {
+ public:
+  explicit RootMarkingVisitor(MutatorMarkingState&);
+  ~RootMarkingVisitor() override = default;
+
+ protected:
+  void VisitRoot(const void*, TraceDescriptor, const SourceLocation&) final;
+  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback, const void*,
+                     const SourceLocation&) final;
+
+  MutatorMarkingState& mutator_marking_state_;
+};
+
 class ConservativeMarkingVisitor : public ConservativeTracingVisitor,
                                    public heap::base::StackVisitor {
  public:

src/heap/cppgc/persistent-node.cc

@@ -76,20 +76,20 @@ void PersistentRegionBase::RefillFreeList() {
 }
 
 PersistentNode* PersistentRegionBase::RefillFreeListAndAllocateNode(
-    void* owner, TraceCallback trace) {
+    void* owner, TraceRootCallback trace) {
   RefillFreeList();
   auto* node = TryAllocateNodeFromFreeList(owner, trace);
   CPPGC_DCHECK(node);
   return node;
 }
 
-void PersistentRegionBase::Trace(Visitor* visitor) {
+void PersistentRegionBase::Iterate(RootVisitor& root_visitor) {
   free_list_head_ = nullptr;
   for (auto& slots : nodes_) {
     bool is_empty = true;
     for (auto& node : *slots) {
       if (node.IsUsed()) {
-        node.Trace(visitor);
+        node.Trace(root_visitor);
         is_empty = false;
       } else {
         node.InitializeAsFreeNode(free_list_head_);
@@ -145,9 +145,9 @@ CrossThreadPersistentRegion::~CrossThreadPersistentRegion() {
   // PersistentRegionBase destructor will be a noop.
 }
 
-void CrossThreadPersistentRegion::Trace(Visitor* visitor) {
+void CrossThreadPersistentRegion::Iterate(RootVisitor& root_visitor) {
   PersistentRegionLock::AssertLocked();
-  PersistentRegionBase::Trace(visitor);
+  PersistentRegionBase::Iterate(root_visitor);
 }
 
 size_t CrossThreadPersistentRegion::NodesInUse() const {

src/heap/cppgc/visitor.h

@@ -15,7 +15,7 @@ class HeapBase;
 class HeapObjectHeader;
 class PageBackend;
 
-class VisitorFactory {
+class VisitorFactory final {
  public:
   static constexpr Visitor::Key CreateKey() { return {}; }
 };
@@ -34,11 +34,15 @@ class VisitorBase : public cppgc::Visitor {
 
   VisitorBase(const VisitorBase&) = delete;
   VisitorBase& operator=(const VisitorBase&) = delete;
+};
 
-  template <typename Persistent>
-  void TraceRootForTesting(const Persistent& p, const SourceLocation& loc) {
-    TraceRoot(p, loc);
-  }
+class RootVisitorBase : public RootVisitor {
+ public:
+  RootVisitorBase() : RootVisitor(VisitorFactory::CreateKey()) {}
+  ~RootVisitorBase() override = default;
+
+  RootVisitorBase(const RootVisitorBase&) = delete;
+  RootVisitorBase& operator=(const RootVisitorBase&) = delete;
 };
 
 // Regular visitor that additionally allows for conservative tracing.

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

@@ -56,6 +56,15 @@ class TestMarkingVisitor : public MutatorMarkingVisitor {
   BasicMarkingState& marking_state() { return marking_state_; }
 };
 
+class TestRootMarkingVisitor : public RootMarkingVisitor {
+ public:
+  explicit TestRootMarkingVisitor(Marker* marker)
+      : RootMarkingVisitor(marker->MutatorMarkingStateForTesting()) {}
+  ~TestRootMarkingVisitor() { mutator_marking_state_.Publish(); }
+
+  MutatorMarkingState& marking_state() { return mutator_marking_state_; }
+};
+
 }  // namespace
 
 TEST_F(MarkingVisitorTest, MarkedBytesAreInitiallyZero) {
@@ -96,11 +105,11 @@ TEST_F(MarkingVisitorTest, MarkPersistent) {
   Persistent<GCed> object(MakeGarbageCollected<GCed>(GetAllocationHandle()));
   HeapObjectHeader& header = HeapObjectHeader::FromObject(object);
 
-  TestMarkingVisitor visitor(GetMarker());
+  TestRootMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
-  visitor.TraceRootForTesting(object, SourceLocation::Current());
+  visitor.Trace(object);
 
   EXPECT_TRUE(header.IsMarked());
 }
@@ -111,11 +120,11 @@ TEST_F(MarkingVisitorTest, MarkPersistentMixin) {
   Persistent<Mixin> mixin(object);
   HeapObjectHeader& header = HeapObjectHeader::FromObject(object);
 
-  TestMarkingVisitor visitor(GetMarker());
+  TestRootMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
-  visitor.TraceRootForTesting(mixin, SourceLocation::Current());
+  visitor.Trace(mixin);
 
   EXPECT_TRUE(header.IsMarked());
 }
@@ -155,11 +164,11 @@ TEST_F(MarkingVisitorTest, DontMarkWeakPersistent) {
       MakeGarbageCollected<GCed>(GetAllocationHandle()));
   HeapObjectHeader& header = HeapObjectHeader::FromObject(object);
 
-  TestMarkingVisitor visitor(GetMarker());
+  TestRootMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
-  visitor.TraceRootForTesting(object, SourceLocation::Current());
+  visitor.Trace(object);
 
   EXPECT_FALSE(header.IsMarked());
 }
@@ -170,11 +179,11 @@ TEST_F(MarkingVisitorTest, DontMarkWeakPersistentMixin) {
   WeakPersistent<Mixin> mixin(object);
   HeapObjectHeader& header = HeapObjectHeader::FromObject(object);
 
-  TestMarkingVisitor visitor(GetMarker());
+  TestRootMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
-  visitor.TraceRootForTesting(mixin, SourceLocation::Current());
+  visitor.Trace(mixin);
 
   EXPECT_FALSE(header.IsMarked());
 }
@@ -275,13 +284,13 @@ TEST_F(MarkingVisitorTest, DontMarkWeakMemberMixinInConstruction) {
 }
 
 TEST_F(MarkingVisitorTest, MarkPersistentInConstruction) {
-  TestMarkingVisitor visitor(GetMarker());
+  TestRootMarkingVisitor visitor(GetMarker());
   GCedWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithInConstructionCallback>(
           GetAllocationHandle(),
           [&visitor](GCedWithInConstructionCallback* obj) {
             Persistent<GCedWithInConstructionCallback> object(obj);
-            visitor.TraceRootForTesting(object, SourceLocation::Current());
+            visitor.Trace(object);
           });
   HeapObjectHeader& header = HeapObjectHeader::FromObject(gced);
   EXPECT_TRUE(visitor.marking_state().not_fully_constructed_worklist().Contains(
@@ -290,13 +299,13 @@ TEST_F(MarkingVisitorTest, MarkPersistentInConstruction) {
 }
 
 TEST_F(MarkingVisitorTest, MarkPersistentMixinInConstruction) {
-  TestMarkingVisitor visitor(GetMarker());
+  TestRootMarkingVisitor visitor(GetMarker());
   GCedWithMixinWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
           GetAllocationHandle(),
           [&visitor](MixinWithInConstructionCallback* obj) {
             Persistent<MixinWithInConstructionCallback> mixin(obj);
-            visitor.TraceRootForTesting(mixin, SourceLocation::Current());
+            visitor.Trace(mixin);
           });
   HeapObjectHeader& header = HeapObjectHeader::FromObject(gced);
   EXPECT_TRUE(visitor.marking_state().not_fully_constructed_worklist().Contains(

test/unittests/heap/cppgc/persistent-family-unittest.cc

@@ -84,9 +84,9 @@ using LocalizedCrossThreadPersistent = internal::BasicCrossThreadPersistent<
     T, internal::StrongCrossThreadPersistentPolicy,
     internal::KeepLocationPolicy, internal::DisabledCheckingPolicy>;
 
-class RootVisitor final : public VisitorBase {
+class TestRootVisitor final : public RootVisitorBase {
  public:
-  RootVisitor() = default;
+  TestRootVisitor() = default;
 
   const auto& WeakCallbacks() const { return weak_callbacks_; }
 
@@ -101,7 +101,7 @@ class RootVisitor final : public VisitorBase {
  protected:
   void VisitRoot(const void* t, TraceDescriptor desc,
                  const SourceLocation&) final {
-    desc.callback(this, desc.base_object_payload);
+    desc.callback(nullptr, desc.base_object_payload);
   }
   void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback callback,
                      const void* object, const SourceLocation&) final {
@@ -746,8 +746,8 @@ TEST_F(PersistentTest, TraceStrong) {
   }
   {
     GCed::trace_call_count = 0;
-    RootVisitor v;
-    GetRegion<Persistent>(heap).Trace(&v);
+    TestRootVisitor v;
+    GetRegion<Persistent>(heap).Iterate(v);
     EXPECT_EQ(kItems, GCed::trace_call_count);
     EXPECT_EQ(kItems, GetRegion<Persistent>(heap).NodesInUse());
   }
@@ -757,16 +757,16 @@ TEST_F(PersistentTest, TraceStrong) {
     vec[kItems / 2].Clear();
     vec[kItems / 4].Clear();
     vec[kItems - 1].Clear();
-    RootVisitor v;
-    GetRegion<Persistent>(heap).Trace(&v);
+    TestRootVisitor v;
+    GetRegion<Persistent>(heap).Iterate(v);
     EXPECT_EQ(kItems - 4, GCed::trace_call_count);
     EXPECT_EQ(kItems - 4, GetRegion<Persistent>(heap).NodesInUse());
   }
   {
     GCed::trace_call_count = 0;
     vec.clear();
-    RootVisitor v;
-    GetRegion<Persistent>(heap).Trace(&v);
+    TestRootVisitor v;
+    GetRegion<Persistent>(heap).Iterate(v);
     EXPECT_EQ(0u, GCed::trace_call_count);
     EXPECT_EQ(0u, GetRegion<Persistent>(heap).NodesInUse());
   }
@@ -780,8 +780,8 @@ TEST_F(PersistentTest, TraceWeak) {
     p = MakeGarbageCollected<GCed>(GetAllocationHandle());
   }
   GCed::trace_call_count = 0;
-  RootVisitor v;
-  GetRegion<WeakPersistent>(heap).Trace(&v);
+  TestRootVisitor v;
+  GetRegion<WeakPersistent>(heap).Iterate(v);
   const auto& callbacks = v.WeakCallbacks();
   EXPECT_EQ(kItems, callbacks.size());
   EXPECT_EQ(kItems, GetRegion<WeakPersistent>(heap).NodesInUse());
@@ -927,7 +927,7 @@ TEST_F(PersistentTest, LocalizedPersistent) {
 
 namespace {
 
-class ExpectingLocationVisitor final : public VisitorBase {
+class ExpectingLocationVisitor final : public RootVisitorBase {
  public:
   explicit ExpectingLocationVisitor(const SourceLocation& expected_location)
       : expected_loc_(expected_location) {}
@@ -960,7 +960,7 @@ TEST_F(PersistentTest, PersistentTraceLocation) {
 #endif  // !CCPPGC_SUPPORTS_SOURCE_LOCATION
     LocalizedPersistent<GCed> p = gced;
     ExpectingLocationVisitor visitor(expected_loc);
-    visitor.TraceRootForTesting(p, p.Location());
+    visitor.Trace(p);
   }
 }