src: track BaseObjects with an efficient list

PR-URL: nodejs#55104
Refs: nodejs#54880
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

src/README.md

@@ -760,10 +760,19 @@ a `void* hint` argument.
 Inside these cleanup hooks, new asynchronous operations _may_ be started on the
 event loop, although ideally that is avoided as much as possible.
 
-Every [`BaseObject`][] has its own cleanup hook that deletes it. For
-[`ReqWrap`][] and [`HandleWrap`][] instances, cleanup of the associated libuv
-objects is performed automatically, i.e. handles are closed and requests
-are cancelled if possible.
+For every [`ReqWrap`][] and [`HandleWrap`][] instance, the cleanup of the
+associated libuv objects is performed automatically, i.e. handles are closed
+and requests are cancelled if possible.
+
+#### Cleanup realms and BaseObjects
+
+Realm cleanup depends on the realm types. All realms are destroyed when the
+[`Environment`][] is destroyed with the cleanup hook. A [`ShadowRealm`][] can
+also be destroyed by the garbage collection when there is no strong reference
+to it.
+
+Every [`BaseObject`][] is tracked with its creation realm and will be destroyed
+when the realm is tearing down.
 
 #### Closing libuv handles
 

src/aliased_buffer-inl.h

@@ -4,6 +4,7 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "aliased_buffer.h"
+#include "memory_tracker-inl.h"
 #include "util-inl.h"
 
 namespace node {

src/base_object.cc

@@ -1,5 +1,6 @@
 #include "base_object.h"
 #include "env-inl.h"
+#include "memory_tracker-inl.h"
 #include "node_messaging.h"
 #include "node_realm-inl.h"
 
@@ -23,13 +24,11 @@ BaseObject::BaseObject(Realm* realm, Local<Object> object)
   CHECK_EQ(false, object.IsEmpty());
   CHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount);
   SetInternalFields(realm->isolate_data(), object, static_cast<void*>(this));
