cppgc: Use object start bitmap to trace mixins

This CL removes the GetTraceDescriptor virtual call from garbage
collected mixins and replaces it with querying the object start
bitmap.

The CL also removes the mixin macros which are now no longer needed.

Bug: chromium:1056170
Change-Id: I27ed299f93025d09a3bb3f0d17b14bed3c200565
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2287508Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68787}

BUILD.gn

@@ -4251,6 +4251,7 @@ v8_source_set("cppgc_base") {
     "src/heap/cppgc/sweeper.cc",
     "src/heap/cppgc/sweeper.h",
     "src/heap/cppgc/task-handle.h",
+    "src/heap/cppgc/trace-trait.cc",
     "src/heap/cppgc/virtual-memory.cc",
     "src/heap/cppgc/virtual-memory.h",
     "src/heap/cppgc/visitor.cc",

include/cppgc/garbage-collected.h

@@ -104,16 +104,6 @@ class GarbageCollectedMixin : public internal::GarbageCollectedBase {
  public:
   using IsGarbageCollectedMixinTypeMarker = void;
 
-  // Sentinel used to mark not-fully-constructed mixins.
-  static constexpr void* kNotFullyConstructedObject = nullptr;
-
-  // Provide default implementation that indicate that the vtable is not yet
-  // set up properly. This is used to to get GCInfo objects for mixins so that
-  // these objects can be processed later on.
-  virtual TraceDescriptor GetTraceDescriptor() const {
-    return {kNotFullyConstructedObject, nullptr};
-  }
-
   /**
    * This Trace method must be overriden by objects inheriting from
    * GarbageCollectedMixin.
@@ -121,71 +111,6 @@ class GarbageCollectedMixin : public internal::GarbageCollectedBase {
   virtual void Trace(cppgc::Visitor*) const {}
 };
 
-/**
- * Macro defines all methods and markers needed for handling mixins. Must be
- * used on the type that is inheriting from GarbageCollected *and*
- * GarbageCollectedMixin.
- *
- * \code
- * class Mixin : public GarbageCollectedMixin {
- *  public:
- *   void Trace(cppgc::Visitor* visitor) const override {
- *     // Dispatch using visitor->Trace(...);
- *   }
- * };
- *
- * class Foo : public GarbageCollected<Foo>, public Mixin {
- *   USING_GARBAGE_COLLECTED_MIXIN();
- *  public:
- *   void Trace(cppgc::Visitor* visitor) const override {
- *     // Dispatch using visitor->Trace(...);
- *     Mixin::Trace(visitor);
- *   }
- * };
- * \endcode
- */
-#define USING_GARBAGE_COLLECTED_MIXIN()                                      \
- public:                                                                     \
-  /* Marker is used by clang to check for proper usages of the macro. */     \
-  typedef int HasUsingGarbageCollectedMixinMacro;                            \
-                                                                             \
-  TraceDescriptor GetTraceDescriptor() const override {                      \
-    static_assert(                                                           \
-        internal::IsSubclassOfTemplate<                                      \
-            std::remove_const_t<std::remove_pointer_t<decltype(this)>>,      \
-            cppgc::GarbageCollected>::value,                                 \
-        "Only garbage collected objects can have garbage collected mixins"); \
-    return {this, TraceTrait<std::remove_const_t<                            \
-                      std::remove_pointer_t<decltype(this)>>>::Trace};       \
-  }                                                                          \
-                                                                             \
- private:                                                                    \
-  static_assert(true, "Force semicolon.")
-
-/**
- * Merge two or more Mixins into one.
- *
- * \code
- * class A : public GarbageCollectedMixin {};
- * class B : public GarbageCollectedMixin {};
- * class C : public A, public B {
- *   MERGE_GARBAGE_COLLECTED_MIXINS();
- *  public:
- * };
- * \endcode
- */
-#define MERGE_GARBAGE_COLLECTED_MIXINS()                \
- public:                                                \
-  /* When using multiple mixins the methods become   */ \
-  /* ambiguous. Providing additional implementations */ \
-  /* disambiguate them again.                        */ \
-  TraceDescriptor GetTraceDescriptor() const override { \
-    return {kNotFullyConstructedObject, nullptr};       \
-  }                                                     \
-                                                        \
- private:                                               \
-  static_assert(true, "Force semicolon.")
-
 }  // namespace cppgc
 
 #endif  // INCLUDE_CPPGC_GARBAGE_COLLECTED_H_

include/cppgc/trace-trait.h

@@ -6,7 +6,9 @@
 #define INCLUDE_CPPGC_TRACE_TRAIT_H_
 
 #include <type_traits>
+
 #include "cppgc/type-traits.h"
+#include "v8config.h"  // NOLINT(build/include_directory)
 
 namespace cppgc {
 
@@ -47,6 +49,14 @@ struct TraceDescriptor {
   TraceCallback callback;
 };
 
+namespace internal {
+
+struct V8_EXPORT TraceTraitFromInnerAddressImpl {
+  static TraceDescriptor GetTraceDescriptor(const void* address);
+};
+
+}  // namespace internal
+
 /**
  * Trait specifying how the garbage collector processes an object of type T.
  *
@@ -91,7 +101,7 @@ struct TraceTraitImpl<T, false> {
 template <typename T>
 struct TraceTraitImpl<T, true> {
   static TraceDescriptor GetTraceDescriptor(const void* self) {
-    return static_cast<const T*>(self)->GetTraceDescriptor();
+    return internal::TraceTraitFromInnerAddressImpl::GetTraceDescriptor(self);
   }
 };
 

src/heap/cppgc/heap-page.cc

@@ -26,21 +26,6 @@ Address AlignAddress(Address address, size_t alignment) {
       RoundUp(reinterpret_cast<uintptr_t>(address), alignment));
 }
 
-const HeapObjectHeader* ObjectHeaderFromInnerAddressImpl(const BasePage* page,
-                                                         const void* address) {
-  if (page->is_large()) {
-    return LargePage::From(page)->ObjectHeader();
-  }
-  const PlatformAwareObjectStartBitmap& bitmap =
-      NormalPage::From(page)->object_start_bitmap();
-  const HeapObjectHeader* header =
-      bitmap.FindHeader(static_cast<ConstAddress>(address));
-  DCHECK_LT(address,
-            reinterpret_cast<ConstAddress>(header) +
-                header->GetSize<HeapObjectHeader::AccessMode::kAtomic>());
-  return header;
-}
-
 }  // namespace
 
 // static
@@ -83,19 +68,6 @@ ConstAddress BasePage::PayloadEnd() const {
   return const_cast<BasePage*>(this)->PayloadEnd();
 }
 
-HeapObjectHeader& BasePage::ObjectHeaderFromInnerAddress(void* address) const {
-  return const_cast<HeapObjectHeader&>(
-      ObjectHeaderFromInnerAddress(const_cast<const void*>(address)));
-}
-
-const HeapObjectHeader& BasePage::ObjectHeaderFromInnerAddress(
-    const void* address) const {
-  const HeapObjectHeader* header =
-      ObjectHeaderFromInnerAddressImpl(this, address);
-  DCHECK_NE(kFreeListGCInfoIndex, header->GetGCInfoIndex());
-  return *header;
-}
-
 HeapObjectHeader* BasePage::TryObjectHeaderFromInnerAddress(
     void* address) const {
   return const_cast<HeapObjectHeader*>(

src/heap/cppgc/heap-page.h

@@ -48,7 +48,11 @@ class V8_EXPORT_PRIVATE BasePage {
   ConstAddress PayloadEnd() const;
 
   // |address| must refer to real object.
+  template <
+      HeapObjectHeader::AccessMode = HeapObjectHeader::AccessMode::kNonAtomic>
   HeapObjectHeader& ObjectHeaderFromInnerAddress(void* address) const;
+  template <
+      HeapObjectHeader::AccessMode = HeapObjectHeader::AccessMode::kNonAtomic>
   const HeapObjectHeader& ObjectHeaderFromInnerAddress(
       const void* address) const;
 
@@ -217,6 +221,38 @@ const BasePage* BasePage::FromPayload(const void* payload) {
       kGuardPageSize);
 }
 
+template <HeapObjectHeader::AccessMode mode =
+              HeapObjectHeader::AccessMode::kNonAtomic>
+const HeapObjectHeader* ObjectHeaderFromInnerAddressImpl(const BasePage* page,
+                                                         const void* address) {
+  if (page->is_large()) {
+    return LargePage::From(page)->ObjectHeader();
+  }
+  const PlatformAwareObjectStartBitmap& bitmap =
+      NormalPage::From(page)->object_start_bitmap();
+  const HeapObjectHeader* header =
+      bitmap.FindHeader<mode>(static_cast<ConstAddress>(address));
+  DCHECK_LT(address,
+            reinterpret_cast<ConstAddress>(header) +
+                header->GetSize<HeapObjectHeader::AccessMode::kAtomic>());
+  return header;
+}
+
+template <HeapObjectHeader::AccessMode mode>
+HeapObjectHeader& BasePage::ObjectHeaderFromInnerAddress(void* address) const {
+  return const_cast<HeapObjectHeader&>(
+      ObjectHeaderFromInnerAddress<mode>(const_cast<const void*>(address)));
+}
+
+template <HeapObjectHeader::AccessMode mode>
+const HeapObjectHeader& BasePage::ObjectHeaderFromInnerAddress(
+    const void* address) const {
+  const HeapObjectHeader* header =
+      ObjectHeaderFromInnerAddressImpl<mode>(this, address);
+  DCHECK_NE(kFreeListGCInfoIndex, header->GetGCInfoIndex());
+  return *header;
+}
+
 }  // namespace internal
 }  // namespace cppgc
 

src/heap/cppgc/marking-state.h

@@ -71,13 +71,6 @@ MarkingState::MarkingState(
 
 void MarkingState::MarkAndPush(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;
-  }
   MarkAndPush(HeapObjectHeader::FromPayload(
                   const_cast<void*>(desc.base_object_payload)),
               desc);
@@ -128,9 +121,7 @@ void MarkingState::RegisterWeakReferenceIfNeeded(const void* object,
   // 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)
+  if (HeapObjectHeader::FromPayload(desc.base_object_payload)
           .IsMarked<HeapObjectHeader::AccessMode::kAtomic>())
     return;
   RegisterWeakCallback(weak_callback, parameter);
@@ -140,14 +131,13 @@ void MarkingState::InvokeWeakRootsCallbackIfNeeded(const void* object,
                                                    TraceDescriptor desc,
                                                    WeakCallback weak_callback,
                                                    const void* parameter) {
-  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 are only traced at the end of marking, we can execute
   // the callback instead of registering it.
+#if DEBUG
+  const HeapObjectHeader& header =
+      HeapObjectHeader::FromPayload(desc.base_object_payload);
+  DCHECK_IMPLIES(header.IsInConstruction(), header.IsMarked());
+#endif  // DEBUG
   weak_callback(LivenessBrokerFactory::Create(), parameter);
 }
 

src/heap/cppgc/trace-trait.cc

+// 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 "include/cppgc/trace-trait.h"
+
+#include "src/heap/cppgc/gc-info-table.h"
+#include "src/heap/cppgc/heap-page.h"
+
+namespace cppgc {
+namespace internal {
+
+TraceDescriptor TraceTraitFromInnerAddressImpl::GetTraceDescriptor(
+    const void* address) {
+  // address is guaranteed to be on a normal page because this is used only for
+  // mixins.
+  const HeapObjectHeader& header =
+      BasePage::FromPayload(address)
+          ->ObjectHeaderFromInnerAddress<HeapObjectHeader::AccessMode::kAtomic>(
+              address);
+  return {header.Payload(),
+          GlobalGCInfoTable::GCInfoFromIndex(
+              header.GetGCInfoIndex<HeapObjectHeader::AccessMode::kAtomic>())
+              .trace};
+}
+
+}  // namespace internal
+}  // namespace cppgc

test/benchmarks/cpp/cppgc/trace_perf.cc

@@ -32,8 +32,6 @@ class Mixin : public GarbageCollectedMixin {
 };
 
 class GCedWithMixin final : public GCed, public OtherPayload, public Mixin {
-  USING_GARBAGE_COLLECTED_MIXIN();
-
  public:
   void Trace(Visitor* visitor) const final {
     GCed::Trace(visitor);

test/unittests/heap/cppgc/garbage-collected-unittest.cc

@@ -22,13 +22,9 @@ class GCed : public GarbageCollected<GCed> {
 };
 class NotGCed {};
 class Mixin : public GarbageCollectedMixin {};
-class GCedWithMixin : public GarbageCollected<GCedWithMixin>, public Mixin {
-  USING_GARBAGE_COLLECTED_MIXIN();
-};
+class GCedWithMixin : public GarbageCollected<GCedWithMixin>, public Mixin {};
 class OtherMixin : public GarbageCollectedMixin {};
 class MergedMixins : public Mixin, public OtherMixin {
-  MERGE_GARBAGE_COLLECTED_MIXINS();
-
  public:
   void Trace(cppgc::Visitor* visitor) const override {
     Mixin::Trace(visitor);
@@ -36,8 +32,6 @@ class MergedMixins : public Mixin, public OtherMixin {
   }
 };
 class GCWithMergedMixins : public GCed, public MergedMixins {
-  USING_GARBAGE_COLLECTED_MIXIN();
-
  public:
   void Trace(cppgc::Visitor* visitor) const override {
     MergedMixins::Trace(visitor);
@@ -73,12 +67,11 @@ TEST_F(GarbageCollectedTestWithHeap, GetObjectStartReturnsCurrentAddress) {
   GCed* gced = MakeGarbageCollected<GCed>(GetAllocationHandle());
   GCedWithMixin* gced_with_mixin =
       MakeGarbageCollected<GCedWithMixin>(GetAllocationHandle());
-  EXPECT_EQ(gced_with_mixin, static_cast<Mixin*>(gced_with_mixin)
-                                 ->GetTraceDescriptor()
-                                 .base_object_payload);
-  EXPECT_NE(gced, static_cast<Mixin*>(gced_with_mixin)
-                      ->GetTraceDescriptor()
-                      .base_object_payload);
+  const void* base_object_payload = TraceTrait<Mixin>::GetTraceDescriptor(
+                                        static_cast<Mixin*>(gced_with_mixin))
+                                        .base_object_payload;
+  EXPECT_EQ(gced_with_mixin, base_object_payload);
+  EXPECT_NE(gced, base_object_payload);
 }
 
 namespace {

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

@@ -39,8 +39,6 @@ class GCed : public GarbageCollected<GCed> {
 
 class Mixin : public GarbageCollectedMixin {};
 class GCedWithMixin : public GarbageCollected<GCedWithMixin>, public Mixin {
-  USING_GARBAGE_COLLECTED_MIXIN();
-
  public:
   void Trace(cppgc::Visitor*) const override {}
 };
@@ -198,8 +196,6 @@ class MixinWithInConstructionCallback : public GarbageCollectedMixin {
 class GCedWithMixinWithInConstructionCallback
     : public GarbageCollected<GCedWithMixinWithInConstructionCallback>,
       public MixinWithInConstructionCallback {
-  USING_GARBAGE_COLLECTED_MIXIN();
-
  public:
   template <typename Callback>
   explicit GCedWithMixinWithInConstructionCallback(Callback callback)
@@ -281,29 +277,5 @@ TEST_F(MarkingVisitorTest, DontMarkPersistentMixinInConstruction) {
   EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
 }
 
-TEST_F(MarkingVisitorTest, DontMarkWeakPersistentInConstruction) {
-  TestMarkingVisitor visitor(GetMarker());
-  GCedWithInConstructionCallback* gced =
-      MakeGarbageCollected<GCedWithInConstructionCallback>(
-          GetAllocationHandle(),
-          [&visitor](GCedWithInConstructionCallback* obj) {
-            WeakPersistent<GCedWithInConstructionCallback> object(obj);
-            visitor.TraceRootForTesting(object, SourceLocation::Current());
-          });
-  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
-}
-
-TEST_F(MarkingVisitorTest, DontMarkWeakPersistentMixinInConstruction) {
-  TestMarkingVisitor visitor(GetMarker());
-  GCedWithMixinWithInConstructionCallback* gced =
-      MakeGarbageCollected<GCedWithMixinWithInConstructionCallback>(
-          GetAllocationHandle(),
-          [&visitor](MixinWithInConstructionCallback* obj) {
-            WeakPersistent<MixinWithInConstructionCallback> mixin(obj);
-            visitor.TraceRootForTesting(mixin, SourceLocation::Current());
-          });
-  EXPECT_FALSE(HeapObjectHeader::FromPayload(gced).IsMarked());
-}
-
 }  // namespace internal
 }  // namespace cppgc

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

@@ -67,9 +67,7 @@ class Mixin : public GarbageCollectedMixin {
 };
 size_t Mixin::prefinalizer_callcount = 0;
 
-class GCedWithMixin : public GarbageCollected<GCedWithMixin>, public Mixin {
-  USING_GARBAGE_COLLECTED_MIXIN();
-};
+class GCedWithMixin : public GarbageCollected<GCedWithMixin>, public Mixin {};
 
 }  // namespace
 
@@ -121,7 +119,6 @@ size_t InheritingMixin::prefinalizer_callcount = 0;
 
 class GCedWithMixins : public GarbageCollected<GCedWithMixins>,
                        public InheritingMixin {
-  USING_GARBAGE_COLLECTED_MIXIN();
   CPPGC_USING_PRE_FINALIZER(GCedWithMixins, PreFinalizer);
 
  public:

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

@@ -42,8 +42,6 @@ class OtherPayload {
 class GCedMixinApplication : public GCed,
                              public OtherPayload,
                              public GCedMixin {
-  USING_GARBAGE_COLLECTED_MIXIN();
-
  public:
   void Trace(cppgc::Visitor* visitor) const override {
     GCed::Trace(visitor);

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

@@ -270,8 +270,6 @@ class ClassWithVirtual {
 class Child : public GarbageCollected<Child>,
               public ClassWithVirtual,
               public Mixin {
-  USING_GARBAGE_COLLECTED_MIXIN();
-
  public:
   Child() : ClassWithVirtual(), Mixin() {}
   ~Child() = default;