cppgc: Various cleanups

- Cleanup includes, fix typo, fix qualifiers.
- Fix getter names of MarkerBase when only exposed for testing.

Bug: chromium:1056170
Change-Id: Ibcb0f62414c9c865fa98e6d2b2c9b150aa2a361f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2281004
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Auto-Submit: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68678}

include/cppgc/garbage-collected.h

@@ -176,9 +176,9 @@ class GarbageCollectedMixin : public internal::GarbageCollectedBase {
  */
 #define MERGE_GARBAGE_COLLECTED_MIXINS()                \
  public:                                                \
-  /* When using multiple mixins the methods become  */  \
-  /* ambigous. Providing additional implementations */  \
-  /* disambiguate them again.                       */  \
+  /* When using multiple mixins the methods become   */ \
+  /* ambiguous. Providing additional implementations */ \
+  /* disambiguate them again.                        */ \
   TraceDescriptor GetTraceDescriptor() const override { \
     return {kNotFullyConstructedObject, nullptr};       \
   }                                                     \

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

@@ -82,8 +82,9 @@ class UnifiedHeapMarker : public cppgc::internal::MarkerBase {
 
 UnifiedHeapMarker::UnifiedHeapMarker(cppgc::internal::HeapBase& heap)
     : cppgc::internal::MarkerBase(heap),
-      marking_visitor_(heap, marking_state()),
-      conservative_marking_visitor_(heap, marking_state(), marking_visitor_) {}
+      marking_visitor_(heap, mutator_marking_state_),
+      conservative_marking_visitor_(heap, mutator_marking_state_,
+                                    marking_visitor_) {}
 
 void UnifiedHeapMarker::AddObject(void* object) {
   mutator_marking_state_.MarkAndPush(

src/heap/cppgc/marker.cc

@@ -259,8 +259,9 @@ void MarkerBase::ClearAllWorklistsForTesting() {
 
 Marker::Marker(HeapBase& heap)
     : MarkerBase(heap),
-      marking_visitor_(heap, marking_state()),
-      conservative_marking_visitor_(heap, marking_state(), marking_visitor_) {}
+      marking_visitor_(heap, mutator_marking_state_),
+      conservative_marking_visitor_(heap, mutator_marking_state_,
+                                    marking_visitor_) {}
 
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/marker.h

@@ -8,10 +8,10 @@
 #include <memory>
 
 #include "include/cppgc/heap.h"
-#include "include/cppgc/trace-trait.h"
 #include "include/cppgc/visitor.h"
 #include "src/base/macros.h"
 #include "src/base/platform/time.h"
+#include "src/heap/cppgc/globals.h"
 #include "src/heap/cppgc/marking-state.h"
 #include "src/heap/cppgc/marking-visitor.h"
 #include "src/heap/cppgc/marking-worklists.h"
@@ -82,10 +82,13 @@ class V8_EXPORT_PRIVATE MarkerBase {
 
   void ProcessWeakness();
 
+  inline void WriteBarrierForInConstructionObject(ConstAddress);
+  inline void WriteBarrierForObject(HeapObjectHeader&);
+
   HeapBase& heap() { return heap_; }
-  MarkingState& marking_state() { return mutator_marking_state_; }
-  MarkingWorklists& marking_worklists() { return marking_worklists_; }
 
+  MarkingWorklists& MarkingWorklistsForTesting() { return marking_worklists_; }
+  MarkingState& MarkingStateForTesting() { return mutator_marking_state_; }
   cppgc::Visitor& VisitorForTesting() { return visitor(); }
   void ClearAllWorklistsForTesting();
 
@@ -125,6 +128,21 @@ class V8_EXPORT_PRIVATE Marker final : public MarkerBase {
   ConservativeMarkingVisitor conservative_marking_visitor_;
 };
 
+void MarkerBase::WriteBarrierForInConstructionObject(ConstAddress payload) {
+  MarkingWorklists::NotFullyConstructedWorklist::View
+      not_fully_constructed_worklist(
+          marking_worklists_.not_fully_constructed_worklist(),
+          MarkingWorklists::kMutatorThreadId);
+  not_fully_constructed_worklist.Push(payload);
+}
+
+void MarkerBase::WriteBarrierForObject(HeapObjectHeader& header) {
+  MarkingWorklists::WriteBarrierWorklist::View write_barrier_worklist(
+      marking_worklists_.write_barrier_worklist(),
+      MarkingWorklists::kMutatorThreadId);
+  write_barrier_worklist.Push(&header);
+}
+
 }  // namespace internal
 }  // namespace cppgc
 

src/heap/cppgc/marking-visitor.cc

@@ -4,11 +4,7 @@
 
 #include "src/heap/cppgc/marking-visitor.h"
 
-#include "include/cppgc/garbage-collected.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/liveness-broker.h"
 #include "src/heap/cppgc/marking-state.h"
 
 namespace cppgc {
@@ -60,8 +56,5 @@ void ConservativeMarkingVisitor::VisitPointer(const void* address) {
   TraceConservativelyIfNeeded(address);
 }
 
-MutatorThreadMarkingVisitor::MutatorThreadMarkingVisitor(Marker* marker)
-    : MarkingVisitor(marker->heap(), marker->marking_state()) {}
-
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/marking-visitor.h

@@ -18,7 +18,7 @@ class HeapObjectHeader;
 class Marker;
 class MarkingState;
 
-class MarkingVisitor : public VisitorBase {
+class V8_EXPORT_PRIVATE MarkingVisitor : public VisitorBase {
  public:
   MarkingVisitor(HeapBase&, MarkingState&);
   ~MarkingVisitor() override = default;
@@ -43,16 +43,11 @@ class ConservativeMarkingVisitor : public ConservativeTracingVisitor,
  private:
   void VisitConservatively(HeapObjectHeader&,
                            TraceConservativelyCallback) final;
-  void VisitPointer(const void*) override;
+  void VisitPointer(const void*) final;
 
   MarkingState& marking_state_;
 };
 
-class V8_EXPORT_PRIVATE MutatorThreadMarkingVisitor : public MarkingVisitor {
- public:
-  explicit MutatorThreadMarkingVisitor(Marker*);
-};
-
 }  // namespace internal
 }  // namespace cppgc
 

src/heap/cppgc/write-barrier.cc

@@ -40,18 +40,11 @@ void MarkValue(const BasePage* page, MarkerBase* marker, const void* value) {
     // It is assumed that objects on not_fully_constructed_worklist_ are not
     // marked.
     header.Unmark();
-    MarkingWorklists::NotFullyConstructedWorklist::View
-        not_fully_constructed_worklist(
-            marker->marking_worklists().not_fully_constructed_worklist(),
-            MarkingWorklists::kMutatorThreadId);
-    not_fully_constructed_worklist.Push(header.Payload());
+    marker->WriteBarrierForInConstructionObject(header.Payload());
     return;
   }
 
-  MarkingWorklists::WriteBarrierWorklist::View write_barrier_worklist(
-      marker->marking_worklists().write_barrier_worklist(),
-      MarkingWorklists::kMutatorThreadId);
-  write_barrier_worklist.Push(&header);
+  marker->WriteBarrierForObject(header);
 }
 
 }  // namespace

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

@@ -24,7 +24,7 @@ class MarkingVisitorTest : public testing::TestWithHeap {
  public:
   MarkingVisitorTest()
       : marker_(std::make_unique<Marker>(Heap::From(GetHeap())->AsBase())) {}
-  ~MarkingVisitorTest() { marker_->ClearAllWorklistsForTesting(); }
+  ~MarkingVisitorTest() override { marker_->ClearAllWorklistsForTesting(); }
 
   Marker* GetMarker() { return marker_.get(); }
 
@@ -45,10 +45,16 @@ class GCedWithMixin : public GarbageCollected<GCedWithMixin>, public Mixin {
   void Trace(cppgc::Visitor*) const override {}
 };
 
+class TestMarkingVisitor : public MarkingVisitor {
+ public:
+  explicit TestMarkingVisitor(Marker* marker)
+      : MarkingVisitor(marker->heap(), marker->MarkingStateForTesting()) {}
+};
+
 }  // namespace
 
 TEST_F(MarkingVisitorTest, MarkedBytesAreInitiallyZero) {
-  EXPECT_EQ(0u, GetMarker()->marking_state().marked_bytes());
+  EXPECT_EQ(0u, GetMarker()->MarkingStateForTesting().marked_bytes());
 }
 
 // Strong references are marked.
@@ -57,7 +63,7 @@ TEST_F(MarkingVisitorTest, MarkMember) {
   Member<GCed> object(MakeGarbageCollected<GCed>(GetAllocationHandle()));
   HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
 
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
@@ -72,7 +78,7 @@ TEST_F(MarkingVisitorTest, MarkMemberMixin) {
   Member<Mixin> mixin(object);
   HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
 
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
@@ -85,7 +91,7 @@ TEST_F(MarkingVisitorTest, MarkPersistent) {
   Persistent<GCed> object(MakeGarbageCollected<GCed>(GetAllocationHandle()));
   HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
 
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
@@ -100,7 +106,7 @@ TEST_F(MarkingVisitorTest, MarkPersistentMixin) {
   Persistent<Mixin> mixin(object);
   HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
 
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
@@ -115,7 +121,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakMember) {
   WeakMember<GCed> object(MakeGarbageCollected<GCed>(GetAllocationHandle()));
   HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
 
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
@@ -130,7 +136,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakMemberMixin) {
   WeakMember<Mixin> mixin(object);
   HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
 
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
@@ -144,7 +150,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakPersistent) {
       MakeGarbageCollected<GCed>(GetAllocationHandle()));
   HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
 
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
@@ -159,7 +165,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakPersistentMixin) {
   WeakPersistent<Mixin> mixin(object);
   HeapObjectHeader& header = HeapObjectHeader::FromPayload(object);
 
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
 
   EXPECT_FALSE(header.IsMarked());
 
@@ -204,7 +210,7 @@ class GCedWithMixinWithInConstructionCallback
 }  // namespace
 
 TEST_F(MarkingVisitorTest, DontMarkMemberInConstruction) {
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
   GCedWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithInConstructionCallback>(
           GetAllocationHandle(),
@@ -216,7 +222,7 @@ TEST_F(MarkingVisitorTest, DontMarkMemberInConstruction) {
 }
 
 TEST_F(MarkingVisitorTest, DontMarkMemberMixinInConstruction) {
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
   GCedWithMixinWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
           GetAllocationHandle(),
@@ -228,7 +234,7 @@ TEST_F(MarkingVisitorTest, DontMarkMemberMixinInConstruction) {
 }
 
 TEST_F(MarkingVisitorTest, DontMarkWeakMemberInConstruction) {
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
   GCedWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithInConstructionCallback>(
           GetAllocationHandle(),
@@ -240,7 +246,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakMemberInConstruction) {
 }
 
 TEST_F(MarkingVisitorTest, DontMarkWeakMemberMixinInConstruction) {
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
   GCedWithMixinWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
           GetAllocationHandle(),
@@ -252,7 +258,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakMemberMixinInConstruction) {
 }
 
 TEST_F(MarkingVisitorTest, DontMarkPersistentInConstruction) {
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
   GCedWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithInConstructionCallback>(
           GetAllocationHandle(),
@@ -264,7 +270,7 @@ TEST_F(MarkingVisitorTest, DontMarkPersistentInConstruction) {
 }
 
 TEST_F(MarkingVisitorTest, DontMarkPersistentMixinInConstruction) {
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
   GCedWithMixinWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
           GetAllocationHandle(),
@@ -276,7 +282,7 @@ TEST_F(MarkingVisitorTest, DontMarkPersistentMixinInConstruction) {
 }
 
 TEST_F(MarkingVisitorTest, DontMarkWeakPersistentInConstruction) {
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
   GCedWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithInConstructionCallback>(
           GetAllocationHandle(),
@@ -288,7 +294,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakPersistentInConstruction) {
 }
 
 TEST_F(MarkingVisitorTest, DontMarkWeakPersistentMixinInConstruction) {
-  MutatorThreadMarkingVisitor visitor(GetMarker());
+  TestMarkingVisitor visitor(GetMarker());
   GCedWithMixinWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
           GetAllocationHandle(),

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

@@ -45,10 +45,11 @@ class ExpectWriteBarrierFires final : private IncrementalMarkingScope {
   ExpectWriteBarrierFires(MarkerBase* marker,
                           std::initializer_list<void*> objects)
       : IncrementalMarkingScope(marker),
-        marking_worklist_(marker->marking_worklists().marking_worklist(),
-                          MarkingWorklists::kMutatorThreadId),
+        marking_worklist_(
+            marker->MarkingWorklistsForTesting().marking_worklist(),
+            MarkingWorklists::kMutatorThreadId),
         write_barrier_worklist_(
-            marker->marking_worklists().write_barrier_worklist(),
+            marker->MarkingWorklistsForTesting().write_barrier_worklist(),
             MarkingWorklists::kMutatorThreadId),
         objects_(objects) {
     EXPECT_TRUE(marking_worklist_.IsGlobalPoolEmpty());
@@ -96,10 +97,11 @@ class ExpectNoWriteBarrierFires final : private IncrementalMarkingScope {
   ExpectNoWriteBarrierFires(MarkerBase* marker,
                             std::initializer_list<void*> objects)
       : IncrementalMarkingScope(marker),
-        marking_worklist_(marker->marking_worklists().marking_worklist(),
-                          MarkingWorklists::kMutatorThreadId),
+        marking_worklist_(
+            marker->MarkingWorklistsForTesting().marking_worklist(),
+            MarkingWorklists::kMutatorThreadId),
         write_barrier_worklist_(
-            marker->marking_worklists().write_barrier_worklist(),
+            marker->MarkingWorklistsForTesting().write_barrier_worklist(),
             MarkingWorklists::kMutatorThreadId) {
     EXPECT_TRUE(marking_worklist_.IsGlobalPoolEmpty());
     EXPECT_TRUE(write_barrier_worklist_.IsGlobalPoolEmpty());