Revert "cppgc: Rework prefinalizers"

This reverts commit cf25b3bc.

Reason for revert: https://crbug.com/1307471. TraceTrait must only be used during marking.

Original change's description:
> cppgc: Rework prefinalizers
>
> Move the check for whether an object is live or dead out of the
> prefinalizer trampoline. Moving it into the backend allows for
> inlining the check which avoids a call to the trampoline for live
> objects.
>
> On catapult benchmarks (e.g. cnn:2021, nytimes:2020), there's often
> ~2k finalizers registered. In order to avoid memory overhead in the
> range of a few KB, we store the fact whether the object points to the
> base object payload in the LSB of the pointer. For caged builds this
> is replaced with just storing the index into the cage for both object
> and base object payload.
>
> Locally saves around ~10% of atomic sweeping processing time which is
> in the order of .05ms.
>
> Bug: v8:12698
> Change-Id: I198205a6b1d57fc2df821ee4e73e53dc6f825ff5
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3497764
> Reviewed-by: Omer Katz <omerkatz@chromium.org>
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#79442}

Bug: v8:12698, chromium:1307471
Change-Id: I5c4e70d46cb99af66c77f0c013625b6af6c6eb8e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3535781
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79527}

include/cppgc/prefinalizer.h

@@ -14,9 +14,9 @@ namespace internal {
 
 class V8_EXPORT PrefinalizerRegistration final {
  public:
-  using Callback = void (*)(void*);
+  using Callback = bool (*)(const cppgc::LivenessBroker&, void*);
 
-  PrefinalizerRegistration(void*, const void*, Callback);
+  PrefinalizerRegistration(void*, Callback);
 
   void* operator new(size_t, void* location) = delete;
   void* operator new(size_t) = delete;
@@ -55,19 +55,19 @@ class V8_EXPORT PrefinalizerRegistration final {
  */
 #define CPPGC_USING_PRE_FINALIZER(Class, PreFinalizer)                         \
  public:                                                                       \
-  static void InvokePreFinalizer(void* object) {                             \
+  static bool InvokePreFinalizer(const cppgc::LivenessBroker& liveness_broker, \
+                                 void* object) {                               \
     static_assert(cppgc::IsGarbageCollectedOrMixinTypeV<Class>,                \
                   "Only garbage collected objects can have prefinalizers");    \
     Class* self = static_cast<Class*>(object);                                 \
+    if (liveness_broker.IsHeapObjectAlive(self)) return false;                 \
     self->PreFinalizer();                                                      \
+    return true;                                                               \
   }                                                                            \
                                                                                \
  private:                                                                      \
   CPPGC_NO_UNIQUE_ADDRESS cppgc::internal::PrefinalizerRegistration            \
-      prefinalizer_dummy_{this,                                              \
-                          cppgc::TraceTrait<Class>::GetTraceDescriptor(this) \
-                              .base_object_payload,                          \
-                          Class::InvokePreFinalizer};                        \
+      prefinalizer_dummy_{this, Class::InvokePreFinalizer};                    \
   static_assert(true, "Force semicolon.")
 
 }  // namespace cppgc

src/base/pointer-with-payload.h

@@ -43,7 +43,6 @@ class PointerWithPayload {
 
   explicit PointerWithPayload(PointerType* pointer)
       : pointer_with_payload_(reinterpret_cast<uintptr_t>(pointer)) {
-    DCHECK_EQ(reinterpret_cast<uintptr_t>(pointer) & kPayloadMask, 0);
     DCHECK_EQ(GetPointer(), pointer);
     DCHECK_EQ(GetPayload(), static_cast<PayloadType>(0));
   }
@@ -58,11 +57,6 @@ class PointerWithPayload {
     Update(pointer, payload);
   }
 
-  PointerWithPayload(const PointerWithPayload& other) V8_NOEXCEPT = default;
-
-  PointerWithPayload& operator=(const PointerWithPayload& other)
-      V8_NOEXCEPT = default;
-
   V8_INLINE PointerType* GetPointer() const {
     return reinterpret_cast<PointerType*>(pointer_with_payload_ & kPointerMask);
   }
@@ -77,18 +71,17 @@ class PointerWithPayload {
   V8_INLINE PointerType* operator->() const { return GetPointer(); }
 
   V8_INLINE void Update(PointerType* new_pointer, PayloadType new_payload) {
-    DCHECK_EQ(reinterpret_cast<uintptr_t>(new_pointer) & kPayloadMask, 0);
     pointer_with_payload_ = reinterpret_cast<uintptr_t>(new_pointer) |
                             static_cast<uintptr_t>(new_payload);
     DCHECK_EQ(GetPayload(), new_payload);
     DCHECK_EQ(GetPointer(), new_pointer);
   }
 
-  V8_INLINE void SetPointer(PointerType* new_pointer) {
-    DCHECK_EQ(reinterpret_cast<uintptr_t>(new_pointer) & kPayloadMask, 0);
-    pointer_with_payload_ = reinterpret_cast<uintptr_t>(new_pointer) |
+  V8_INLINE void SetPointer(PointerType* newptr) {
+    DCHECK_EQ(reinterpret_cast<uintptr_t>(newptr) & kPayloadMask, 0);
+    pointer_with_payload_ = reinterpret_cast<uintptr_t>(newptr) |
                             (pointer_with_payload_ & kPayloadMask);
-    DCHECK_EQ(GetPointer(), new_pointer);
+    DCHECK_EQ(GetPointer(), newptr);
   }
 
   V8_INLINE PayloadType GetPayload() const {
@@ -117,15 +110,6 @@ class PointerWithPayload {
   static constexpr uintptr_t kPointerMask = ~kPayloadMask;
 
   uintptr_t pointer_with_payload_ = 0;
-
-  friend bool operator==(const PointerWithPayload& p1,
-                         const PointerWithPayload& p2) {
-    return p1.pointer_with_payload_ == p2.pointer_with_payload_;
-  }
-  friend bool operator!=(const PointerWithPayload& p1,
-                         const PointerWithPayload& p2) {
-    return !(p1 == p2);
-  }
 };
 
 }  // namespace base

src/heap/cppgc/caged-heap.h

@@ -10,7 +10,6 @@
 
 #include "include/cppgc/platform.h"
 #include "src/base/bounded-page-allocator.h"
-#include "src/base/logging.h"
 #include "src/heap/cppgc/globals.h"
 #include "src/heap/cppgc/virtual-memory.h"
 
@@ -33,12 +32,6 @@ class CagedHeap final {
            (kCagedHeapReservationAlignment - 1);
   }
 
-  static void* AddressFromOffset(void* base, size_t offset) {
-    DCHECK_LT(offset, kCagedHeapReservationSize);
-    DCHECK_NOT_NULL(base);
-    return reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(base) + offset);
-  }
-
   static uintptr_t BaseFromAddress(const void* address) {
     return reinterpret_cast<uintptr_t>(address) &
            ~(kCagedHeapReservationAlignment - 1);

src/heap/cppgc/prefinalizer-handler.cc

@@ -8,8 +8,6 @@
 #include <memory>
 
 #include "src/base/platform/platform.h"
-#include "src/base/pointer-with-payload.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"
@@ -18,37 +16,15 @@
 namespace cppgc {
 namespace internal {
 
-PrefinalizerRegistration::PrefinalizerRegistration(
-    void* object, const void* base_object_payload, Callback callback) {
+PrefinalizerRegistration::PrefinalizerRegistration(void* object,
+                                                   Callback callback) {
   auto* page = BasePage::FromPayload(object);
   DCHECK(!page->space().is_compactable());
-  page->heap().prefinalizer_handler()->RegisterPrefinalizer(
-      PreFinalizer(object, base_object_payload, callback));
+  page->heap().prefinalizer_handler()->RegisterPrefinalizer({object, callback});
 }
 
 bool PreFinalizer::operator==(const PreFinalizer& other) const {
-  return
-#if defined(CPPGC_CAGED_HEAP)
-      (object_offset == other.object_offset)
-#else   // !defined(CPPGC_CAGED_HEAP)
-      (object_and_offset == other.object_and_offset)
-#endif  // !defined(CPPGC_CAGED_HEAP)
-      && (callback == other.callback);
-}
-
-PreFinalizer::PreFinalizer(void* object, const void* base_object_payload,
-                           Callback cb)
-    :
-#if defined(CPPGC_CAGED_HEAP)
-      object_offset(CagedHeap::OffsetFromAddress<uint32_t>(object)),
-      base_object_payload_offset(
-          CagedHeap::OffsetFromAddress<uint32_t>(base_object_payload)),
-#else   // !defined(CPPGC_CAGED_HEAP)
-      object_and_offset(object, (object == base_object_payload)
-                                    ? PointerType::kAtBase
-                                    : PointerType::kInnerPointer),
-#endif  // !defined(CPPGC_CAGED_HEAP)
-      callback(cb) {
+  return (object == other.object) && (callback == other.callback);
 }
 
 PreFinalizerHandler::PreFinalizerHandler(HeapBase& heap)
@@ -72,33 +48,6 @@ void PreFinalizerHandler::RegisterPrefinalizer(PreFinalizer pre_finalizer) {
   current_ordered_pre_finalizers_->push_back(pre_finalizer);
 }
 
-namespace {
-
-// Returns true in case the prefinalizer was invoked.
-V8_INLINE bool InvokeUnmarkedPrefinalizers(void* cage_base,
-                                           const PreFinalizer& pf) {
-#if defined(CPPGC_CAGED_HEAP)
-  void* object = CagedHeap::AddressFromOffset(cage_base, pf.object_offset);
-  void* base_object_payload =
-      CagedHeap::AddressFromOffset(cage_base, pf.base_object_payload_offset);
-#else   // !defined(CPPGC_CAGED_HEAP)
-  void* object = pf.object_and_offset.GetPointer();
-  void* base_object_payload =
-      pf.object_and_offset.GetPayload() == PreFinalizer::PointerType::kAtBase
-          ? object
-          : reinterpret_cast<void*>(BasePage::FromPayload(object)
-                                        ->ObjectHeaderFromInnerAddress(object)
-                                        .ObjectStart());
-#endif  // !defined(CPPGC_CAGED_HEAP)
-
-  if (HeapObjectHeader::FromObject(base_object_payload).IsMarked())
-    return false;
-  pf.callback(object);
-  return true;
-}
-
-}  // namespace
-
 void PreFinalizerHandler::InvokePreFinalizers() {
   StatsCollector::EnabledScope stats_scope(heap_.stats_collector(),
                                            StatsCollector::kAtomicSweep);
@@ -106,15 +55,11 @@ void PreFinalizerHandler::InvokePreFinalizers() {
       heap_.stats_collector(), StatsCollector::kSweepInvokePreFinalizers);
 
   DCHECK(CurrentThreadIsCreationThread());
+  LivenessBroker liveness_broker = LivenessBrokerFactory::Create();
   is_invoking_ = true;
   DCHECK_EQ(0u, bytes_allocated_in_prefinalizers);
   // Reset all LABs to force allocations to the slow path for black allocation.
   heap_.object_allocator().ResetLinearAllocationBuffers();
-
-  void* cage_base = nullptr;
-#if defined(CPPGC_CAGED_HEAP)
-  cage_base = heap_.caged_heap().base();
-#endif  // defined(CPPGC_CAGED_HEAP)
   // Prefinalizers can allocate other objects with prefinalizers, which will
   // modify ordered_pre_finalizers_ and break iterators.
   std::vector<PreFinalizer> new_ordered_pre_finalizers;
@@ -123,8 +68,8 @@ void PreFinalizerHandler::InvokePreFinalizers() {
       ordered_pre_finalizers_.begin(),
       std::remove_if(ordered_pre_finalizers_.rbegin(),
                      ordered_pre_finalizers_.rend(),
-                     [cage_base](const PreFinalizer& pf) {
-                       return InvokeUnmarkedPrefinalizers(cage_base, pf);
+                     [liveness_broker](const PreFinalizer& pf) {
+                       return (pf.callback)(liveness_broker, pf.object);
                      })
           .base());
   // Newly added objects with prefinalizers will always survive the current GC

src/heap/cppgc/prefinalizer-handler.h

@@ -9,7 +9,6 @@
 #include <vector>
 
 #include "include/cppgc/prefinalizer.h"
-#include "src/base/pointer-with-payload.h"
 
 namespace cppgc {
 namespace internal {
@@ -19,27 +18,7 @@ class HeapBase;
 struct PreFinalizer final {
   using Callback = PrefinalizerRegistration::Callback;
 
-  PreFinalizer(void* object, const void* base_object_payload,
-               Callback callback);
-
-#if defined(CPPGC_CAGED_HEAP)
-
-  uint32_t object_offset;
-  uint32_t base_object_payload_offset;
-
-#else  // !defined(CPPGC_CAGED_HEAP)
-
-  enum class PointerType : uint8_t {
-    kAtBase,
-    kInnerPointer,
-  };
-
-  // Contains the pointer and also an indicator of whether the pointer points to
-  // the base of the object or is an inner pointer.
-  v8::base::PointerWithPayload<void, PointerType, 1> object_and_offset;
-
-#endif  // !defined(CPPGC_CAGED_HEAP)
-
+  void* object;
   Callback callback;
 
   bool operator==(const PreFinalizer& other) const;