Reland "cppgc: Initial marking loop"

This reverts commit dc1af6a219.

Reason for revert: Diff in patchset 2

Original change's description:
> Revert "cppgc: Initial marking loop"
> 
> This reverts commit fb9a19fe0d.
> 
> Reason for revert: https://ci.chromium.org/p/v8/builders/ci/V8%20Linux64%20UBSan/11028
> 
> Original change's description:
> > cppgc: Initial marking loop
> > 
> > This CL introduces:
> > - Worklist
> > - MarkingHandler to manage gc marking phase
> > - Integration into CollectGarbage for atomic pause GC
> > - MarkingVisitor for main thread marking
> > 
> > Still missing from this CL:
> > - Proper handling for stack scanning
> > - Handling of previously not fully constructed objects
> > 
> > Bug: chromium:1056170
> > Change-Id: I70ac8534dfb898777cf3a06e3119cac8072174fd
> > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2170526
> > Commit-Queue: Omer Katz <omerkatz@chromium.org>
> > Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> > Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#67642}
> 
> TBR=ulan@chromium.org,mlippautz@chromium.org,bikineev@chromium.org,omerkatz@chromium.org
> 
> Change-Id: I666481f44119771be685bf2555aa0dd5eda83a01
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: chromium:1056170
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2187502
> Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
> Commit-Queue: Nico Hartmann <nicohartmann@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#67643}

TBR=ulan@chromium.org,mlippautz@chromium.org,bikineev@chromium.org,omerkatz@chromium.org,nicohartmann@chromium.org

# Not skipping CQ checks because this is a reland.

Bug: chromium:1056170
Change-Id: I54e963e2aeaaf16069bdcdb019c0ac65e28ef6e2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2187733
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67654}

BUILD.gn

@@ -4061,6 +4061,10 @@ v8_source_set("cppgc_base") {
     "src/heap/cppgc/heap.h",
     "src/heap/cppgc/liveness-broker.cc",
     "src/heap/cppgc/logging.cc",
+    "src/heap/cppgc/marker.cc",
+    "src/heap/cppgc/marker.h",
+    "src/heap/cppgc/marking-visitor.cc",
+    "src/heap/cppgc/marking-visitor.h",
     "src/heap/cppgc/object-allocator-inl.h",
     "src/heap/cppgc/object-allocator.cc",
     "src/heap/cppgc/object-allocator.h",

include/cppgc/heap.h

@@ -45,7 +45,7 @@ class V8_EXPORT Heap {
     /**
      * The embedder does not know anything about it's stack.
      */
-    kUnkown,
+    kUnknown,
     /**
      * The stack is empty, i.e., it does not contain any raw pointers
      * to garbage-collected objects.
@@ -71,8 +71,9 @@ class V8_EXPORT Heap {
    *   collection.
    * \param stack_state The embedder stack state, see StackState.
    */
-  void ForceGarbageCollectionSlow(const char* source, const char* reason,
+  void ForceGarbageCollectionSlow(
-                                  StackState stack_state = StackState::kUnkown);
+      const char* source, const char* reason,
+      StackState stack_state = StackState::kUnknown);
 
  private:
   Heap() = default;

include/cppgc/visitor.h

@@ -64,20 +64,21 @@ class Visitor {
     if (!p.Get()) {
       return;
     }
-    VisitRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()),
+    VisitRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()));
-              loc);
   }
 
-  template <typename Persistent,
+  template <
-            std::enable_if_t<!Persistent::IsStrongPersistent::value>* = nullptr>
+      typename WeakPersistent,
-  void TraceRoot(const Persistent& p, const SourceLocation& loc) {
+      std::enable_if_t<!WeakPersistent::IsStrongPersistent::value>* = nullptr>
-    using PointeeType = typename Persistent::PointeeType;
+  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::IsGarbageCollectedType<PointeeType>::value,
                   "Persisent's pointee type must be GarabgeCollected or "
                   "GarbageCollectedMixin");
-    VisitWeakRoot(&p, &HandleWeak<Persistent>);
+    VisitWeakRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()),
+                  &HandleWeak<WeakPersistent>, &p);
   }
 
   template <typename T, void (T::*method)(const LivenessBroker&)>
@@ -91,9 +92,9 @@ class 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,
+  virtual void VisitRoot(const void*, TraceDescriptor) {}
-                         const SourceLocation& loc) {}
+  virtual void VisitWeakRoot(const void* self, TraceDescriptor, WeakCallback,
-  virtual void VisitWeakRoot(const void*, WeakCallback) {}
+                             const void* weak_root) {}
 
  private:
   template <typename T, void (T::*method)(const LivenessBroker&)>

src/heap/cppgc/heap-page.cc

