src: make CppgcMixin a MemoryRetainer

Author: joyeecheung

node.gyp

@@ -206,6 +206,7 @@
       'src/connect_wrap.h',
       'src/connection_wrap.h',
       'src/cppgc_helpers.h',
+      'src/cppgc_helpers.cc',
       'src/dataqueue/queue.h',
       'src/debug_utils.h',
       'src/debug_utils-inl.h',

src/base_object.cc

@@ -181,5 +181,4 @@ void BaseObjectList::MemoryInfo(node::MemoryTracker* tracker) const {
     if (bo->IsDoneInitializing()) tracker->Track(bo);
   }
 }
-
 }  // namespace node

src/cppgc_helpers-inl.h

@@ -0,0 +1,50 @@
+#include "cppgc_helpers.h"
+#include "env-inl.h"
+
+namespace node {
+
+template <typename T>
+void CppgcMixin::Wrap(T* ptr, Realm* realm, v8::Local<v8::Object> obj) {
+  CHECK_GE(obj->InternalFieldCount(), T::kInternalFieldCount);
+  ptr->realm_ = realm;
+  v8::Isolate* isolate = realm->isolate();
+  ptr->traced_reference_ = v8::TracedReference<v8::Object>(isolate, obj);
+  // Note that ptr must be of concrete type T in Wrap.
+  v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(isolate, obj, ptr);
+  // Keep the layout consistent with BaseObjects.
+  obj->SetAlignedPointerInInternalField(
+      kEmbedderType, realm->isolate_data()->embedder_id_for_cppgc());
+  obj->SetAlignedPointerInInternalField(kSlot, ptr);
+  realm->TrackCppgcWrapper(ptr);
+}
+
+template <typename T>
+void CppgcMixin::Wrap(T* ptr, Environment* env, v8::Local<v8::Object> obj) {
+  Wrap(ptr, env->principal_realm(), obj);
+}
+
+template <typename T>
+T* CppgcMixin::Unwrap(v8::Local<v8::Object> obj) {
+  // We are not using v8::Object::Unwrap currently because that requires
+  // access to isolate which the ASSIGN_OR_RETURN_UNWRAP macro that we'll shim
+  // with ASSIGN_OR_RETURN_UNWRAP_GC doesn't take, and we also want a
+  // signature consistent with BaseObject::Unwrap() to avoid churn. Since
+  // cppgc-managed objects share the same layout as BaseObjects, just unwrap
+  // from the pointer in the internal field, which should be valid as long as
+  // the object is still alive.
+  if (obj->InternalFieldCount() != T::kInternalFieldCount) {
+    return nullptr;
+  }
+  T* ptr = static_cast<T*>(obj->GetAlignedPointerFromInternalField(T::kSlot));
+  return ptr;
+}
+
+v8::Local<v8::Object> CppgcMixin::object() const {
+  return traced_reference_.Get(realm_->isolate());
+}
+
+Environment* CppgcMixin::env() const {
+  return realm_->env();
+}
+
+}  // namespace node

src/cppgc_helpers.cc

@@ -0,0 +1,18 @@
+#include "cppgc_helpers.h"
+#include "env-inl.h"
+
+namespace node {
+
+void CppgcWrapperList::Cleanup() {
+  for (auto handle : *this) {
+    handle->Clean();
+  }
+}
+
+void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const {
+  for (auto handle : *this) {
+    tracker->Track(handle);
+  }
+}
+
+}  // namespace node

src/cppgc_helpers.h

@@ -6,14 +6,18 @@
 #include <type_traits>  // std::remove_reference
 #include "cppgc/garbage-collected.h"
 #include "cppgc/name-provider.h"
-#include "env.h"
 #include "memory_tracker.h"
