From 84966703e0f90757d046f3ecfff479c463ad5ab3 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 24 Sep 2024 12:34:16 +0100 Subject: [PATCH] src: track BaseObjects with an efficient list PR-URL: https://github.com/nodejs/node/pull/55104 Refs: https://github.com/nodejs/node/pull/54880 Reviewed-By: Matteo Collina Reviewed-By: Joyee Cheung --- src/README.md | 17 +++++++++++++---- src/aliased_buffer-inl.h | 1 + src/base_object.cc | 29 ++++++++++++++++++++--------- src/base_object.h | 17 ++++++++++++++++- src/cleanup_queue-inl.h | 26 -------------------------- src/cleanup_queue.h | 8 +------- src/debug_utils-inl.h | 1 + src/env.cc | 2 +- src/node_messaging.cc | 17 ++++++++--------- src/node_realm-inl.h | 25 +++++++++++++------------ src/node_realm.cc | 4 ++-- src/node_realm.h | 9 ++++----- src/node_shadow_realm.cc | 2 +- src/node_task_runner.cc | 2 +- test/pummel/test-heapdump-env.js | 1 + 15 files changed, 83 insertions(+), 78 deletions(-) diff --git a/src/README.md b/src/README.md index a08ef42b112..300b74c28d5 100644 --- a/src/README.md +++ b/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 diff --git a/src/aliased_buffer-inl.h b/src/aliased_buffer-inl.h index f04fb7c2667..da2c45321f6 100644 --- a/src/aliased_buffer-inl.h +++ b/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 { diff --git a/src/base_object.cc b/src/base_object.cc index 58ceecca2f9..216a9729f0d 100644 --- a/src/base_object.cc +++ b/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) CHECK_EQ(false, object.IsEmpty()); CHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount); SetInternalFields(realm->isolate_data(), object, static_cast(this)); - realm->AddCleanupHook(DeleteMe, static_cast(this)); - realm->modify_base_object_count(1); + realm->TrackBaseObject(this); } BaseObject::~BaseObject() { - realm()->modify_base_object_count(-1); - realm()->RemoveCleanupHook(DeleteMe, static_cast(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(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 diff --git a/src/base_object.h b/src/base_object.h index 9a93e41b918..29fbeed40da 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -27,6 +27,7 @@ #include // 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 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 base_object_list_node_; + + friend class BaseObjectList; +}; + +class BaseObjectList + : public ListHead, + 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, ...) \ diff --git a/src/cleanup_queue-inl.h b/src/cleanup_queue-inl.h index dad23ecf3ae..ac86d06ada7 100644 --- a/src/cleanup_queue-inl.h +++ b/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 -void CleanupQueue::ForEachBaseObject(T&& iterator) const { - std::vector 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(callback.arg_); - else - return nullptr; -} - } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/cleanup_queue.h b/src/cleanup_queue.h index 15451ab9473..7edc828eb65 100644 --- a/src/cleanup_queue.h +++ b/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 - inline void ForEachBaseObject(T&& iterator) const; - private: class CleanupHookCallback { public: @@ -68,7 +63,6 @@ class CleanupQueue : public MemoryRetainer { }; std::vector GetOrdered() const; - inline BaseObject* GetBaseObject(const CleanupHookCallback& callback) const; // Use an unordered_set, so that we have efficient insertion and removal. std::unordered_set diff --git a/src/env.cc b/src/env.cc index 3b3bf696555..57da01db8f3 100644 --- a/src/env.cc +++ b/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) { diff --git a/src/node_messaging.cc b/src/node_messaging.cc index be51859b73e..c1153e8a090 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -42,7 +42,7 @@ using v8::WasmModuleObject; namespace node { -using BaseObjectList = std::vector>; +using BaseObjectPtrList = std::vector>; using TransferMode = BaseObject::TransferMode; // Hack to have WriteHostObject inform ReadHostObject that the value @@ -1347,8 +1347,7 @@ std::unique_ptr JSTransferable::TransferOrClone() const { Global(env()->isolate(), data)); } -Maybe -JSTransferable::NestedTransferables() const { +Maybe JSTransferable::NestedTransferables() const { // Call `this[kTransferList]()` and return the resulting list of BaseObjects. HandleScope handle_scope(env()->isolate()); Local context = env()->isolate()->GetCurrentContext(); @@ -1356,24 +1355,24 @@ JSTransferable::NestedTransferables() const { Local method; if (!target()->Get(context, method_name).ToLocal(&method)) { - return Nothing(); + return Nothing(); } - if (!method->IsFunction()) return Just(BaseObjectList {}); + if (!method->IsFunction()) return Just(BaseObjectPtrList{}); Local list_v; if (!method.As() ->Call(context, target(), 0, nullptr) .ToLocal(&list_v)) { - return Nothing(); + return Nothing(); } - if (!list_v->IsArray()) return Just(BaseObjectList {}); + if (!list_v->IsArray()) return Just(BaseObjectPtrList{}); Local list = list_v.As(); - BaseObjectList ret; + BaseObjectPtrList ret; for (size_t i = 0; i < list->Length(); i++) { Local value; if (!list->Get(context, i).ToLocal(&value)) - return Nothing(); + return Nothing(); if (!value->IsObject()) { continue; } diff --git a/src/node_realm-inl.h b/src/node_realm-inl.h index fe20ac2b2fe..7d04898b8f5 100644 --- a/src/node_realm-inl.h +++ b/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 void Realm::ForEachBaseObject(T&& iterator) const { - cleanup_queue_.ForEachBaseObject(std::forward(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 diff --git a/src/node_realm.cc b/src/node_realm.cc index 6262ed8cde5..ead7f02f6cb 100644 --- a/src/node_realm.cc +++ b/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() { diff --git a/src/node_realm.h b/src/node_realm.h index 51fbd502a10..be18f39c5bf 100644 --- a/src/node_realm.h +++ b/src/node_realm.h @@ -71,9 +71,9 @@ class Realm : public MemoryRetainer { v8::MaybeLocal ExecuteBootstrapper(const char* id); v8::MaybeLocal 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 @@ -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 { diff --git a/src/node_shadow_realm.cc b/src/node_shadow_realm.cc index e7199e18756..1273555b50c 100644 --- a/src/node_shadow_realm.cc +++ b/src/node_shadow_realm.cc @@ -84,7 +84,7 @@ ShadowRealm::ShadowRealm(Environment* env) } ShadowRealm::~ShadowRealm() { - while (HasCleanupHooks()) { + while (PendingCleanup()) { RunCleanup(); } diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc index 32ef0aa2ad1..359212d3ef5 100644 --- a/src/node_task_runner.cc +++ b/src/node_task_runner.cc @@ -1,5 +1,5 @@ #include "node_task_runner.h" -#include "util.h" +#include "util-inl.h" #include // NOLINT(build/c++11) diff --git a/test/pummel/test-heapdump-env.js b/test/pummel/test-heapdump-env.js index dc532ddb939..4027a2b8a7b 100644 --- a/test/pummel/test-heapdump-env.js +++ b/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' }, ], }]);