cppgc: Rework pre-finalizer registration

Since the registration requires calling into the library, there's no
reason to get the heap through a magic getter on API level.

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

BUILD.gn

@@ -4099,7 +4099,6 @@ v8_source_set("cppgc_base") {
     "include/cppgc/custom-space.h",
     "include/cppgc/garbage-collected.h",
     "include/cppgc/heap.h",
-    "include/cppgc/internal/accessors.h",
     "include/cppgc/internal/api-contants.h",
     "include/cppgc/internal/atomic-entry-flag.h",
     "include/cppgc/internal/compiler-specific.h",
@@ -4136,6 +4135,7 @@ v8_source_set("cppgc_base") {
     "src/heap/cppgc/heap-object-header-inl.h",
     "src/heap/cppgc/heap-object-header.cc",
     "src/heap/cppgc/heap-object-header.h",
+    "src/heap/cppgc/heap-page-inl.h",
     "src/heap/cppgc/heap-page.cc",
     "src/heap/cppgc/heap-page.h",
     "src/heap/cppgc/heap-space.cc",

include/cppgc/internal/api-constants.h

@@ -25,14 +25,7 @@ static constexpr size_t kFullyConstructedBitFieldOffsetFromPayload =
 // Mask for in-construction bit.
 static constexpr size_t kFullyConstructedBitMask = size_t{1};
 
-// Page constants used to align pointers to page begin.
 static constexpr size_t kPageSize = size_t{1} << 17;
-static constexpr size_t kPageAlignment = kPageSize;
-static constexpr size_t kPageBaseMask = ~(kPageAlignment - 1);
-static constexpr size_t kGuardPageSize = 4096;
-
-// Offset of the Heap backref.
-static constexpr size_t kHeapOffset = 0;
 
 static constexpr size_t kLargeObjectSizeThreshold = kPageSize / 2;
 

include/cppgc/internal/prefinalizer-handler.h

@@ -15,14 +15,13 @@ class V8_EXPORT PreFinalizerRegistrationDispatcher final {
  public:
   using PreFinalizerCallback = bool (*)(const LivenessBroker&, void*);
   struct PreFinalizer {
-    void* object_;
-    PreFinalizerCallback callback_;
+    void* object;
+    PreFinalizerCallback callback;
 
     bool operator==(const PreFinalizer& other);
   };
 
-  static void RegisterPrefinalizer(cppgc::Heap* heap,
-                                   PreFinalizer prefinalzier);
+  static void RegisterPrefinalizer(PreFinalizer pre_finalizer);
 };
 
 }  // namespace internal

include/cppgc/prefinalizer.h

@@ -5,7 +5,6 @@
 #ifndef INCLUDE_CPPGC_PREFINALIZER_H_
 #define INCLUDE_CPPGC_PREFINALIZER_H_
 
-#include "cppgc/internal/accessors.h"
 #include "cppgc/internal/compiler-specific.h"
 #include "cppgc/internal/prefinalizer-handler.h"
 #include "cppgc/liveness-broker.h"
@@ -22,7 +21,7 @@ class PrefinalizerRegistration final {
                   "USING_PRE_FINALIZER(T) must be defined.");
 
     cppgc::internal::PreFinalizerRegistrationDispatcher::RegisterPrefinalizer(
-        internal::GetHeapFromPayload(self), {self, T::InvokePreFinalizer});
+        {self, T::InvokePreFinalizer});
   }
 
   void* operator new(size_t, void* location) = delete;

include/cppgc/internal/accessors.h → src/heap/cppgc/heap-page-inl.h

@@ -2,25 +2,29 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef INCLUDE_CPPGC_INTERNAL_ACCESSORS_H_
-#define INCLUDE_CPPGC_INTERNAL_ACCESSORS_H_
+#ifndef V8_HEAP_CPPGC_HEAP_PAGE_INL_H_
+#define V8_HEAP_CPPGC_HEAP_PAGE_INL_H_
 
