From 68803ba5afc29d2fba29039913f4d76bbcfb76a6 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 19 Mar 2025 23:05:46 +0100 Subject: [PATCH] fixup! src: track cppgc wrappers with a list in Realm --- src/cppgc_helpers-inl.h | 6 ++++++ src/cppgc_helpers.cc | 27 +++++++++++++++++++++++---- src/cppgc_helpers.h | 16 ++++++++++------ src/node_realm-inl.h | 4 +++- src/node_realm.cc | 19 +++++++++++++++++++ src/node_realm.h | 39 ++++++++++++++++++++++++++++++++++++++- 6 files changed, 99 insertions(+), 12 deletions(-) diff --git a/src/cppgc_helpers-inl.h b/src/cppgc_helpers-inl.h index 9a1a9278914..745ecab746f 100644 --- a/src/cppgc_helpers-inl.h +++ b/src/cppgc_helpers-inl.h @@ -52,6 +52,12 @@ Environment* CppgcMixin::env() const { return realm_->env(); } +CppgcMixin::~CppgcMixin() { + if (realm_ != nullptr) { + realm_->set_should_purge_empty_cppgc_wrappers(true); + } +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/cppgc_helpers.cc b/src/cppgc_helpers.cc index 2d09abc19ba..4e3ffb678c2 100644 --- a/src/cppgc_helpers.cc +++ b/src/cppgc_helpers.cc @@ -4,15 +4,34 @@ namespace node { void CppgcWrapperList::Cleanup() { - for (auto handle : *this) { - handle->Finalize(); + for (auto node : *this) { + CppgcMixin* ptr = node->persistent.Get(); + if (ptr != nullptr) { + ptr->Finalize(); + } } } void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const { - for (auto handle : *this) { - tracker->Track(handle); + for (auto node : *this) { + CppgcMixin* ptr = node->persistent.Get(); + if (ptr != nullptr) { + tracker->Track(ptr); + } } } +void CppgcWrapperList::PurgeEmpty() { + for (auto weak_it = begin(); weak_it != end();) { + CppgcWrapperListNode* node = *weak_it; + auto next_it = ++weak_it; + // The underlying cppgc wrapper has already been garbage collected. + // Remove it from the list. + if (!node->persistent) { + node->persistent.Clear(); + delete node; + } + weak_it = next_it; + } +} } // namespace node diff --git a/src/cppgc_helpers.h b/src/cppgc_helpers.h index bdaef203ad5..fb2af584a4a 100644 --- a/src/cppgc_helpers.h +++ b/src/cppgc_helpers.h @@ -6,6 +6,7 @@ #include // std::remove_reference #include "cppgc/garbage-collected.h" #include "cppgc/name-provider.h" +#include "cppgc/persistent.h" #include "memory_tracker.h" #include "util.h" #include "v8-cppgc.h" @@ -16,7 +17,7 @@ namespace node { class Environment; class Realm; -class CppgcWrapperList; +class CppgcWrapperListNode; /** * This is a helper mixin with a BaseObject-like interface to help @@ -100,17 +101,20 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer { realm_ = nullptr; } - // The default implementation of Clean() is a no-op. Subclasses - // should override it to perform cleanup that require a living Realm, - // instead of doing these cleanups directly in the destructor. + // The default implementation of Clean() is a no-op. If subclasses wish + // to perform cleanup that require a living Realm, they should + // should put the cleanups in a Clean() override, and call this->Finalize() + // in the destructor, instead of doing those cleanups directly in the + // destructor. virtual void Clean(Realm* realm) {} - friend class CppgcWrapperList; + inline ~CppgcMixin(); + + friend class CppgcWrapperListNode; private: Realm* realm_ = nullptr; v8::TracedReference traced_reference_; - ListNode wrapper_list_node_; }; // If the class doesn't have additional owned traceable data, use this macro to diff --git a/src/node_realm-inl.h b/src/node_realm-inl.h index d1c3f18cc6e..f162d1506c9 100644 --- a/src/node_realm-inl.h +++ b/src/node_realm-inl.h @@ -130,9 +130,11 @@ void Realm::TrackBaseObject(BaseObject* bo) { ++base_object_count_; } +CppgcWrapperListNode::CppgcWrapperListNode(CppgcMixin* ptr) : persistent(ptr) {} + void Realm::TrackCppgcWrapper(CppgcMixin* handle) { DCHECK_EQ(handle->realm(), this); - cppgc_wrapper_list_.PushFront(handle); + cppgc_wrapper_list_.PushFront(new CppgcWrapperListNode(handle)); } void Realm::UntrackBaseObject(BaseObject* bo) { diff --git a/src/node_realm.cc b/src/node_realm.cc index c9ea23df32f..b7ac5d74c3b 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -10,7 +10,10 @@ namespace node { using v8::Context; using v8::EscapableHandleScope; +using v8::GCCallbackFlags; +using v8::GCType; using v8::HandleScope; +using v8::Isolate; using v8::Local; using v8::MaybeLocal; using v8::Object; @@ -22,12 +25,28 @@ Realm::Realm(Environment* env, v8::Local context, Kind kind) : env_(env), isolate_(context->GetIsolate()), kind_(kind) { context_.Reset(isolate_, context); env->AssignToContext(context, this, ContextInfo("")); + // The environment can also purge empty wrappers in the check callback, + // though that may be a bit excessive depending on usage patterns. + // For now using the GC epilogue is adequate. + isolate_->AddGCEpilogueCallback(PurgeEmptyCppgcWrappers, this); } Realm::~Realm() { + isolate_->RemoveGCEpilogueCallback(PurgeEmptyCppgcWrappers, this); CHECK_EQ(base_object_count_, 0); } +void Realm::PurgeEmptyCppgcWrappers(Isolate* isolate, + GCType type, + GCCallbackFlags flags, + void* data) { + Realm* realm = static_cast(data); + if (realm->should_purge_empty_cppgc_wrappers_) { + realm->cppgc_wrapper_list_.PurgeEmpty(); + realm->should_purge_empty_cppgc_wrappers_ = false; + } +} + void Realm::MemoryInfo(MemoryTracker* tracker) const { #define V(PropertyName, TypeName) \ tracker->TrackField(#PropertyName, PropertyName()); diff --git a/src/node_realm.h b/src/node_realm.h index 2c662f14ddb..9ad38be761d 100644 --- a/src/node_realm.h +++ b/src/node_realm.h @@ -26,11 +26,34 @@ using BindingDataStore = std::array, static_cast(BindingDataType::kBindingDataTypeCount)>; +/** + * This is a wrapper around a weak persistent of CppgcMixin, used in the + * CppgcWrapperList to avoid accessing already garbage collected CppgcMixins. + */ +class CppgcWrapperListNode { + public: + explicit inline CppgcWrapperListNode(CppgcMixin* ptr); + inline explicit operator bool() const { return !persistent; } + inline CppgcMixin* operator->() const { return persistent.Get(); } + inline CppgcMixin* operator*() const { return persistent.Get(); } + + cppgc::WeakPersistent persistent; + // Used by ContainerOf in the ListNode implementation for fast manipulation of + // CppgcWrapperList. + ListNode wrapper_list_node; +}; + +/** + * A per-realm list of weak persistent of cppgc wrappers, which implements + * iterations that require iterate over cppgc wrappers created by Node.js. + */ class CppgcWrapperList - : public ListHead, + : public ListHead, public MemoryRetainer { public: void Cleanup(); + void PurgeEmpty(); SET_MEMORY_INFO_NAME(CppgcWrapperList) SET_SELF_SIZE(CppgcWrapperList) @@ -141,6 +164,14 @@ class Realm : public MemoryRetainer { // it's only used for tests. std::vector builtins_in_snapshot; + // This used during the destruction of cppgc wrappers to inform a GC epilogue + // callback to clean up the weak persistents used to track cppgc wrappers if + // the wrappers are already garbage collected to prevent holding on to + // excessive useless persistents. + inline void set_should_purge_empty_cppgc_wrappers(bool value) { + should_purge_empty_cppgc_wrappers_ = value; + } + protected: ~Realm(); @@ -150,11 +181,17 @@ class Realm : public MemoryRetainer { // Shorthand for isolate pointer. v8::Isolate* isolate_; v8::Global context_; + bool should_purge_empty_cppgc_wrappers_ = false; #define V(PropertyName, TypeName) v8::Global PropertyName##_; PER_REALM_STRONG_PERSISTENT_VALUES(V) #undef V + static void PurgeEmptyCppgcWrappers(v8::Isolate* isolate, + v8::GCType type, + v8::GCCallbackFlags flags, + void* data); + private: void InitializeContext(v8::Local context, const RealmSerializeInfo* realm_info);