cppgc: Establish marking invariants

This is a revival of https://chromium-review.googlesource.com/c/v8/v8/+/2228332

The CL establishes the following:
*) Objects are marked before being pushed to the worklists.
*) Live bytes are always accounted after tracing an object (i.e. move
   from Gray to Black below).
*) Previously not fully constructed objects are traced immediately
   instead of pushed to the marking worklist.

This establishes the following invariants for all marking worklists:
1) White = !object.is_marked() && !worklist.contains(object)
2) Gray = object.is_marked() && worklist.contains(object)
3) Black = object.is_marked() && !worklist.contains(object)

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

src/heap/cppgc/marker.cc

@@ -98,6 +98,15 @@ bool DrainWorklistWithDeadline(v8::base::TimeTicks deadline, Worklist* worklist,
   return true;
 }
 
+void TraceMarkedObject(Visitor* visitor, const HeapObjectHeader* header) {
+  DCHECK(header);
+  DCHECK(!header->IsInConstruction<HeapObjectHeader::AccessMode::kNonAtomic>());
+  DCHECK(header->IsMarked<HeapObjectHeader::AccessMode::kNonAtomic>());
+  const GCInfo& gcinfo =
+      GlobalGCInfoTable::GCInfoFromIndex(header->GetGCInfoIndex());
+  gcinfo.trace(visitor, header->Payload());
+}
+
 }  // namespace
 
 MarkerBase::MarkerBase(HeapBase& heap)
