cppgc: Refactor visitation 2/3

Split off ConservativeMarkingVisitor from MarkingVisitor.

After this change, MarkingVisitor and ConservativeMarkingVisitor are
types that are instantiated with Marking and merely forward to
MarkingState accrodingly. The two marking-related visitors can be
passed along as interface types cppgc::Visitor and
ConservativeTracingVisitor, respectively.

Change-Id: Iad103dc3053c61d1a104a8802edd420d21cdf935
Bug: chromium:1056170
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2270538
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: 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@{#68661}

include/cppgc/visitor.h

@@ -19,6 +19,7 @@ namespace internal {
 template <typename T, typename WeaknessPolicy, typename LocationPolicy,
           typename CheckingPolicy>
 class BasicPersistent;
+class ConservativeTracingVisitor;
 class VisitorBase;
 }  // namespace internal
 
@@ -43,6 +44,8 @@ using WeakCallback = void (*)(const LivenessBroker&, const void*);
  */
 class Visitor {
  public:
+  virtual ~Visitor() = default;
+
   /**
    * Trace method for Member.
    *
@@ -194,6 +197,7 @@ class Visitor {
   template <typename T, typename WeaknessPolicy, typename LocationPolicy,
             typename CheckingPolicy>
   friend class internal::BasicPersistent;
+  friend class internal::ConservativeTracingVisitor;
 };
 
 }  // namespace cppgc

src/heap/cppgc/marker.cc

@@ -106,7 +106,10 @@ Marker::Marker(HeapBase& heap)
       mutator_marking_state_(std::make_unique<MarkingState>(
           heap, &marking_worklist_, &not_fully_constructed_worklist_,
           &weak_callback_worklist_, Marker::kMutatorThreadId)),
-      marking_visitor_(CreateMutatorThreadMarkingVisitor()) {}
+      marking_visitor_(CreateMutatorThreadMarkingVisitor()),
+      conservative_marking_visitor_(
+          std::make_unique<ConservativeMarkingVisitor>(
+              heap, *mutator_marking_state_.get(), *marking_visitor_.get())) {}
 
 Marker::~Marker() {
   // The fixed point iteration may have found not-fully-constructed objects.
@@ -184,7 +187,7 @@ void Marker::VisitRoots() {
 
   heap().GetStrongPersistentRegion().Trace(marking_visitor_.get());
   if (config_.stack_state != MarkingConfig::StackState::kNoHeapPointers) {
-    heap().stack()->IteratePointers(marking_visitor_.get());
+    heap().stack()->IteratePointers(conservative_marking_visitor_.get());
   }
   if (config_.collection_type == MarkingConfig::CollectionType::kMinor) {
     VisitRememberedSlots(heap(), marking_visitor_.get());
@@ -258,7 +261,7 @@ void Marker::MarkNotFullyConstructedObjects() {
   NotFullyConstructedWorklist::View view(&not_fully_constructed_worklist_,
                                          kMutatorThreadId);
   while (view.Pop(&item)) {
-    marking_visitor_->TraceConservativelyIfNeeded(item);
+    conservative_marking_visitor_->TraceConservativelyIfNeeded(item);
   }
 }
 

src/heap/cppgc/marker.h

@@ -11,6 +11,7 @@
 #include "include/cppgc/trace-trait.h"
 #include "include/cppgc/visitor.h"
 #include "src/base/platform/time.h"
+#include "src/heap/cppgc/marking-visitor.h"
 #include "src/heap/cppgc/worklist.h"
 
 namespace cppgc {
@@ -138,6 +139,7 @@ class V8_EXPORT_PRIVATE Marker {
 
   std::unique_ptr<MarkingState> mutator_marking_state_;
   std::unique_ptr<MutatorThreadMarkingVisitor> marking_visitor_;
+  std::unique_ptr<ConservativeMarkingVisitor> conservative_marking_visitor_;
 
   MarkingWorklist marking_worklist_;
   NotFullyConstructedWorklist not_fully_constructed_worklist_;

src/heap/cppgc/marking-visitor.cc

@@ -15,8 +15,7 @@ namespace cppgc {
 namespace internal {
 
 MarkingVisitor::MarkingVisitor(HeapBase& heap, MarkingState& marking_state)
-    : ConservativeTracingVisitor(heap, *heap.page_backend()),
+    : marking_state_(marking_state) {}
-      marking_state_(marking_state) {}
 
 void MarkingVisitor::Visit(const void* object, TraceDescriptor desc) {
   marking_state_.MarkAndPush(object, desc);
@@ -40,20 +39,25 @@ void MarkingVisitor::VisitWeakRoot(const void* object, TraceDescriptor desc,
                                                  weak_root);
 }
 
-void MarkingVisitor::VisitPointer(const void* address) {
+void MarkingVisitor::RegisterWeakCallback(WeakCallback callback,
-  TraceConservativelyIfNeeded(address);
+                                          const void* object) {
+  marking_state_.RegisterWeakCallback(callback, object);
 }
 
-void MarkingVisitor::VisitConservatively(HeapObjectHeader& header,
+ConservativeMarkingVisitor::ConservativeMarkingVisitor(
-                                         TraceConservativelyCallback callback) {
+    HeapBase& heap, MarkingState& marking_state, cppgc::Visitor& visitor)
+    : ConservativeTracingVisitor(heap, *heap.page_backend(), visitor),
+      marking_state_(marking_state) {}
+
+void ConservativeMarkingVisitor::VisitConservatively(
+    HeapObjectHeader& header, TraceConservativelyCallback callback) {
   marking_state_.MarkNoPush(header);
   callback(this, header);
   marking_state_.AccountMarkedBytes(header);
 }
 
-void MarkingVisitor::RegisterWeakCallback(WeakCallback callback,
+void ConservativeMarkingVisitor::VisitPointer(const void* address) {
-                                          const void* object) {
+  TraceConservativelyIfNeeded(address);
-  marking_state_.RegisterWeakCallback(callback, object);
 }
 
 MutatorThreadMarkingVisitor::MutatorThreadMarkingVisitor(Marker* marker)

src/heap/cppgc/marking-visitor.h

@@ -18,14 +18,10 @@ class HeapObjectHeader;
 class Marker;
 class MarkingState;
 
-class MarkingVisitor : public ConservativeTracingVisitor,
+class MarkingVisitor : public VisitorBase {
-                       public heap::base::StackVisitor {
  public:
   MarkingVisitor(HeapBase&, MarkingState&);
-  virtual ~MarkingVisitor() = default;
+  ~MarkingVisitor() override = default;
-
-  MarkingVisitor(const MarkingVisitor&) = delete;
-  MarkingVisitor& operator=(const MarkingVisitor&) = delete;
 
  private:
   void Visit(const void*, TraceDescriptor) final;
@@ -33,11 +29,20 @@ class MarkingVisitor : public ConservativeTracingVisitor,
   void VisitRoot(const void*, TraceDescriptor) final;
   void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback,
                      const void*) final;
-  void VisitConservatively(HeapObjectHeader&,
-                           TraceConservativelyCallback) final;
   void RegisterWeakCallback(WeakCallback, const void*) final;
 
-  // StackMarker interface.
+  MarkingState& marking_state_;
+};
+
+class ConservativeMarkingVisitor : public ConservativeTracingVisitor,
+                                   public heap::base::StackVisitor {
+ public:
+  ConservativeMarkingVisitor(HeapBase&, MarkingState&, cppgc::Visitor&);
+  ~ConservativeMarkingVisitor() override = default;
+
+ private:
+  void VisitConservatively(HeapObjectHeader&,
+                           TraceConservativelyCallback) final;
   void VisitPointer(const void*) override;
 
   MarkingState& marking_state_;

src/heap/cppgc/visitor.cc

@@ -22,12 +22,12 @@ void Visitor::CheckObjectNotInConstruction(const void* address) {
 namespace internal {
 
 ConservativeTracingVisitor::ConservativeTracingVisitor(
-    HeapBase& heap, PageBackend& page_backend)
+    HeapBase& heap, PageBackend& page_backend, cppgc::Visitor& visitor)
-    : heap_(heap), page_backend_(page_backend) {}
+    : heap_(heap), page_backend_(page_backend), visitor_(visitor) {}
 
 namespace {
 
-void TraceConservatively(ConservativeTracingVisitor* visitor,
+void TraceConservatively(ConservativeTracingVisitor* conservative_visitor,
                          const HeapObjectHeader& header) {
   Address* payload = reinterpret_cast<Address*>(header.Payload());
   const size_t payload_size = header.GetSize();
@@ -40,7 +40,7 @@ void TraceConservatively(ConservativeTracingVisitor* visitor,
     MSAN_UNPOISON(&maybe_ptr, sizeof(maybe_ptr));
 #endif
     if (maybe_ptr) {
-      visitor->TraceConservativelyIfNeeded(maybe_ptr);
+      conservative_visitor->TraceConservativelyIfNeeded(maybe_ptr);
     }
   }
 }
@@ -64,9 +64,10 @@ void ConservativeTracingVisitor::TraceConservativelyIfNeeded(
   if (!header) return;
 
   if (!header->IsInConstruction<HeapObjectHeader::AccessMode::kNonAtomic>()) {
-    Visit(header->Payload(),
+    visitor_.Visit(
-          {header->Payload(),
+        header->Payload(),
-           GlobalGCInfoTable::GCInfoFromIndex(header->GetGCInfoIndex()).trace});
+        {header->Payload(),
+         GlobalGCInfoTable::GCInfoFromIndex(header->GetGCInfoIndex()).trace});
   } else {
     VisitConservatively(*header, TraceConservatively);
   }

src/heap/cppgc/visitor.h

@@ -21,6 +21,10 @@ class PageBackend;
 class VisitorBase : public cppgc::Visitor {
  public:
   VisitorBase() = default;
+  ~VisitorBase() override = default;
+
+  VisitorBase(const VisitorBase&) = delete;
+  VisitorBase& operator=(const VisitorBase&) = delete;
 
   template <typename T>
   void TraceRootForTesting(const Persistent<T>& p, const SourceLocation& loc) {
@@ -35,9 +39,14 @@ class VisitorBase : public cppgc::Visitor {
 };
 
 // Regular visitor that additionally allows for conservative tracing.
-class ConservativeTracingVisitor : public VisitorBase {
+class ConservativeTracingVisitor {
  public:
-  ConservativeTracingVisitor(HeapBase&, PageBackend&);
+  ConservativeTracingVisitor(HeapBase&, PageBackend&, cppgc::Visitor&);
+  virtual ~ConservativeTracingVisitor() = default;
+
+  ConservativeTracingVisitor(const ConservativeTracingVisitor&) = delete;
+  ConservativeTracingVisitor& operator=(const ConservativeTracingVisitor&) =
+      delete;
 
   void TraceConservativelyIfNeeded(const void*);
 
@@ -49,6 +58,7 @@ class ConservativeTracingVisitor : public VisitorBase {
 
   HeapBase& heap_;
   PageBackend& page_backend_;
+  cppgc::Visitor& visitor_;
 };
 
 }  // namespace internal