-  realm->AddCleanupHook(DeleteMe, static_cast<void*>(this));
-  realm->modify_base_object_count(1);
+  realm->TrackBaseObject(this);
 }
 
 BaseObject::~BaseObject() {
-  realm()->modify_base_object_count(-1);
-  realm()->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
+  realm()->UntrackBaseObject(this);
 
   if (UNLIKELY(has_pointer_data())) {
     PointerData* metadata = pointer_data();
@@ -146,12 +145,11 @@ void BaseObject::increase_refcount() {
     persistent_handle_.ClearWeak();
 }
 
-void BaseObject::DeleteMe(void* data) {
-  BaseObject* self = static_cast<BaseObject*>(data);
-  if (self->has_pointer_data() && self->pointer_data()->strong_ptr_count > 0) {
-    return self->Detach();
+void BaseObject::DeleteMe() {
+  if (has_pointer_data() && pointer_data()->strong_ptr_count > 0) {
+    return Detach();
   }
-  delete self;
+  delete this;
 }
 
 bool BaseObject::IsDoneInitializing() const {
@@ -170,4 +168,17 @@ bool BaseObject::IsNotIndicativeOfMemoryLeakAtExit() const {
   return IsWeakOrDetached();
 }
 
+void BaseObjectList::Cleanup() {
+  while (!IsEmpty()) {
+    BaseObject* bo = PopFront();
+    bo->DeleteMe();
+  }
+}
+
+void BaseObjectList::MemoryInfo(node::MemoryTracker* tracker) const {
+  for (auto bo : *this) {
+    if (bo->IsDoneInitializing()) tracker->Track(bo);
+  }
+}
+
 }  // namespace node

src/base_object.h

@@ -27,6 +27,7 @@
 #include <type_traits>  // std::remove_reference
 #include "base_object_types.h"
 #include "memory_tracker.h"
+#include "util.h"
 #include "v8.h"
 
 namespace node {
@@ -192,7 +193,7 @@ class BaseObject : public MemoryRetainer {
  private:
   v8::Local<v8::Object> WrappedObject() const override;
   bool IsRootNode() const override;
-  static void DeleteMe(void* data);
+  void DeleteMe();
 
   // persistent_handle_ needs to be at a fixed offset from the start of the
   // class because it is used by src/node_postmortem_metadata.cc to calculate
@@ -237,6 +238,20 @@ class BaseObject : public MemoryRetainer {
 
   Realm* realm_;
   PointerData* pointer_data_ = nullptr;
+  ListNode<BaseObject> base_object_list_node_;
+
+  friend class BaseObjectList;
+};
+
+class BaseObjectList
+    : public ListHead<BaseObject, &BaseObject::base_object_list_node_>,
+      public MemoryRetainer {
+ public:
+  void Cleanup();
+
+  SET_MEMORY_INFO_NAME(BaseObjectList)
+  SET_SELF_SIZE(BaseObjectList)
+  void MemoryInfo(node::MemoryTracker* tracker) const override;
 };
 
 #define ASSIGN_OR_RETURN_UNWRAP(ptr, obj, ...)                                 \

src/cleanup_queue-inl.h

@@ -3,19 +3,11 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
-#include "base_object.h"
 #include "cleanup_queue.h"
-#include "memory_tracker-inl.h"
 #include "util.h"
 
 namespace node {
 
-inline void CleanupQueue::MemoryInfo(MemoryTracker* tracker) const {
-  ForEachBaseObject([&](BaseObject* obj) {
-    if (obj->IsDoneInitializing()) tracker->Track(obj);
-  });
-}
-
 inline size_t CleanupQueue::SelfSize() const {
   return sizeof(CleanupQueue) +
          cleanup_hooks_.size() * sizeof(CleanupHookCallback);
@@ -37,24 +29,6 @@ void CleanupQueue::Remove(Callback cb, void* arg) {
   cleanup_hooks_.erase(search);
 }
 
-template <typename T>
-void CleanupQueue::ForEachBaseObject(T&& iterator) const {
-  std::vector<CleanupHookCallback> callbacks = GetOrdered();
-
-  for (const auto& hook : callbacks) {
-    BaseObject* obj = GetBaseObject(hook);
-    if (obj != nullptr) iterator(obj);
-  }
-}
-
-BaseObject* CleanupQueue::GetBaseObject(
-    const CleanupHookCallback& callback) const {
-  if (callback.fn_ == BaseObject::DeleteMe)
-    return static_cast<BaseObject*>(callback.arg_);
-  else
-    return nullptr;
-}
-
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

src/cleanup_queue.h

@@ -12,8 +12,6 @@
 
 namespace node {
 
-class BaseObject;
-
 class CleanupQueue : public MemoryRetainer {
  public:
   typedef void (*Callback)(void*);
@@ -24,7 +22,7 @@ class CleanupQueue : public MemoryRetainer {
   CleanupQueue(const CleanupQueue&) = delete;
 
   SET_MEMORY_INFO_NAME(CleanupQueue)
-  inline void MemoryInfo(node::MemoryTracker* tracker) const override;
+  SET_NO_MEMORY_INFO()
   inline size_t SelfSize() const override;
 
   inline bool empty() const;
@@ -33,9 +31,6 @@ class CleanupQueue : public MemoryRetainer {
   inline void Remove(Callback cb, void* arg);
   void Drain();
 
-  template <typename T>
-  inline void ForEachBaseObject(T&& iterator) const;
-
  private:
   class CleanupHookCallback {
    public:
@@ -68,7 +63,6 @@ class CleanupQueue : public MemoryRetainer {
   };
 
   std::vector<CleanupHookCallback> GetOrdered() const;
-  inline BaseObject* GetBaseObject(const CleanupHookCallback& callback) const;
 
   // Use an unordered_set, so that we have efficient insertion and removal.
   std::unordered_set<CleanupHookCallback,

src/debug_utils-inl.h

@@ -5,6 +5,7 @@
 
 #include "debug_utils.h"
 #include "env.h"
+#include "util-inl.h"
 
 #include <type_traits>
 

src/env.cc

@@ -1279,7 +1279,7 @@ void Environment::RunCleanup() {
     cleanable->Clean();
   }
 
-  while (!cleanup_queue_.empty() || principal_realm_->HasCleanupHooks() ||
+  while (!cleanup_queue_.empty() || principal_realm_->PendingCleanup() ||
          native_immediates_.size() > 0 ||
          native_immediates_threadsafe_.size() > 0 ||
          native_immediates_interrupts_.size() > 0) {

src/node_messaging.cc

@@ -42,7 +42,7 @@ using v8::WasmModuleObject;
 
 namespace node {
 
-using BaseObjectList = std::vector<BaseObjectPtr<BaseObject>>;
+using BaseObjectPtrList = std::vector<BaseObjectPtr<BaseObject>>;
 using TransferMode = BaseObject::TransferMode;
 
 // Hack to have WriteHostObject inform ReadHostObject that the value
@@ -1347,33 +1347,32 @@ std::unique_ptr<TransferData> JSTransferable::TransferOrClone() const {
                                 Global<Value>(env()->isolate(), data));
 }
 
-Maybe<BaseObjectList>
-JSTransferable::NestedTransferables() const {
+Maybe<BaseObjectPtrList> JSTransferable::NestedTransferables() const {
   // Call `this[kTransferList]()` and return the resulting list of BaseObjects.
   HandleScope handle_scope(env()->isolate());
   Local<Context> context = env()->isolate()->GetCurrentContext();
   Local<Symbol> method_name = env()->messaging_transfer_list_symbol();
 
   Local<Value> method;
   if (!target()->Get(context, method_name).ToLocal(&method)) {
-    return Nothing<BaseObjectList>();
+    return Nothing<BaseObjectPtrList>();
   }
-  if (!method->IsFunction()) return Just(BaseObjectList {});
+  if (!method->IsFunction()) return Just(BaseObjectPtrList{});
 
   Local<Value> list_v;
   if (!method.As<Function>()
            ->Call(context, target(), 0, nullptr)
            .ToLocal(&list_v)) {
-    return Nothing<BaseObjectList>();
+    return Nothing<BaseObjectPtrList>();
   }
-  if (!list_v->IsArray()) return Just(BaseObjectList {});
+  if (!list_v->IsArray()) return Just(BaseObjectPtrList{});
   Local<Array> list = list_v.As<Array>();
 
-  BaseObjectList ret;
+  BaseObjectPtrList ret;
   for (size_t i = 0; i < list->Length(); i++) {
     Local<Value> value;
     if (!list->Get(context, i).ToLocal(&value))
-      return Nothing<BaseObjectList>();
+      return Nothing<BaseObjectPtrList>();
     if (!value->IsObject()) {
       continue;
     }

src/node_realm-inl.h

@@ -3,7 +3,7 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
-#include "cleanup_queue-inl.h"
+#include "node_context_data.h"
 #include "node_realm.h"
 
 namespace node {
@@ -105,11 +105,9 @@ inline BindingDataStore* Realm::binding_data_store() {
 
 template <typename T>
 void Realm::ForEachBaseObject(T&& iterator) const {
-  cleanup_queue_.ForEachBaseObject(std::forward<T>(iterator));
-}
-
-void Realm::modify_base_object_count(int64_t delta) {
-  base_object_count_ += delta;
+  for (auto bo : base_object_list_) {
+    iterator(bo);
+  }
 }
 
 int64_t Realm::base_object_created_after_bootstrap() const {
@@ -120,16 +118,19 @@ int64_t Realm::base_object_count() const {
   return base_object_count_;
 }
 
-void Realm::AddCleanupHook(CleanupQueue::Callback fn, void* arg) {
-  cleanup_queue_.Add(fn, arg);
+void Realm::TrackBaseObject(BaseObject* bo) {
+  DCHECK_EQ(bo->realm(), this);
+  base_object_list_.PushBack(bo);
+  ++base_object_count_;
 }
 
-void Realm::RemoveCleanupHook(CleanupQueue::Callback fn, void* arg) {
-  cleanup_queue_.Remove(fn, arg);
+void Realm::UntrackBaseObject(BaseObject* bo) {
+  DCHECK_EQ(bo->realm(), this);
+  --base_object_count_;
 }
 
-bool Realm::HasCleanupHooks() const {
-  return !cleanup_queue_.empty();
+bool Realm::PendingCleanup() const {
+  return !base_object_list_.IsEmpty();
 }
 
 }  // namespace node

src/node_realm.cc

@@ -34,7 +34,7 @@ void Realm::MemoryInfo(MemoryTracker* tracker) const {
   PER_REALM_STRONG_PERSISTENT_VALUES(V)
 #undef V
 
-  tracker->TrackField("cleanup_queue", cleanup_queue_);
+  tracker->TrackField("base_object_list", base_object_list_);
   tracker->TrackField("builtins_with_cache", builtins_with_cache);
   tracker->TrackField("builtins_without_cache", builtins_without_cache);
 }
@@ -215,7 +215,7 @@ void Realm::RunCleanup() {
   for (size_t i = 0; i < binding_data_store_.size(); ++i) {
     binding_data_store_[i].reset();
   }
-  cleanup_queue_.Drain();
+  base_object_list_.Cleanup();
 }
 
 void Realm::PrintInfoForSnapshot() {

src/node_realm.h

@@ -71,9 +71,9 @@ class Realm : public MemoryRetainer {
   v8::MaybeLocal<v8::Value> ExecuteBootstrapper(const char* id);
   v8::MaybeLocal<v8::Value> RunBootstrapping();
 
-  inline void AddCleanupHook(CleanupQueue::Callback cb, void* arg);
-  inline void RemoveCleanupHook(CleanupQueue::Callback cb, void* arg);
-  inline bool HasCleanupHooks() const;
+  inline void TrackBaseObject(BaseObject* bo);
+  inline void UntrackBaseObject(BaseObject* bo);
+  inline bool PendingCleanup() const;
   void RunCleanup();
 
   template <typename T>
@@ -108,7 +108,6 @@ class Realm : public MemoryRetainer {
   // The BaseObject count is a debugging helper that makes sure that there are
   // no memory leaks caused by BaseObjects staying alive longer than expected
   // (in particular, no circular BaseObjectPtr references).
-  inline void modify_base_object_count(int64_t delta);
   inline int64_t base_object_count() const;
 
   // Base object count created after the bootstrap of the realm.
@@ -154,7 +153,7 @@ class Realm : public MemoryRetainer {
 
   BindingDataStore binding_data_store_;
 
-  CleanupQueue cleanup_queue_;
+  BaseObjectList base_object_list_;
 };
 
 class PrincipalRealm : public Realm {

src/node_shadow_realm.cc

@@ -84,7 +84,7 @@ ShadowRealm::ShadowRealm(Environment* env)
 }
 
 ShadowRealm::~ShadowRealm() {
-  while (HasCleanupHooks()) {
+  while (PendingCleanup()) {
     RunCleanup();
   }
 

src/node_task_runner.cc

@@ -1,5 +1,5 @@
 #include "node_task_runner.h"
-#include "util.h"
+#include "util-inl.h"
 
 #include <regex>  // NOLINT(build/c++11)
 

test/pummel/test-heapdump-env.js

@@ -19,5 +19,6 @@ validateSnapshotNodes('Node / Environment', [{
 validateSnapshotNodes('Node / PrincipalRealm', [{
   children: [
     { node_name: 'process', edge_name: 'process_object' },
+    { node_name: 'Node / BaseObjectList', edge_name: 'base_object_list' },
   ],
 }]);