@@ -115,15 +124,12 @@ MarkerBase::~MarkerBase() {
   if (!marking_worklists_.not_fully_constructed_worklist()->IsEmpty()) {
 #if DEBUG
     DCHECK_NE(MarkingConfig::StackState::kNoHeapPointers, config_.stack_state);
-    MarkingWorklists::NotFullyConstructedItem item;
+    HeapObjectHeader* header;
     MarkingWorklists::NotFullyConstructedWorklist::View view(
         marking_worklists_.not_fully_constructed_worklist(),
         MarkingWorklists::kMutatorThreadId);
-    while (view.Pop(&item)) {
-      const HeapObjectHeader& header =
-          BasePage::FromPayload(item)->ObjectHeaderFromInnerAddress(
-              static_cast<ConstAddress>(item));
-      DCHECK(header.IsMarked());
+    while (view.Pop(&header)) {
+      DCHECK(header->IsMarked());
     }
 #else
     marking_worklists_.not_fully_constructed_worklist()->Clear();
@@ -204,9 +210,9 @@ bool MarkerBase::AdvanceMarkingWithDeadline(v8::base::TimeDelta duration) {
     if (!DrainWorklistWithDeadline(
             deadline,
             marking_worklists_.previously_not_fully_constructed_worklist(),
-            [this](MarkingWorklists::NotFullyConstructedItem& item) {
-              mutator_marking_state_.DynamicallyMarkAddress(
-                  reinterpret_cast<ConstAddress>(item));
+            [this](HeapObjectHeader* header) {
+              TraceMarkedObject(&visitor(), header);
+              mutator_marking_state_.AccountMarkedBytes(*header);
             },
             MarkingWorklists::kMutatorThreadId))
       return false;
@@ -218,6 +224,8 @@ bool MarkerBase::AdvanceMarkingWithDeadline(v8::base::TimeDelta duration) {
                   HeapObjectHeader::FromPayload(item.base_object_payload);
               DCHECK(!header.IsInConstruction<
                       HeapObjectHeader::AccessMode::kNonAtomic>());
+              DCHECK(
+                  header.IsMarked<HeapObjectHeader::AccessMode::kNonAtomic>());
               item.callback(&visitor(), item.base_object_payload);
               mutator_marking_state_.AccountMarkedBytes(header);
             },
@@ -227,12 +235,7 @@ bool MarkerBase::AdvanceMarkingWithDeadline(v8::base::TimeDelta duration) {
     if (!DrainWorklistWithDeadline(
             deadline, marking_worklists_.write_barrier_worklist(),
             [this](HeapObjectHeader* header) {
-              DCHECK(header);
-              DCHECK(!header->IsInConstruction<
-                      HeapObjectHeader::AccessMode::kNonAtomic>());
-              const GCInfo& gcinfo =
-                  GlobalGCInfoTable::GCInfoFromIndex(header->GetGCInfoIndex());
-              gcinfo.trace(&visitor(), header->Payload());
+              TraceMarkedObject(&visitor(), header);
               mutator_marking_state_.AccountMarkedBytes(*header);
             },
             MarkingWorklists::kMutatorThreadId))
@@ -244,12 +247,16 @@ bool MarkerBase::AdvanceMarkingWithDeadline(v8::base::TimeDelta duration) {
 }
 
 void MarkerBase::MarkNotFullyConstructedObjects() {
-  MarkingWorklists::NotFullyConstructedItem item;
+  HeapObjectHeader* header;
   MarkingWorklists::NotFullyConstructedWorklist::View view(
       marking_worklists_.not_fully_constructed_worklist(),
       MarkingWorklists::kMutatorThreadId);
-  while (view.Pop(&item)) {
-    conservative_visitor().TraceConservativelyIfNeeded(item);
+  while (view.Pop(&header)) {
+    DCHECK(header);
+    DCHECK(header->IsMarked<HeapObjectHeader::AccessMode::kNonAtomic>());
+    // TraceConservativelyIfNeeded will either push to a worklist
+    // or trace conservatively and call AccountMarkedBytes.
+    conservative_visitor().TraceConservativelyIfNeeded(*header);
   }
 }
 

src/heap/cppgc/marker.h

@@ -82,7 +82,7 @@ class V8_EXPORT_PRIVATE MarkerBase {
 
   void ProcessWeakness();
 
-  inline void WriteBarrierForInConstructionObject(ConstAddress);
+  inline void WriteBarrierForInConstructionObject(HeapObjectHeader&);
   inline void WriteBarrierForObject(HeapObjectHeader&);
 
   HeapBase& heap() { return heap_; }
@@ -128,12 +128,12 @@ class V8_EXPORT_PRIVATE Marker final : public MarkerBase {
   ConservativeMarkingVisitor conservative_marking_visitor_;
 };
 
-void MarkerBase::WriteBarrierForInConstructionObject(ConstAddress payload) {
+void MarkerBase::WriteBarrierForInConstructionObject(HeapObjectHeader& header) {
   MarkingWorklists::NotFullyConstructedWorklist::View
       not_fully_constructed_worklist(
           marking_worklists_.not_fully_constructed_worklist(),
           MarkingWorklists::kMutatorThreadId);
-  not_fully_constructed_worklist.Push(payload);
+  not_fully_constructed_worklist.Push(&header);
 }
 
 void MarkerBase::WriteBarrierForObject(HeapObjectHeader& header) {

src/heap/cppgc/marking-state.h

@@ -81,9 +81,11 @@ void MarkingState::MarkAndPush(const void* object, TraceDescriptor desc) {
 void MarkingState::MarkAndPush(HeapObjectHeader& header, TraceDescriptor desc) {
   DCHECK_NOT_NULL(desc.callback);
 
+  if (!MarkNoPush(header)) return;
+
   if (header.IsInConstruction<HeapObjectHeader::AccessMode::kNonAtomic>()) {
-    not_fully_constructed_worklist_.Push(header.Payload());
-  } else if (MarkNoPush(header)) {
+    not_fully_constructed_worklist_.Push(&header);
+  } else {
     marking_worklist_.Push(desc);
   }
 }

src/heap/cppgc/marking-worklists.h

@@ -21,7 +21,6 @@ class MarkingWorklists {
   static constexpr int kMutatorThreadId = 0;
 
   using MarkingItem = cppgc::TraceDescriptor;
-  using NotFullyConstructedItem = const void*;
   struct WeakCallbackItem {
     cppgc::WeakCallback callback;
     const void* parameter;
@@ -32,7 +31,7 @@ class MarkingWorklists {
   using MarkingWorklist =
       Worklist<MarkingItem, 512 /* local entries */, kNumMarkers>;
   using NotFullyConstructedWorklist =
-      Worklist<NotFullyConstructedItem, 16 /* local entries */, kNumMarkers>;
+      Worklist<HeapObjectHeader*, 16 /* local entries */, kNumMarkers>;
   using WeakCallbackWorklist =
       Worklist<WeakCallbackItem, 64 /* local entries */, kNumMarkers>;
   using WriteBarrierWorklist =

src/heap/cppgc/visitor.cc

@@ -63,13 +63,18 @@ void ConservativeTracingVisitor::TraceConservativelyIfNeeded(
 
   if (!header) return;
 
-  if (!header->IsInConstruction<HeapObjectHeader::AccessMode::kNonAtomic>()) {
+  TraceConservativelyIfNeeded(*header);
+}
+
+void ConservativeTracingVisitor::TraceConservativelyIfNeeded(
+    HeapObjectHeader& header) {
+  if (!header.IsInConstruction<HeapObjectHeader::AccessMode::kNonAtomic>()) {
     visitor_.Visit(
-        header->Payload(),
-        {header->Payload(),
-         GlobalGCInfoTable::GCInfoFromIndex(header->GetGCInfoIndex()).trace});
+        header.Payload(),
+        {header.Payload(),
+         GlobalGCInfoTable::GCInfoFromIndex(header.GetGCInfoIndex()).trace});
   } else {
-    VisitConservatively(*header, TraceConservatively);
+    VisitConservatively(header, TraceConservatively);
   }
 }
 

src/heap/cppgc/visitor.h

@@ -54,6 +54,7 @@ class ConservativeTracingVisitor {
       delete;
 
   void TraceConservativelyIfNeeded(const void*);
+  void TraceConservativelyIfNeeded(HeapObjectHeader&);
 
  protected:
   using TraceConservativelyCallback = void(ConservativeTracingVisitor*,

src/heap/cppgc/write-barrier.cc

@@ -37,10 +37,7 @@ void MarkValue(const BasePage* page, MarkerBase* marker, const void* value) {
   if (V8_UNLIKELY(
           header
               .IsInConstruction<HeapObjectHeader::AccessMode::kNonAtomic>())) {
-    // It is assumed that objects on not_fully_constructed_worklist_ are not
-    // marked.
-    header.Unmark();
-    marker->WriteBarrierForInConstructionObject(header.Payload());
+    marker->WriteBarrierForInConstructionObject(header);
     return;
   }
 

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

@@ -224,7 +224,7 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedEmptyStack) {
         Member<GCedWithCallback> member(obj);
         marker.VisitorForTesting().Trace(member);
       });
-  EXPECT_FALSE(HeapObjectHeader::FromPayload(object).IsMarked());
+  EXPECT_TRUE(HeapObjectHeader::FromPayload(object).IsMarked());
   marker.FinishMarking({MarkingConfig::CollectionType::kMajor,
                         MarkingConfig::StackState::kMayContainHeapPointers});
   EXPECT_TRUE(HeapObjectHeader::FromPayload(object).IsMarked());
@@ -240,7 +240,7 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedNonEmptyStack) {
       GetAllocationHandle(), [&marker](GCedWithCallback* obj) {
         Member<GCedWithCallback> member(obj);
         marker.VisitorForTesting().Trace(member);
-        EXPECT_FALSE(HeapObjectHeader::FromPayload(obj).IsMarked());
+        EXPECT_TRUE(HeapObjectHeader::FromPayload(obj).IsMarked());
         marker.FinishMarking(config);
         EXPECT_TRUE(HeapObjectHeader::FromPayload(obj).IsMarked());
       });

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

@@ -205,7 +205,7 @@ class GCedWithMixinWithInConstructionCallback
 
 }  // namespace
 
-TEST_F(MarkingVisitorTest, DontMarkMemberInConstruction) {
+TEST_F(MarkingVisitorTest, MarkMemberInConstruction) {
   TestMarkingVisitor visitor(GetMarker());
   GCedWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithInConstructionCallback>(
@@ -214,10 +214,10 @@ TEST_F(MarkingVisitorTest, DontMarkMemberInConstruction) {
             Member<GCedWithInConstructionCallback> object(obj);
             visitor.Trace(object);
           });
-  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+  EXPECT_TRUE(HeapObjectHeader::FromPayload(gced).IsMarked());
 }
 
-TEST_F(MarkingVisitorTest, DontMarkMemberMixinInConstruction) {
+TEST_F(MarkingVisitorTest, MarkMemberMixinInConstruction) {
   TestMarkingVisitor visitor(GetMarker());
   GCedWithMixinWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
@@ -226,7 +226,7 @@ TEST_F(MarkingVisitorTest, DontMarkMemberMixinInConstruction) {
             Member<MixinWithInConstructionCallback> mixin(obj);
             visitor.Trace(mixin);
           });
-  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+  EXPECT_TRUE(HeapObjectHeader::FromPayload(gced).IsMarked());
 }
 
 TEST_F(MarkingVisitorTest, DontMarkWeakMemberInConstruction) {
@@ -253,7 +253,7 @@ TEST_F(MarkingVisitorTest, DontMarkWeakMemberMixinInConstruction) {
   EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
 }
 
-TEST_F(MarkingVisitorTest, DontMarkPersistentInConstruction) {
+TEST_F(MarkingVisitorTest, MarkPersistentInConstruction) {
   TestMarkingVisitor visitor(GetMarker());
   GCedWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithInConstructionCallback>(
@@ -262,10 +262,10 @@ TEST_F(MarkingVisitorTest, DontMarkPersistentInConstruction) {
             Persistent<GCedWithInConstructionCallback> object(obj);
             visitor.TraceRootForTesting(object, SourceLocation::Current());
           });
-  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+  EXPECT_TRUE(HeapObjectHeader::FromPayload(gced).IsMarked());
 }
 
-TEST_F(MarkingVisitorTest, DontMarkPersistentMixinInConstruction) {
+TEST_F(MarkingVisitorTest, MarkPersistentMixinInConstruction) {
   TestMarkingVisitor visitor(GetMarker());
   GCedWithMixinWithInConstructionCallback* gced =
       MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
@@ -274,7 +274,7 @@ TEST_F(MarkingVisitorTest, DontMarkPersistentMixinInConstruction) {
             Persistent<MixinWithInConstructionCallback> mixin(obj);
             visitor.TraceRootForTesting(mixin, SourceLocation::Current());
           });
-  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
+  EXPECT_TRUE(HeapObjectHeader::FromPayload(gced).IsMarked());
 }
 
 }  // namespace internal