+#include "util.h"
 #include "v8-cppgc.h"
 #include "v8-sandbox.h"
 #include "v8.h"
 
 namespace node {
 
+class Environment;
+class Realm;
+class CppgcWrapperList;
+
 /**
  * This is a helper mixin with a BaseObject-like interface to help
  * implementing wrapper objects managed by V8's cppgc (Oilpan) library.
@@ -47,8 +51,7 @@ namespace node {
  *     // Do cleanup that relies on a living Environemnt.
  *   }
  */
-class CppgcMixin : public cppgc::GarbageCollectedMixin,
-                   public CppgcWrapperListNode {
+class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer {
  public:
   // To help various callbacks access wrapper objects with different memory
   // management, cppgc-managed objects share the same layout as BaseObjects.
@@ -58,71 +61,55 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin,
   // invoked from the child class constructor, per cppgc::GarbageCollectedMixin
   // rules.
   template <typename T>
-  static void Wrap(T* ptr, Environment* env, v8::Local<v8::Object> obj) {
-    CHECK_GE(obj->InternalFieldCount(), T::kInternalFieldCount);
-    ptr->env_ = env;
-    v8::Isolate* isolate = env->isolate();
-    ptr->traced_reference_ = v8::TracedReference<v8::Object>(isolate, obj);
-    v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(isolate, obj, ptr);
-    // Keep the layout consistent with BaseObjects.
-    obj->SetAlignedPointerInInternalField(
-        kEmbedderType, env->isolate_data()->embedder_id_for_cppgc());
-    obj->SetAlignedPointerInInternalField(kSlot, ptr);
-    env->cppgc_wrapper_list()->PushFront(ptr);
-  }
+  static inline void Wrap(T* ptr, Realm* realm, v8::Local<v8::Object> obj);
+  template <typename T>
+  static inline void Wrap(T* ptr, Environment* env, v8::Local<v8::Object> obj);
 
-  v8::Local<v8::Object> object() const {
-    return traced_reference_.Get(env_->isolate());
+  inline v8::Local<v8::Object> object() const;
+  inline Environment* env() const;
+  inline v8::Local<v8::Object> object(v8::Isolate* isolate) const {
+    return traced_reference_.Get(isolate);
   }
 
-  Environment* env() const { return env_; }
-
   template <typename T>
-  static T* Unwrap(v8::Local<v8::Object> obj) {
-    // We are not using v8::Object::Unwrap currently because that requires
-    // access to isolate which the ASSIGN_OR_RETURN_UNWRAP macro that we'll shim
-    // with ASSIGN_OR_RETURN_UNWRAP_GC doesn't take, and we also want a
-    // signature consistent with BaseObject::Unwrap() to avoid churn. Since
-    // cppgc-managed objects share the same layout as BaseObjects, just unwrap
-    // from the pointer in the internal field, which should be valid as long as
-    // the object is still alive.
-    if (obj->InternalFieldCount() != T::kInternalFieldCount) {
-      return nullptr;
-    }
-    T* ptr = static_cast<T*>(obj->GetAlignedPointerFromInternalField(T::kSlot));
-    return ptr;
-  }
+  static inline T* Unwrap(v8::Local<v8::Object> obj);
 
   // Subclasses are expected to invoke CppgcMixin::Trace() in their own Trace()
   // methods.
   void Trace(cppgc::Visitor* visitor) const override {
     visitor->Trace(traced_reference_);
   }
 
-  // This implements CppgcWrapperListNode::Clean and is run for all the
-  // remaining Cppgc wrappers tracked in the Environment during Environment
-  // shutdown. The destruction of the wrappers would happen later, when the
-  // final garbage collection is triggered when CppHeap is torn down as part of
-  // the Isolate teardown. If subclasses of CppgcMixin wish to perform cleanups
-  // that depend on the Environment during destruction, they should implment it
-  // in a CleanEnvResource() override, and then call this->Clean() from their
+  // TODO(joyeecheung): use ObjectSizeTrait;
+  inline size_t SelfSize() const override { return sizeof(*this); }
+  inline bool IsCppgcWrapper() const override { return true; }
+
+  // This is run for all the remaining Cppgc wrappers tracked in the Realm
+  // during Realm shutdown. The destruction of the wrappers would happen later,
+  // when the final garbage collection is triggered when CppHeap is torn down as
+  // part of the Isolate teardown. If subclasses of CppgcMixin wish to perform
+  // cleanups that depend on the Realm during destruction, they should implment
+  // it in a CleanEnvResource() override, and then call this->Clean() from their
   // destructor. Outside of CleanEnvResource(), subclasses should avoid calling
   // into JavaScript or perform any operation that can trigger garbage
   // collection during the destruction.
-  void Clean() override {
-    if (env_ == nullptr) return;
-    this->CleanEnvResource(env_);
-    env_ = nullptr;
+  void Clean() {
+    if (realm_ == nullptr) return;
+    this->CleanEnvResource(realm_);
+    realm_ = nullptr;
   }
 
   // The default implementation of CleanEnvResource() is a no-op. Subclasses
-  // should override it to perform cleanup that require a living Environment,
+  // should override it to perform cleanup that require a living Realm,
   // instead of doing these cleanups directly in the destructor.
-  virtual void CleanEnvResource(Environment* env) {}
+  virtual void CleanEnvResource(Realm* realm) {}
+
+  friend class CppgcWrapperList;
 
  private:
-  Environment* env_ = nullptr;
+  Realm* realm_ = nullptr;
   v8::TracedReference<v8::Object> traced_reference_;
+  ListNode<CppgcMixin> wrapper_list_node_;
 };
 
 // If the class doesn't have additional owned traceable data, use this macro to
@@ -137,6 +124,9 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin,
 #define SET_CPPGC_NAME(Klass)                                                  \
   inline const char* GetHumanReadableName() const final {                      \
     return "Node / " #Klass;                                                   \
+  }                                                                            \
+  inline const char* MemoryInfoName() const override {                         \
+    return #Klass;                                                             \
   }
 
 /**

src/env.cc

@@ -1302,8 +1302,6 @@ void Environment::RunCleanup() {
     CleanupHandles();
   }
 
-  for (CppgcWrapperListNode* handle : cppgc_wrapper_list_) handle->Clean();
-
   for (const int fd : unmanaged_fds_) {
     uv_fs_t close_req;
     uv_fs_close(nullptr, &close_req, fd, nullptr);

src/env.h

@@ -602,12 +602,6 @@ class Cleanable {
   friend class Environment;
 };
 
-class CppgcWrapperListNode {
- public:
-  virtual void Clean() = 0;
-  ListNode<CppgcWrapperListNode> wrapper_list_node;
-};
-
 /**
  * Environment is a per-isolate data structure that represents an execution
  * environment. Each environment has a principal realm. An environment can
@@ -903,18 +897,12 @@ class Environment final : public MemoryRetainer {
   typedef ListHead<HandleWrap, &HandleWrap::handle_wrap_queue_> HandleWrapQueue;
   typedef ListHead<ReqWrapBase, &ReqWrapBase::req_wrap_queue_> ReqWrapQueue;
   typedef ListHead<Cleanable, &Cleanable::cleanable_queue_> CleanableQueue;
-  typedef ListHead<CppgcWrapperListNode,
-                   &CppgcWrapperListNode::wrapper_list_node>
-      CppgcWrapperList;
 
   inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
   inline CleanableQueue* cleanable_queue() {
     return &cleanable_queue_;
   }
   inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }
-  inline CppgcWrapperList* cppgc_wrapper_list() {
-    return &cppgc_wrapper_list_;
-  }
 
   // https://w3c.github.io/hr-time/#dfn-time-origin
   inline uint64_t time_origin() {
@@ -1203,7 +1191,6 @@ class Environment final : public MemoryRetainer {
   CleanableQueue cleanable_queue_;
   HandleWrapQueue handle_wrap_queue_;
   ReqWrapQueue req_wrap_queue_;
-  CppgcWrapperList cppgc_wrapper_list_;
 
   int handle_cleanup_waiting_ = 0;
   int request_waiting_ = 0;