-#include "cppgc/internal/api-constants.h"
+#include "src/heap/cppgc/heap-page.h"
 
 namespace cppgc {
-
-class Heap;
-
 namespace internal {
 
-inline cppgc::Heap* GetHeapFromPayload(const void* payload) {
-  return *reinterpret_cast<cppgc::Heap**>(
-      ((reinterpret_cast<uintptr_t>(payload) & api_constants::kPageBaseMask) +
-       api_constants::kGuardPageSize) +
-      api_constants::kHeapOffset);
+// static
+BasePage* BasePage::FromPayload(void* payload) {
+  return reinterpret_cast<BasePage*>(
+      (reinterpret_cast<uintptr_t>(payload) & kPageBaseMask) + kGuardPageSize);
+}
+
+// static
+const BasePage* BasePage::FromPayload(const void* payload) {
+  return reinterpret_cast<const BasePage*>(
+      (reinterpret_cast<uintptr_t>(const_cast<void*>(payload)) &
+       kPageBaseMask) +
+      kGuardPageSize);
 }
 
 }  // namespace internal
 }  // namespace cppgc
 
-#endif  // INCLUDE_CPPGC_INTERNAL_ACCESSORS_H_
+#endif  // V8_HEAP_CPPGC_HEAP_PAGE_INL_H_

src/heap/cppgc/heap-page.cc

@@ -44,22 +44,6 @@ const HeapObjectHeader* ObjectHeaderFromInnerAddressImpl(const BasePage* page,
 
 }  // namespace
 
-STATIC_ASSERT(kPageSize == api_constants::kPageAlignment);
-
-// static
-BasePage* BasePage::FromPayload(void* payload) {
-  return reinterpret_cast<BasePage*>(
-      (reinterpret_cast<uintptr_t>(payload) & kPageBaseMask) + kGuardPageSize);
-}
-
-// static
-const BasePage* BasePage::FromPayload(const void* payload) {
-  return reinterpret_cast<const BasePage*>(
-      (reinterpret_cast<uintptr_t>(const_cast<void*>(payload)) &
-       kPageBaseMask) +
-      kGuardPageSize);
-}
-
 // static
 BasePage* BasePage::FromInnerAddress(const Heap* heap, void* address) {
   return const_cast<BasePage*>(
@@ -129,8 +113,6 @@ BasePage::BasePage(Heap* heap, BaseSpace* space, PageType type)
     : heap_(heap), space_(space), type_(type) {
   DCHECK_EQ(0u, (reinterpret_cast<uintptr_t>(this) - kGuardPageSize) &
                     kPageOffsetMask);
-  DCHECK_EQ(reinterpret_cast<void*>(&heap_),
-            FromPayload(this) + api_constants::kHeapOffset);
   DCHECK_EQ(&heap_->raw_heap(), space_->raw_heap());
 }
 

src/heap/cppgc/heap-page.h

@@ -22,8 +22,8 @@ class PageBackend;
 
 class V8_EXPORT_PRIVATE BasePage {
  public:
-  static BasePage* FromPayload(void*);
-  static const BasePage* FromPayload(const void*);
+  static inline BasePage* FromPayload(void*);
+  static inline const BasePage* FromPayload(const void*);
 
   static BasePage* FromInnerAddress(const Heap*, void*);
   static const BasePage* FromInnerAddress(const Heap*, const void*);

src/heap/cppgc/heap-space.cc

@@ -8,7 +8,7 @@
 
 #include "src/base/logging.h"
 #include "src/base/platform/mutex.h"
-#include "src/heap/cppgc/heap-page.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/object-start-bitmap-inl.h"
 
 namespace cppgc {

src/heap/cppgc/heap.cc

@@ -12,7 +12,7 @@
 #include "src/heap/cppgc/gc-invoker.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
 #include "src/heap/cppgc/heap-object-header.h"
-#include "src/heap/cppgc/heap-page.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/heap-visitor.h"
 #include "src/heap/cppgc/page-memory.h"
 #include "src/heap/cppgc/stack.h"

src/heap/cppgc/marker.cc

@@ -6,6 +6,7 @@
 
 #include "include/cppgc/internal/process-heap.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/heap-visitor.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/marking-visitor.h"

src/heap/cppgc/marking-visitor.cc

@@ -5,9 +5,8 @@
 #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-page.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/page-memory-inl.h"
 #include "src/heap/cppgc/sanitizers.h"

src/heap/cppgc/object-allocator-inl.h

@@ -10,7 +10,7 @@
 #include "src/base/logging.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
 #include "src/heap/cppgc/heap-object-header.h"
-#include "src/heap/cppgc/heap-page.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/object-allocator.h"
 #include "src/heap/cppgc/object-start-bitmap-inl.h"
 #include "src/heap/cppgc/object-start-bitmap.h"

src/heap/cppgc/pointer-policies.cc

@@ -3,10 +3,10 @@
 // found in the LICENSE file.
 
 #include "include/cppgc/internal/pointer-policies.h"
-#include "include/cppgc/internal/persistent-node.h"
 
+#include "include/cppgc/internal/persistent-node.h"
 #include "src/base/macros.h"
-#include "src/heap/cppgc/heap-page.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/heap.h"
 
 namespace cppgc {

src/heap/cppgc/prefinalizer-handler.cc

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "src/base/platform/platform.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/heap.h"
 
 namespace cppgc {
@@ -15,14 +16,16 @@ namespace internal {
 
 // static
 void PreFinalizerRegistrationDispatcher::RegisterPrefinalizer(
-    cppgc::Heap* heap, PreFinalizer prefinalzier) {
-  internal::Heap::From(heap)->prefinalizer_handler()->RegisterPrefinalizer(
-      prefinalzier);
+    PreFinalizer pre_finalizer) {
+  BasePage::FromPayload(pre_finalizer.object)
+      ->heap()
+      ->prefinalizer_handler()
+      ->RegisterPrefinalizer(pre_finalizer);
 }
 
 bool PreFinalizerRegistrationDispatcher::PreFinalizer::operator==(
     const PreFinalizer& other) {
-  return (object_ == other.object_) && (callback_ == other.callback_);
+  return (object == other.object) && (callback == other.callback);
 }
 
 PreFinalizerHandler::PreFinalizerHandler()
@@ -32,12 +35,12 @@ PreFinalizerHandler::PreFinalizerHandler()
 {
 }
 
-void PreFinalizerHandler::RegisterPrefinalizer(PreFinalizer prefinalizer) {
+void PreFinalizerHandler::RegisterPrefinalizer(PreFinalizer pre_finalizer) {
   DCHECK(CurrentThreadIsCreationThread());
   DCHECK_EQ(ordered_pre_finalizers_.end(),
             std::find(ordered_pre_finalizers_.begin(),
-                      ordered_pre_finalizers_.end(), prefinalizer));
-  ordered_pre_finalizers_.push_back(prefinalizer);
+                      ordered_pre_finalizers_.end(), pre_finalizer));
+  ordered_pre_finalizers_.push_back(pre_finalizer);
 }
 
 void PreFinalizerHandler::InvokePreFinalizers() {
@@ -48,7 +51,7 @@ void PreFinalizerHandler::InvokePreFinalizers() {
       std::remove_if(ordered_pre_finalizers_.rbegin(),
                      ordered_pre_finalizers_.rend(),
                      [liveness_broker](const PreFinalizer& pf) {
-                       return (pf.callback_)(liveness_broker, pf.object_);
+                       return (pf.callback)(liveness_broker, pf.object);
                      })
           .base());
   ordered_pre_finalizers_.shrink_to_fit();

src/heap/cppgc/prefinalizer-handler.h

@@ -19,7 +19,7 @@ class PreFinalizerHandler final {
 
   PreFinalizerHandler();
 
-  void RegisterPrefinalizer(PreFinalizer prefinalzier);
+  void RegisterPrefinalizer(PreFinalizer pre_finalizer);
 
   void InvokePreFinalizers();
 

src/heap/cppgc/write-barrier.cc

@@ -7,7 +7,7 @@
 #include "include/cppgc/internal/pointer-policies.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
 #include "src/heap/cppgc/heap-object-header.h"
-#include "src/heap/cppgc/heap-page.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/marker.h"
 #include "src/heap/cppgc/marking-visitor.h"

test/unittests/heap/cppgc/concurrent-sweeper-unittest.cc

@@ -11,7 +11,7 @@
 #include "include/v8-platform.h"
 #include "src/heap/cppgc/globals.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
-#include "src/heap/cppgc/heap-page.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/heap-space.h"
 #include "src/heap/cppgc/heap-visitor.h"
 #include "src/heap/cppgc/page-memory-inl.h"

test/unittests/heap/cppgc/custom-spaces-unittest.cc

@@ -4,7 +4,7 @@
 
 #include "include/cppgc/allocation.h"
 #include "include/cppgc/custom-space.h"
-#include "src/heap/cppgc/heap-page.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/raw-heap.h"
 #include "test/unittests/heap/cppgc/tests.h"
 

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

@@ -7,12 +7,12 @@
 #include <algorithm>
 
 #include "include/cppgc/allocation.h"
-#include "include/cppgc/internal/accessors.h"
 #include "include/cppgc/persistent.h"
 #include "src/base/macros.h"
 #include "src/heap/cppgc/globals.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
 #include "src/heap/cppgc/heap-object-header.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/page-memory-inl.h"
 #include "src/heap/cppgc/page-memory.h"
 #include "src/heap/cppgc/raw-heap.h"
@@ -41,11 +41,6 @@ class GCed : public GarbageCollected<GCed<Size>> {
 
 }  // namespace
 
-TEST_F(PageTest, GetHeapForAllocatedObject) {
-  auto* gced = MakeGarbageCollected<GCed<1>>(GetAllocationHandle());
-  EXPECT_EQ(GetHeap(), GetHeapFromPayload(gced));
-}
-
 TEST_F(PageTest, SpaceIndexing) {
   RawHeap& heap = GetRawHeap();
   size_t space = 0u;

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

@@ -11,7 +11,7 @@
 #include "src/heap/cppgc/globals.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
 #include "src/heap/cppgc/heap-object-header.h"
-#include "src/heap/cppgc/heap-page.h"
+#include "src/heap/cppgc/heap-page-inl.h"
 #include "src/heap/cppgc/heap-visitor.h"
 #include "src/heap/cppgc/heap.h"
 #include "src/heap/cppgc/page-memory-inl.h"