@@ -55,6 +55,7 @@ BasePage::BasePage(Heap* heap, BaseSpace* space, PageType type)
 NormalPage* NormalPage::Create(NormalPageSpace* space) {
   DCHECK(space);
   Heap* heap = space->raw_heap()->heap();
+  DCHECK(heap);
   void* memory = heap->page_backend()->AllocateNormalPageMemory(space->index());
   auto* normal_page = new (memory) NormalPage(heap, space);
   space->AddPage(normal_page);

src/heap/cppgc/heap.cc

@@ -29,10 +29,6 @@ namespace internal {
 
 namespace {
 
-constexpr bool NeedsConservativeStackScan(Heap::GCConfig config) {
-  return config.stack_state != Heap::GCConfig::StackState::kEmpty;
-}
-
 class ObjectSizeCounter : public HeapVisitor<ObjectSizeCounter> {
   friend class HeapVisitor<ObjectSizeCounter>;
 
@@ -69,26 +65,6 @@ cppgc::LivenessBroker LivenessBrokerFactory::Create() {
   return cppgc::LivenessBroker();
 }
 
-// TODO(chromium:1056170): Replace with fast stack scanning once
-// object are allocated actual arenas/spaces.
-class StackMarker final : public StackVisitor {
- public:
-  explicit StackMarker(const std::vector<HeapObjectHeader*>& objects)
-      : objects_(objects) {}
-
-  void VisitPointer(const void* address) final {
-    for (auto* header : objects_) {
-      if (address >= header->Payload() &&
-          address < (header + header->GetSize())) {
-        header->TryMarkAtomic();
-      }
-    }
-  }
-
- private:
-  const std::vector<HeapObjectHeader*>& objects_;
-};
-
 Heap::Heap()
     : raw_heap_(this),
       page_backend_(std::make_unique<PageBackend>(&system_allocator_)),
@@ -110,16 +86,17 @@ void Heap::CollectGarbage(GCConfig config) {
 
   // TODO(chromium:1056170): Replace with proper mark-sweep algorithm.
   // "Marking".
-  if (NeedsConservativeStackScan(config)) {
+  marker_ = std::make_unique<Marker>(this);
-    StackMarker marker(objects_);
+  marker_->StartMarking(Marker::MarkingConfig(config.stack_state));
-    stack_->IteratePointers(&marker);
+  marker_->FinishMarking();
-  }
   // "Sweeping and finalization".
   {
     // Pre finalizers are forbidden from allocating objects
     NoAllocationScope no_allocation_scope_(this);
+    marker_->ProcessWeakness();
     prefinalizer_handler_->InvokePreFinalizers();
   }
+  marker_.reset();
   {
     NoGCScope no_gc(this);
     sweeper_.Start(Sweeper::Config::kAtomic);

src/heap/cppgc/heap.h

@@ -14,6 +14,7 @@
 #include "include/cppgc/liveness-broker.h"
 #include "src/base/page-allocator.h"
 #include "src/heap/cppgc/heap-object-header.h"
+#include "src/heap/cppgc/marker.h"
 #include "src/heap/cppgc/object-allocator.h"
 #include "src/heap/cppgc/page-memory.h"
 #include "src/heap/cppgc/prefinalizer-handler.h"
@@ -66,9 +67,9 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
   struct GCConfig {
     using StackState = Heap::StackState;
 
-    static GCConfig Default() { return {StackState::kUnkown}; }
+    static GCConfig Default() { return {StackState::kUnknown}; }
 
-    StackState stack_state = StackState::kUnkown;
+    StackState stack_state = StackState::kUnknown;
   };
 
   static Heap* From(cppgc::Heap* heap) { return static_cast<Heap*>(heap); }
@@ -100,6 +101,8 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
   RawHeap& raw_heap() { return raw_heap_; }
   const RawHeap& raw_heap() const { return raw_heap_; }
 
+  Stack* stack() { return stack_.get(); }
+
   PageBackend* page_backend() { return page_backend_.get(); }
   const PageBackend* page_backend() const { return page_backend_.get(); }
 
@@ -109,6 +112,10 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
 
   size_t ObjectPayloadSize() const;
 
+  // Temporary getter until proper visitation of on-stack objects is
+  // implemented.
+  std::vector<HeapObjectHeader*>& objects() { return objects_; }
+
  private:
   bool in_no_gc_scope() const { return no_gc_scope_ > 0; }
   bool is_allocation_allowed() const { return no_allocation_scope_ == 0; }
@@ -122,6 +129,7 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
 
   std::unique_ptr<Stack> stack_;
   std::unique_ptr<PreFinalizerHandler> prefinalizer_handler_;
+  std::unique_ptr<Marker> marker_;
   std::vector<HeapObjectHeader*> objects_;
 
   PersistentRegion strong_persistent_region_;

src/heap/cppgc/marker.cc

@@ -0,0 +1,152 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/cppgc/marker.h"
+
+#include "src/heap/cppgc/heap-object-header-inl.h"
+#include "src/heap/cppgc/heap.h"
+#include "src/heap/cppgc/marking-visitor.h"
+
+namespace cppgc {
+namespace internal {
+
+namespace {
+template <typename Worklist, typename Callback>
+bool DrainWorklistWithDeadline(v8::base::TimeTicks deadline, Worklist* worklist,
+                               Callback callback, int task_id) {
+  const size_t kDeadlineCheckInterval = 1250;
+
+  size_t processed_callback_count = 0;
+  typename Worklist::View view(worklist, task_id);
+  typename Worklist::EntryType item;
+  while (view.Pop(&item)) {
+    callback(item);
+    if (++processed_callback_count == kDeadlineCheckInterval) {
+      if (deadline <= v8::base::TimeTicks::Now()) {
+        return false;
+      }
+      processed_callback_count = 0;
+    }
+  }
+  return true;
+}
+}  // namespace
+
+constexpr int Marker::kMutatorThreadId;
+
+Marker::Marker(Heap* heap)
+    : heap_(heap), marking_visitor_(CreateMutatorThreadMarkingVisitor()) {}
+
+Marker::~Marker() {
+  // The fixed point iteration may have found not-fully-constructed objects.
+  // Such objects should have already been found through the stack scan though
+  // and should thus already be marked.
+  if (!not_fully_constructed_worklist_.IsEmpty()) {
+#if DEBUG
+    DCHECK_NE(MarkingConfig::StackState::kEmpty, config_.stack_state_);
+    NotFullyConstructedItem item;
+    NotFullyConstructedWorklist::View view(&not_fully_constructed_worklist_,
+                                           kMutatorThreadId);
+    while (view.Pop(&item)) {
+      // TODO(chromium:1056170): uncomment following check after implementing
+      // FromInnerAddress.
+      //
+      // HeapObjectHeader* const header = HeapObjectHeader::FromInnerAddress(
+      //     reinterpret_cast<Address>(const_cast<void*>(item)));
+      // DCHECK(header->IsMarked())
+    }
+#else
+    not_fully_constructed_worklist_.Clear();
+#endif
+  }
+}
+
+void Marker::StartMarking(MarkingConfig config) {
+  config_ = config;
+  VisitRoots();
+}
+
+void Marker::FinishMarking() {
+  if (config_.stack_state_ == MarkingConfig::StackState::kEmpty) {
+    FlushNotFullyConstructedObjects();
+  }
+  AdvanceMarkingWithDeadline(v8::base::TimeDelta::Max());
+}
+
+void Marker::ProcessWeakness() {
+  heap_->GetWeakPersistentRegion().Trace(marking_visitor_.get());
+
+  // Call weak callbacks on objects that may now be pointing to dead objects.
+  WeakCallbackItem item;
+  LivenessBroker broker = LivenessBrokerFactory::Create();
+  WeakCallbackWorklist::View view(&weak_callback_worklist_, kMutatorThreadId);
+  while (view.Pop(&item)) {
+    item.callback(broker, item.parameter);
+  }
+  // Weak callbacks should not add any new objects for marking.
+  DCHECK(marking_worklist_.IsEmpty());
+}
+
+void Marker::VisitRoots() {
+  heap_->GetStrongPersistentRegion().Trace(marking_visitor_.get());
+  if (config_.stack_state_ != MarkingConfig::StackState::kEmpty)
+    heap_->stack()->IteratePointers(marking_visitor_.get());
+}
+
+std::unique_ptr<MutatorThreadMarkingVisitor>
+Marker::CreateMutatorThreadMarkingVisitor() {
+  return std::make_unique<MutatorThreadMarkingVisitor>(this);
+}
+
+bool Marker::AdvanceMarkingWithDeadline(v8::base::TimeDelta duration) {
+  MutatorThreadMarkingVisitor* visitor = marking_visitor_.get();
+  v8::base::TimeTicks deadline = v8::base::TimeTicks::Now() + duration;
+
+  do {
+    // Convert |previously_not_fully_constructed_worklist_| to
+    // |marking_worklist_|. This merely re-adds items with the proper
+    // callbacks.
+    if (!DrainWorklistWithDeadline(
+            deadline, &previously_not_fully_constructed_worklist_,
+            [visitor](NotFullyConstructedItem& item) {
+              visitor->DynamicallyMarkAddress(
+                  reinterpret_cast<ConstAddress>(item));
+            },
+            kMutatorThreadId))
+      return false;
+
+    if (!DrainWorklistWithDeadline(
+            deadline, &marking_worklist_,
+            [visitor](const MarkingItem& item) {
+              const HeapObjectHeader& header =
+                  HeapObjectHeader::FromPayload(item.base_object_payload);
+              DCHECK(!MutatorThreadMarkingVisitor::IsInConstruction(header));
+              item.callback(visitor, item.base_object_payload);
+              visitor->AccountMarkedBytes(header);
+            },
+            kMutatorThreadId))
+      return false;
+  } while (!marking_worklist_.IsLocalViewEmpty(kMutatorThreadId));
+
+  return true;
+}
+
+void Marker::FlushNotFullyConstructedObjects() {
+  if (!not_fully_constructed_worklist_.IsLocalViewEmpty(kMutatorThreadId)) {
+    not_fully_constructed_worklist_.FlushToGlobal(kMutatorThreadId);
+    previously_not_fully_constructed_worklist_.MergeGlobalPool(
+        &not_fully_constructed_worklist_);
+  }
+  DCHECK(not_fully_constructed_worklist_.IsLocalViewEmpty(kMutatorThreadId));
+}
+
+void Marker::ClearAllWorklistsForTesting() {
+  marking_worklist_.Clear();
+  not_fully_constructed_worklist_.Clear();
+  previously_not_fully_constructed_worklist_.Clear();
+  weak_callback_worklist_.Clear();
+}
+
+}  // namespace internal
+}  // namespace cppgc

src/heap/cppgc/marker.h

@@ -0,0 +1,121 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_HEAP_CPPGC_MARKER_H_
+#define V8_HEAP_CPPGC_MARKER_H_
+
+#include <memory>
+
+#include "include/cppgc/heap.h"
+#include "include/cppgc/trace-trait.h"
+#include "include/cppgc/visitor.h"
+#include "src/base/platform/time.h"
+#include "src/heap/cppgc/worklist.h"
+
+namespace cppgc {
+namespace internal {
+
+class Heap;
+class MutatorThreadMarkingVisitor;
+
+class V8_EXPORT_PRIVATE Marker {
+  static constexpr int kNumConcurrentMarkers = 0;
+  static constexpr int kNumMarkers = 1 + kNumConcurrentMarkers;
+
+ public:
+  static constexpr int kMutatorThreadId = 0;
+
+  using MarkingItem = cppgc::TraceDescriptor;
+  using NotFullyConstructedItem = const void*;
+  struct WeakCallbackItem {
+    cppgc::WeakCallback callback;
+    const void* parameter;
+  };
+
+  // Segment size of 512 entries necessary to avoid throughput regressions.
+  // Since the work list is currently a temporary object this is not a problem.
+  using MarkingWorklist =
+      Worklist<MarkingItem, 512 /* local entries */, kNumMarkers>;
+  using NotFullyConstructedWorklist =
+      Worklist<NotFullyConstructedItem, 16 /* local entries */, kNumMarkers>;
+  using WeakCallbackWorklist =
+      Worklist<WeakCallbackItem, 64 /* local entries */, kNumMarkers>;
+
+  struct MarkingConfig {
+    using StackState = cppgc::Heap::StackState;
+    enum class IncrementalMarking : uint8_t { kDisabled };
+    enum class ConcurrentMarking : uint8_t { kDisabled };
+
+    static MarkingConfig Default() {
+      return {StackState::kUnknown, IncrementalMarking::kDisabled,
+              ConcurrentMarking::kDisabled};
+    }
+
+    explicit MarkingConfig(StackState stack_state)
+        : MarkingConfig(stack_state, IncrementalMarking::kDisabled,
+                        ConcurrentMarking::kDisabled) {}
+
+    MarkingConfig(StackState stack_state,
+                  IncrementalMarking incremental_marking_state,
+                  ConcurrentMarking concurrent_marking_state)
+        : stack_state_(stack_state),
+          incremental_marking_state_(incremental_marking_state),
+          concurrent_marking_state_(concurrent_marking_state) {}
+
+    StackState stack_state_;
+    IncrementalMarking incremental_marking_state_;
+    ConcurrentMarking concurrent_marking_state_;
+  };
+
+  explicit Marker(Heap* heap);
+  virtual ~Marker();
+
+  Marker(const Marker&) = delete;
+  Marker& operator=(const Marker&) = delete;
+
+  // Initialize marking according to the given config. This method will
+  // trigger incremental/concurrent marking if needed.
+  void StartMarking(MarkingConfig config);
+  // Finalize marking. This method stops incremental/concurrent marking
+  // if exsists and performs atomic pause marking.
+  void FinishMarking();
+
+  void ProcessWeakness();
+
+  Heap* heap() { return heap_; }
+  MarkingWorklist* marking_worklist() { return &marking_worklist_; }
+  NotFullyConstructedWorklist* not_fully_constructed_worklist() {
+    return &not_fully_constructed_worklist_;
+  }
+  WeakCallbackWorklist* weak_callback_worklist() {
+    return &weak_callback_worklist_;
+  }
+
+  void ClearAllWorklistsForTesting();
+
+ protected:
+  virtual std::unique_ptr<MutatorThreadMarkingVisitor>
+  CreateMutatorThreadMarkingVisitor();
+
+ private:
+  void VisitRoots();
+
+  bool AdvanceMarkingWithDeadline(v8::base::TimeDelta);
+  void FlushNotFullyConstructedObjects();
+
+  Heap* const heap_;
+  MarkingConfig config_ = MarkingConfig::Default();
+
+  std::unique_ptr<MutatorThreadMarkingVisitor> marking_visitor_;
+
+  MarkingWorklist marking_worklist_;
+  NotFullyConstructedWorklist not_fully_constructed_worklist_;
+  NotFullyConstructedWorklist previously_not_fully_constructed_worklist_;
+  WeakCallbackWorklist weak_callback_worklist_;
+};
+
+}  // namespace internal
+}  // namespace cppgc
+
+#endif  // V8_HEAP_CPPGC_MARKER_H_

src/heap/cppgc/marking-visitor.cc

@@ -0,0 +1,143 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/cppgc/marking-visitor.h"
+
+#include "include/cppgc/garbage-collected.h"
+#include "include/cppgc/internal/accessors.h"
+#include "src/heap/cppgc/heap-object-header-inl.h"
+#include "src/heap/cppgc/heap.h"
+
+namespace cppgc {
+namespace internal {
+
+// static
+bool MarkingVisitor::IsInConstruction(const HeapObjectHeader& header) {
+  return header.IsInConstruction<HeapObjectHeader::AccessMode::kNonAtomic>();
+}
+
+MarkingVisitor::MarkingVisitor(Marker* marking_handler, int task_id)
+    : marker_(marking_handler),
+      marking_worklist_(marking_handler->marking_worklist(), task_id),
+      not_fully_constructed_worklist_(
+          marking_handler->not_fully_constructed_worklist(), task_id),
+      weak_callback_worklist_(marking_handler->weak_callback_worklist(),
+                              task_id) {}
+
+void MarkingVisitor::AccountMarkedBytes(const HeapObjectHeader& header) {
+  marked_bytes_ +=
+      header.IsLargeObject()
+          ? reinterpret_cast<const LargePage*>(BasePage::FromPayload(&header))
+                ->PayloadSize()
+          : header.GetSize();
+}
+
+void MarkingVisitor::Visit(const void* object, TraceDescriptor desc) {
+  DCHECK_NOT_NULL(object);
+  if (desc.base_object_payload ==
+      cppgc::GarbageCollectedMixin::kNotFullyConstructedObject) {
+    // This means that the objects are not-yet-fully-constructed. See comments
+    // on GarbageCollectedMixin for how those objects are handled.
+    not_fully_constructed_worklist_.Push(object);
+    return;
+  }
+  MarkHeader(&HeapObjectHeader::FromPayload(
+                 const_cast<void*>(desc.base_object_payload)),
+             desc);
+}
+
+void MarkingVisitor::VisitWeak(const void* object, TraceDescriptor desc,
+                               WeakCallback weak_callback,
+                               const void* weak_member) {
+  // Filter out already marked values. The write barrier for WeakMember
+  // ensures that any newly set value after this point is kept alive and does
+  // not require the callback.
+  if (desc.base_object_payload !=
+          cppgc::GarbageCollectedMixin::kNotFullyConstructedObject &&
+      HeapObjectHeader::FromPayload(desc.base_object_payload)
+          .IsMarked<HeapObjectHeader::AccessMode::kAtomic>())
+    return;
+  RegisterWeakCallback(weak_callback, weak_member);
+}
+
+void MarkingVisitor::VisitRoot(const void* object, TraceDescriptor desc) {
+  Visit(object, desc);
+}
+
+void MarkingVisitor::VisitWeakRoot(const void* object, TraceDescriptor desc,
+                                   WeakCallback weak_callback,
+                                   const void* weak_root) {
+  if (desc.base_object_payload ==
+      cppgc::GarbageCollectedMixin::kNotFullyConstructedObject) {
+    // This method is only called at the end of marking. If the object is in
+    // construction, then it should be reachable from the stack.
+    return;
+  }
+  // Since weak roots arev only traced at the end of marking, we can execute
+  // the callback instead of registering it.
+  weak_callback(LivenessBrokerFactory::Create(), weak_root);
+}
+
+void MarkingVisitor::MarkHeader(HeapObjectHeader* header,
+                                TraceDescriptor desc) {
+  DCHECK(header);
+  DCHECK_NOT_NULL(desc.callback);
+
+  if (IsInConstruction(*header)) {
+    not_fully_constructed_worklist_.Push(header->Payload());
+  } else if (MarkHeaderNoTracing(header)) {
+    marking_worklist_.Push(desc);
+  }
+}
+
+bool MarkingVisitor::MarkHeaderNoTracing(HeapObjectHeader* header) {
+  DCHECK(header);
+  // A GC should only mark the objects that belong in its heap.
+  DCHECK_EQ(marker_->heap(), BasePage::FromPayload(header)->heap());
+  // Never mark free space objects. This would e.g. hint to marking a promptly
+  // freed backing store.
+  DCHECK(!header->IsFree());
+
+  return header->TryMarkAtomic();
+}
+
+void MarkingVisitor::RegisterWeakCallback(WeakCallback callback,
+                                          const void* object) {
+  weak_callback_worklist_.Push({callback, object});
+}
+
+void MarkingVisitor::FlushWorklists() {
+  marking_worklist_.FlushToGlobal();
+  not_fully_constructed_worklist_.FlushToGlobal();
+  weak_callback_worklist_.FlushToGlobal();
+}
+
+void MarkingVisitor::DynamicallyMarkAddress(ConstAddress address) {
+  for (auto* header : marker_->heap()->objects()) {
+    if (address >= header->Payload() &&
+        address < (header->Payload() + header->GetSize())) {
+      header->TryMarkAtomic();
+    }
+  }
+  // TODO(chromium:1056170): Implement dynamically getting HeapObjectHeader
+  // for handling previously_not_fully_constructed objects. Requires object
+  // start bitmap.
+}
+
+void MarkingVisitor::VisitPointer(const void* address) {
+  for (auto* header : marker_->heap()->objects()) {
+    if (address >= header->Payload() &&
+        address < (header->Payload() + header->GetSize())) {
+      header->TryMarkAtomic();
+    }
+  }
+  // TODO(chromium:1056170): Implement proper conservative scanning for
+  // on-stack objects. Requires page bloom filter.
+}
+
+MutatorThreadMarkingVisitor::MutatorThreadMarkingVisitor(Marker* marker)
+    : MarkingVisitor(marker, Marker::kMutatorThreadId) {}
+
+}  // namespace internal
+}  // namespace cppgc

src/heap/cppgc/marking-visitor.h

@@ -0,0 +1,70 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_HEAP_CPPGC_MARKING_VISITOR_H_
+#define V8_HEAP_CPPGC_MARKING_VISITOR_H_
+
+#include "include/cppgc/source-location.h"
+#include "include/cppgc/trace-trait.h"
+#include "include/v8config.h"
+#include "src/heap/cppgc/globals.h"
+#include "src/heap/cppgc/heap-object-header.h"
+#include "src/heap/cppgc/heap-page.h"
+#include "src/heap/cppgc/heap.h"
+#include "src/heap/cppgc/marker.h"
+#include "src/heap/cppgc/stack.h"
+#include "src/heap/cppgc/visitor.h"
+
+namespace cppgc {
+namespace internal {
+
+class MarkingVisitor : public VisitorBase, public StackVisitor {
+ public:
+  MarkingVisitor(Marker*, int);
+  virtual ~MarkingVisitor() = default;
+
+  MarkingVisitor(const MarkingVisitor&) = delete;
+  MarkingVisitor& operator=(const MarkingVisitor&) = delete;
+
+  void FlushWorklists();
+
+  void DynamicallyMarkAddress(ConstAddress);
+
+  void AccountMarkedBytes(const HeapObjectHeader&);
+  size_t marked_bytes() const { return marked_bytes_; }
+
+  static bool IsInConstruction(const HeapObjectHeader&);
+
+ protected:
+  void Visit(const void*, TraceDescriptor) override;
+  void VisitWeak(const void*, TraceDescriptor, WeakCallback,
+                 const void*) override;
+  void VisitRoot(const void*, TraceDescriptor) override;
+  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback,
+                     const void*) override;
+
+  void VisitPointer(const void*) override;
+
+ private:
+  void MarkHeader(HeapObjectHeader*, TraceDescriptor);
+  bool MarkHeaderNoTracing(HeapObjectHeader*);
+  void RegisterWeakCallback(WeakCallback, const void*) override;
+
+  Marker* const marker_;
+  Marker::MarkingWorklist::View marking_worklist_;
+  Marker::NotFullyConstructedWorklist::View not_fully_constructed_worklist_;
+  Marker::WeakCallbackWorklist::View weak_callback_worklist_;
+
+  size_t marked_bytes_;
+};
+
+class V8_EXPORT_PRIVATE MutatorThreadMarkingVisitor : public MarkingVisitor {
+ public:
+  explicit MutatorThreadMarkingVisitor(Marker*);
+};
+
+}  // namespace internal
+}  // namespace cppgc
+
+#endif  // V8_HEAP_CPPGC_MARKING_VISITOR_H_

test/unittests/BUILD.gn

@@ -52,6 +52,8 @@ v8_source_set("cppgc_unittests_sources") {
     "heap/cppgc/heap-page_unittest.cc",
     "heap/cppgc/heap_unittest.cc",
     "heap/cppgc/logging_unittest.cc",
+    "heap/cppgc/marker_unittest.cc",
+    "heap/cppgc/marking-visitor_unittest.cc",
     "heap/cppgc/member_unittests.cc",
     "heap/cppgc/page-memory_unittest.cc",
     "heap/cppgc/persistent_unittests.cc",

test/unittests/heap/cppgc/marker_unittest.cc

@@ -0,0 +1,186 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/cppgc/marker.h"
+
+#include "include/cppgc/allocation.h"
+#include "include/cppgc/member.h"
+#include "include/cppgc/persistent.h"
+#include "src/heap/cppgc/heap-object-header-inl.h"
+#include "test/unittests/heap/cppgc/tests.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace cppgc {
+namespace internal {
+
+namespace {
+
+class MarkerTest : public testing::TestWithHeap {
+ public:
+  using MarkingConfig = Marker::MarkingConfig;
+
+  void DoMarking(MarkingConfig config) {
+    Marker marker(Heap::From(GetHeap()));
+    marker.StartMarking(config);
+    marker.FinishMarking();
+    marker.ProcessWeakness();
+  }
+};
+
+class GCed : public GarbageCollected<GCed> {
+ public:
+  void SetChild(GCed* child) { child_ = child; }
+  void SetWeakChild(GCed* child) { weak_child_ = child; }
+  GCed* child() const { return child_.Get(); }
+  GCed* weak_child() const { return weak_child_.Get(); }
+  void Trace(cppgc::Visitor* visitor) const {
+    visitor->Trace(child_);
+    visitor->Trace(weak_child_);
+  }
+
+ private:
+  Member<GCed> child_;
+  WeakMember<GCed> weak_child_;
+};
+
+template <typename T>
+V8_NOINLINE T access(volatile const T& t) {
+  return t;
+}
+
+}  // namespace
+
+TEST_F(MarkerTest, PersistentIsMarked) {
+  Persistent<GCed> object = MakeGarbageCollected<GCed>(GetHeap());
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+  EXPECT_FALSE(header.IsMarked());
+  DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+  EXPECT_TRUE(header.IsMarked());
+}
+
+TEST_F(MarkerTest, ReachableMemberIsMarked) {
+  Persistent<GCed> parent = MakeGarbageCollected<GCed>(GetHeap());
+  parent->SetChild(MakeGarbageCollected<GCed>(GetHeap()));
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(parent->child());
+  EXPECT_FALSE(header.IsMarked());
+  DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+  EXPECT_TRUE(header.IsMarked());
+}
+
+TEST_F(MarkerTest, UnreachableMemberIsNotMarked) {
+  Member<GCed> object = MakeGarbageCollected<GCed>(GetHeap());
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+  EXPECT_FALSE(header.IsMarked());
+  DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+  EXPECT_FALSE(header.IsMarked());
+}
+
+TEST_F(MarkerTest, ObjectReachableFromStackIsMarked) {
+  GCed* object = MakeGarbageCollected<GCed>(GetHeap());
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(object).IsMarked());
+  DoMarking(MarkingConfig(MarkingConfig::StackState::kNonEmpty));
+  EXPECT_TRUE(HeapObjectHeader::FromPayload(object).IsMarked());
+  access(object);
+}
+
+TEST_F(MarkerTest, ObjectReachableOnlyFromStackIsNotMarkedIfStackIsEmpty) {
+  GCed* object = MakeGarbageCollected<GCed>(GetHeap());
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+  EXPECT_FALSE(header.IsMarked());
+  DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+  EXPECT_FALSE(header.IsMarked());
+  access(object);
+}
+
+TEST_F(MarkerTest, WeakReferenceToUnreachableObjectIsCleared) {
+  {
+    WeakPersistent<GCed> weak_object = MakeGarbageCollected<GCed>(GetHeap());
+    EXPECT_TRUE(weak_object);
+    DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+    EXPECT_FALSE(weak_object);
+  }
+  {
+    Persistent<GCed> parent = MakeGarbageCollected<GCed>(GetHeap());
+    parent->SetWeakChild(MakeGarbageCollected<GCed>(GetHeap()));
+    EXPECT_TRUE(parent->weak_child());
+    DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+    EXPECT_FALSE(parent->weak_child());
+  }
+}
+
+TEST_F(MarkerTest, WeakReferenceToReachableObjectIsNotCleared) {
+  // Reachable from Persistent
+  {
+    Persistent<GCed> object = MakeGarbageCollected<GCed>(GetHeap());
+    WeakPersistent<GCed> weak_object(object);
+    EXPECT_TRUE(weak_object);
+    DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+    EXPECT_TRUE(weak_object);
+  }
+  {
+    Persistent<GCed> object = MakeGarbageCollected<GCed>(GetHeap());
+    Persistent<GCed> parent = MakeGarbageCollected<GCed>(GetHeap());
+    parent->SetWeakChild(object);
+    EXPECT_TRUE(parent->weak_child());
+    DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+    EXPECT_TRUE(parent->weak_child());
+  }
+  // Reachable from Member
+  {
+    Persistent<GCed> parent = MakeGarbageCollected<GCed>(GetHeap());
+    WeakPersistent<GCed> weak_object(MakeGarbageCollected<GCed>(GetHeap()));
+    parent->SetChild(weak_object);
+    EXPECT_TRUE(weak_object);
+    DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+    EXPECT_TRUE(weak_object);
+  }
+  {
+    Persistent<GCed> parent = MakeGarbageCollected<GCed>(GetHeap());
+    parent->SetChild(MakeGarbageCollected<GCed>(GetHeap()));
+    parent->SetWeakChild(parent->child());
+    EXPECT_TRUE(parent->weak_child());
+    DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+    EXPECT_TRUE(parent->weak_child());
+  }
+  // Reachable from stack
+  {
+    GCed* object = MakeGarbageCollected<GCed>(GetHeap());
+    WeakPersistent<GCed> weak_object(object);
+    EXPECT_TRUE(weak_object);
+    DoMarking(MarkingConfig(MarkingConfig::StackState::kNonEmpty));
+    EXPECT_TRUE(weak_object);
+    access(object);
+  }
+  {
+    GCed* object = MakeGarbageCollected<GCed>(GetHeap());
+    Persistent<GCed> parent = MakeGarbageCollected<GCed>(GetHeap());
+    parent->SetWeakChild(object);
+    EXPECT_TRUE(parent->weak_child());
+    DoMarking(MarkingConfig(MarkingConfig::StackState::kNonEmpty));
+    EXPECT_TRUE(parent->weak_child());
+    access(object);
+  }
+}
+
+TEST_F(MarkerTest, DeepHierarchyIsMarked) {
+  static constexpr int kHierarchyDepth = 10;
+  Persistent<GCed> root = MakeGarbageCollected<GCed>(GetHeap());
+  GCed* parent = root;
+  for (int i = 0; i < kHierarchyDepth; ++i) {
+    parent->SetChild(MakeGarbageCollected<GCed>(GetHeap()));
+    parent->SetWeakChild(parent->child());
+    parent = parent->child();
+  }
+  DoMarking(MarkingConfig(MarkingConfig::StackState::kEmpty));
+  EXPECT_TRUE(HeapObjectHeader::FromPayload(root).IsMarked());
+  parent = root;
+  for (int i = 0; i < kHierarchyDepth; ++i) {
+    EXPECT_TRUE(HeapObjectHeader::FromPayload(parent->child()).IsMarked());
+    EXPECT_TRUE(parent->weak_child());
+    parent = parent->child();
+  }
+}
+
+}  // namespace internal
+}  // namespace cppgc

test/unittests/heap/cppgc/marking-visitor_unittest.cc

@@ -0,0 +1,285 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/cppgc/marking-visitor.h"
+
+#include "include/cppgc/allocation.h"
+#include "include/cppgc/member.h"
+#include "include/cppgc/persistent.h"
+#include "include/cppgc/source-location.h"
+#include "src/heap/cppgc/globals.h"
+#include "src/heap/cppgc/heap-object-header-inl.h"
+#include "src/heap/cppgc/marker.h"
+#include "test/unittests/heap/cppgc/tests.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace cppgc {
+namespace internal {
+
+namespace {
+
+class MarkingVisitorTest : public testing::TestWithHeap {
+ public:
+  MarkingVisitorTest()
+      : marker_(std::make_unique<Marker>(Heap::From(GetHeap()))) {}
+  ~MarkingVisitorTest() { marker_->ClearAllWorklistsForTesting(); }
+
+  Marker* GetMarker() { return marker_.get(); }
+
+ private:
+  std::unique_ptr<Marker> marker_;
+};
+
+class GCed : public GarbageCollected<GCed> {
+ public:
+  void Trace(cppgc::Visitor*) const {}
+};
+
+class Mixin : public GarbageCollectedMixin {};
+class GCedWithMixin : public GarbageCollected<GCedWithMixin>, public Mixin {
+  USING_GARBAGE_COLLECTED_MIXIN();
+
+ public:
+  void Trace(cppgc::Visitor*) const override {}
+};
+
+}  // namespace
+
+// Strong refernces are marked.
+
+TEST_F(MarkingVisitorTest, MarkMember) {
+  Member<GCed> object(MakeGarbageCollected<GCed>(GetHeap()));
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+
+  EXPECT_FALSE(header.IsMarked());
+
+  visitor.Trace(object);
+
+  EXPECT_TRUE(header.IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, MarkMemberMixin) {
+  GCedWithMixin* object(MakeGarbageCollected<GCedWithMixin>(GetHeap()));
+  Member<Mixin> mixin(object);
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+
+  EXPECT_FALSE(header.IsMarked());
+
+  visitor.Trace(mixin);
+
+  EXPECT_TRUE(header.IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, MarkPersistent) {
+  Persistent<GCed> object(MakeGarbageCollected<GCed>(GetHeap()));
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+
+  EXPECT_FALSE(header.IsMarked());
+
+  visitor.TraceRoot(object, SourceLocation::Current());
+
+  EXPECT_TRUE(header.IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, MarkPersistentMixin) {
+  GCedWithMixin* object(MakeGarbageCollected<GCedWithMixin>(GetHeap()));
+  Persistent<Mixin> mixin(object);
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+
+  EXPECT_FALSE(header.IsMarked());
+
+  visitor.TraceRoot(mixin, SourceLocation::Current());
+
+  EXPECT_TRUE(header.IsMarked());
+}
+
+// Weak references are not marked.
+
+TEST_F(MarkingVisitorTest, DontMarkWeakMember) {
+  WeakMember<GCed> object(MakeGarbageCollected<GCed>(GetHeap()));
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+
+  EXPECT_FALSE(header.IsMarked());
+
+  visitor.Trace(object);
+
+  EXPECT_FALSE(header.IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, DontMarkWeakMemberMixin) {
+  GCedWithMixin* object(MakeGarbageCollected<GCedWithMixin>(GetHeap()));
+  WeakMember<Mixin> mixin(object);
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+
+  EXPECT_FALSE(header.IsMarked());
+
+  visitor.Trace(mixin);
+
+  EXPECT_FALSE(header.IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, DontMarkWeakPersistent) {
+  WeakPersistent<GCed> object(MakeGarbageCollected<GCed>(GetHeap()));
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+
+  EXPECT_FALSE(header.IsMarked());
+
+  visitor.TraceRoot(object, SourceLocation::Current());
+
+  EXPECT_FALSE(header.IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, DontMarkWeakPersistentMixin) {
+  GCedWithMixin* object(MakeGarbageCollected<GCedWithMixin>(GetHeap()));
+  WeakPersistent<Mixin> mixin(object);
+  HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
+
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+
+  EXPECT_FALSE(header.IsMarked());
+
+  visitor.TraceRoot(mixin, SourceLocation::Current());
+
+  EXPECT_FALSE(header.IsMarked());
+}
+
+// In construction objects are not marked.
+
+namespace {
+
+class GCedWithInConstructionCallback
+    : public GarbageCollected<GCedWithInConstructionCallback> {
+ public:
+  template <typename Callback>
+  explicit GCedWithInConstructionCallback(Callback callback) {
+    callback(this);
+  }
+  void Trace(cppgc::Visitor*) const {}
+};
+
+class MixinWithInConstructionCallback : public GarbageCollectedMixin {
+ public:
+  template <typename Callback>
+  explicit MixinWithInConstructionCallback(Callback callback) {
+    callback(this);
+  }
+};
+class GCedWithMixinWithInConstructionCallback
+    : public GarbageCollected<GCedWithMixinWithInConstructionCallback>,
+      public MixinWithInConstructionCallback {
+  USING_GARBAGE_COLLECTED_MIXIN();
+
+ public:
+  template <typename Callback>
+  explicit GCedWithMixinWithInConstructionCallback(Callback callback)
+      : MixinWithInConstructionCallback(callback) {}
+  void Trace(cppgc::Visitor*) const override {}
+};
+
+}  // namespace
+
+TEST_F(MarkingVisitorTest, DontMarkMemberInConstruction) {
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+  GCedWithInConstructionCallback* gced =
+      MakeGarbageCollected<GCedWithInConstructionCallback>(
+          GetHeap(), [&visitor](GCedWithInConstructionCallback* obj) {
+            Member<GCedWithInConstructionCallback> object(obj);
+            visitor.Trace(object);
+          });
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, DontMarkMemberMixinInConstruction) {
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+  GCedWithMixinWithInConstructionCallback* gced =
+      MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
+          GetHeap(), [&visitor](MixinWithInConstructionCallback* obj) {
+            Member<MixinWithInConstructionCallback> mixin(obj);
+            visitor.Trace(mixin);
+          });
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, DontMarkWeakMemberInConstruction) {
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+  GCedWithInConstructionCallback* gced =
+      MakeGarbageCollected<GCedWithInConstructionCallback>(
+          GetHeap(), [&visitor](GCedWithInConstructionCallback* obj) {
+            WeakMember<GCedWithInConstructionCallback> object(obj);
+            visitor.Trace(object);
+          });
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, DontMarkWeakMemberMixinInConstruction) {
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+  GCedWithMixinWithInConstructionCallback* gced =
+      MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
+          GetHeap(), [&visitor](MixinWithInConstructionCallback* obj) {
+            WeakMember<MixinWithInConstructionCallback> mixin(obj);
+            visitor.Trace(mixin);
+          });
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, DontMarkPersistentInConstruction) {
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+  GCedWithInConstructionCallback* gced =
+      MakeGarbageCollected<GCedWithInConstructionCallback>(
+          GetHeap(), [&visitor](GCedWithInConstructionCallback* obj) {
+            Persistent<GCedWithInConstructionCallback> object(obj);
+            visitor.TraceRoot(object, SourceLocation::Current());
+          });
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, DontMarkPersistentMixinInConstruction) {
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+  GCedWithMixinWithInConstructionCallback* gced =
+      MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
+          GetHeap(), [&visitor](MixinWithInConstructionCallback* obj) {
+            Persistent<MixinWithInConstructionCallback> mixin(obj);
+            visitor.TraceRoot(mixin, SourceLocation::Current());
+          });
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, DontMarkWeakPersistentInConstruction) {
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+  GCedWithInConstructionCallback* gced =
+      MakeGarbageCollected<GCedWithInConstructionCallback>(
+          GetHeap(), [&visitor](GCedWithInConstructionCallback* obj) {
+            WeakPersistent<GCedWithInConstructionCallback> object(obj);
+            visitor.TraceRoot(object, SourceLocation::Current());
+          });
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+}
+
+TEST_F(MarkingVisitorTest, DontMarkWeakPersistentMixinInConstruction) {
+  MutatorThreadMarkingVisitor visitor(GetMarker());
+  GCedWithMixinWithInConstructionCallback* gced =
+      MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
+          GetHeap(), [&visitor](MixinWithInConstructionCallback* obj) {
+            WeakPersistent<MixinWithInConstructionCallback> mixin(obj);
+            visitor.TraceRoot(mixin, SourceLocation::Current());
+          });
+  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+}
+
+}  // namespace internal
+}  // namespace cppgc

test/unittests/heap/cppgc/persistent_unittests.cc

@@ -59,11 +59,11 @@ class RootVisitor final : public VisitorBase {
   }
 
  protected:
-  void VisitRoot(const void* t, TraceDescriptor desc,
+  void VisitRoot(const void* t, TraceDescriptor desc) final {
-                 const SourceLocation&) final {
     desc.callback(this, desc.base_object_payload);
   }
-  void VisitWeakRoot(const void* object, WeakCallback callback) final {
+  void VisitWeakRoot(const void*, TraceDescriptor, WeakCallback callback,
+                     const void* object) final {
     weak_callbacks_.emplace_back(callback, object);
   }