From 993955b0b0007cf7163c8b2482846859a28b540e Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 9 Jan 2025 01:19:57 +0100 Subject: [PATCH 1/2] src: track cppgc wrappers with a list in Realm This allows us to perform cleanups of cppgc wrappers that rely on a living Realm during Realm shutdown. Otherwise the cleanup may happen during object destruction, which can be triggered by GC after Realm shutdown, leading to invalid access to Realm. The general pattern for this type of non-trivial destruction is designed to be: ``` class MyWrap final : CPPGC_MIXIN(MyWrap) { public: ~MyWrap() { this->Finalize(); } void Clean(Realm* realm) override { // Do cleanup that relies on a living Realm. This would be // called by CppgcMixin::Finalize() first during Realm // shutdown, while the Realm is still alive. If the destructor // calls Finalize() again later during garbage collection that // happens after Realm shutdown, Clean() would be skipped, // preventing invalid access to the Realm. } } ``` In addition, this allows us to trace external memory held by the wrappers in the heap snapshots if we add synthethic edges between the wrappers and other nodes in the embdder graph callback, or to perform snapshot serialization for them. --- node.gyp | 1 + src/README.md | 81 ++++++++++++++++++++++++++++++++-- src/cppgc_helpers-inl.h | 59 +++++++++++++++++++++++++ src/cppgc_helpers.cc | 18 ++++++++ src/cppgc_helpers.h | 93 ++++++++++++++++++++++++---------------- src/memory_tracker-inl.h | 88 ++++++++++++++++++++++++++++++++++--- src/memory_tracker.h | 12 +++++- src/node_contextify.h | 4 +- src/node_realm-inl.h | 5 +++ src/node_realm.cc | 2 + src/node_realm.h | 16 +++++++ 11 files changed, 330 insertions(+), 49 deletions(-) create mode 100644 src/cppgc_helpers-inl.h create mode 100644 src/cppgc_helpers.cc diff --git a/node.gyp b/node.gyp index 0496f75fea5637..20903c987dc7ad 100644 --- a/node.gyp +++ b/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', diff --git a/src/README.md b/src/README.md index a97e1b9ba52e8f..5904d01f7d8d5a 100644 --- a/src/README.md +++ b/src/README.md @@ -1131,6 +1131,17 @@ class MyWrap final : CPPGC_MIXIN(MyWrap) { } ``` +If the wrapper needs to perform cleanups when it's destroyed and that +cleanup relies on a living Node.js `Realm`, it should implement a +pattern like this: + +```cpp + ~MyWrap() { this->Finalize(); } + void Clean(Realm* env) override { + // Do cleanup that relies on a living Realm. + } +``` + `cppgc::GarbageCollected` types are expected to implement a `void Trace(cppgc::Visitor* visitor) const` method. When they are the final class in the hierarchy, this method must be marked `final`. For @@ -1285,16 +1296,76 @@ referrer->Set( ).ToLocalChecked(); ``` +#### Creating references between cppgc-managed objects and `BaseObject`s + +This is currently unsupported with the existing helpers. If this has +to be done, new helpers must be implemented first. Consult the cppgc +headers when trying to implement it. + +Another way to work around it is to always do the migration bottom-to-top. +If a cppgc-managed object needs to reference a `BaseObject`, convert +that `BaseObject` to be cppgc-managed first, and then use `cppgc::Member` +to create the references. + +#### Lifetime and cleanups of cppgc-managed objects + Typically, a newly created cppgc-managed wrapper object should be held alive by the JavaScript land (for example, by being returned by a method and staying alive in a closure). Long-lived cppgc objects can also be held alive from C++ using persistent handles (see `deps/v8/include/cppgc/persistent.h`) or as members of other living cppgc-managed objects (see `deps/v8/include/cppgc/member.h`) if necessary. -Its destructor will be called when no other objects from the V8 heap reference -it, this can happen at any time after the garbage collector notices that -it's no longer reachable and before the V8 isolate is torn down. -See the [Oilpan documentation in Chromium][] for more details. + +When a cppgc-managed object is no longer reachable in the heap, its destructor +will be invoked by the garbage collection, which can happen after the `Realm` +is already gone, or after any object it references is gone. It is therefore +unsafe to invoke V8 APIs directly in the destructors. To ensure safety, +the cleanups of a cppgc-managed object should adhere to different patterns, +depending on what it needs to do: + +1. If it does not need to do any non-trivial cleanup, nor does its members, just use + the default destructor. Cleanup of `v8::TracedReference` and + `cppgc::Member` are already handled automatically by V8 so if they are all the + non-trivial members the class has, this case applies. +2. If the cleanup relies on a living `Realm`, but does not need to access V8 + APIs, the class should use this pattern in its class body: + + ```cpp + ~MyWrap() { this->Finalize(); } + void Clean(Realm* env) override { + // Do cleanup that relies on a living Realm. This would be + // called by CppgcMixin::Finalize() first during Realm shutdown, + // while the Realm is still alive. If the destructor calls + // Finalize() again later during garbage collection that happens after + // Realm shutdown, Clean() would be skipped, preventing + // invalid access to the Realm. + } + ``` + + If implementers want to call `Finalize()` from `Clean()` again, they + need to make sure that calling `Clean()` recursively is safe. +3. If the cleanup relies on access to the V8 heap, including using any V8 + handles, in addition to 2, it should use the `CPPGC_USING_PRE_FINALIZER` + macro (from the [`cppgc/prefinalizer.h` header][]) in the private + section of its class body: + + ```cpp + private: + CPPGC_USING_PRE_FINALIZER(MyWrap, Finalize); + ``` + +Both the destructor and the pre-finalizer are always called on the thread +in which the object is created. + +It's worth noting that the use of pre-finalizers would have a negative impact +on the garbage collection performance as V8 needs to scan all of them during +each sweeping. If the object is expected to be created frequently in large +amounts in the application, it's better to avoid access to the V8 heap in its +cleanup to avoid having to use a pre-finalizer. + +For more information about the cleanup of cppgc-managed objects and +what can be done in a pre-finalizer, see the [cppgc documentation][] and +the [`cppgc/prefinalizer.h` header][]. ### Callback scopes @@ -1421,6 +1492,7 @@ static void GetUserInfo(const FunctionCallbackInfo& args) { [`async_hooks` module]: https://nodejs.org/api/async_hooks.html [`async_wrap.h`]: async_wrap.h [`base_object.h`]: base_object.h +[`cppgc/prefinalizer.h` header]: ../deps/v8/include/cppgc/prefinalizer.h [`handle_wrap.h`]: handle_wrap.h [`memory_tracker.h`]: memory_tracker.h [`req_wrap.h`]: req_wrap.h @@ -1432,6 +1504,7 @@ static void GetUserInfo(const FunctionCallbackInfo& args) { [`vm` module]: https://nodejs.org/api/vm.html [binding function]: #binding-functions [cleanup hooks]: #cleanup-hooks +[cppgc documentation]: ../deps/v8/include/cppgc/README.md [event loop]: #event-loop [exception handling]: #exception-handling [fast API calls]: ../doc/contributing/adding-v8-fast-api.md diff --git a/src/cppgc_helpers-inl.h b/src/cppgc_helpers-inl.h new file mode 100644 index 00000000000000..9a1a927891443f --- /dev/null +++ b/src/cppgc_helpers-inl.h @@ -0,0 +1,59 @@ +#ifndef SRC_CPPGC_HELPERS_INL_H_ +#define SRC_CPPGC_HELPERS_INL_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "cppgc_helpers.h" +#include "env-inl.h" + +namespace node { + +template +void CppgcMixin::Wrap(T* ptr, Realm* realm, v8::Local obj) { + CHECK_GE(obj->InternalFieldCount(), T::kInternalFieldCount); + ptr->realm_ = realm; + v8::Isolate* isolate = realm->isolate(); + ptr->traced_reference_ = v8::TracedReference(isolate, obj); + // Note that ptr must be of concrete type T in Wrap. + v8::Object::Wrap(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 +void CppgcMixin::Wrap(T* ptr, Environment* env, v8::Local obj) { + Wrap(ptr, env->principal_realm(), obj); +} + +template +T* CppgcMixin::Unwrap(v8::Local 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(obj->GetAlignedPointerFromInternalField(T::kSlot)); + return ptr; +} + +v8::Local CppgcMixin::object() const { + return traced_reference_.Get(realm_->isolate()); +} + +Environment* CppgcMixin::env() const { + return realm_->env(); +} + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_CPPGC_HELPERS_INL_H_ diff --git a/src/cppgc_helpers.cc b/src/cppgc_helpers.cc new file mode 100644 index 00000000000000..2d09abc19baee2 --- /dev/null +++ b/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->Finalize(); + } +} + +void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const { + for (auto handle : *this) { + tracker->Track(handle); + } +} + +} // namespace node diff --git a/src/cppgc_helpers.h b/src/cppgc_helpers.h index 20b9004917cfbe..bdaef203ad50ce 100644 --- a/src/cppgc_helpers.h +++ b/src/cppgc_helpers.h @@ -6,14 +6,18 @@ #include // 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. @@ -25,20 +29,29 @@ namespace node { * with V8's GC scheduling. * * A cppgc-managed native wrapper should look something like this, note - * that per cppgc rules, CPPGC_MIXIN(Klass) must be at the left-most + * that per cppgc rules, CPPGC_MIXIN(MyWrap) must be at the left-most * position in the hierarchy (which ensures cppgc::GarbageCollected * is at the left-most position). * - * class Klass final : CPPGC_MIXIN(Klass) { + * class MyWrap final : CPPGC_MIXIN(MyWrap) { * public: - * SET_CPPGC_NAME(Klass) // Sets the heap snapshot name to "Node / Klass" + * SET_CPPGC_NAME(MyWrap) // Sets the heap snapshot name to "Node / MyWrap" * void Trace(cppgc::Visitor* visitor) const final { * CppgcMixin::Trace(visitor); * visitor->Trace(...); // Trace any additional owned traceable data * } * } + * + * If the wrapper needs to perform cleanups when it's destroyed and that + * cleanup relies on a living Node.js `Realm`, it should implement a + * pattern like this: + * + * ~MyWrap() { this->Destroy(); } + * void Clean(Realm* env) override { + * // Do cleanup that relies on a living Environemnt. + * } */ -class CppgcMixin : public cppgc::GarbageCollectedMixin { +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. @@ -48,39 +61,19 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin { // invoked from the child class constructor, per cppgc::GarbageCollectedMixin // rules. template - static void Wrap(T* ptr, Environment* env, v8::Local obj) { - CHECK_GE(obj->InternalFieldCount(), T::kInternalFieldCount); - ptr->env_ = env; - v8::Isolate* isolate = env->isolate(); - ptr->traced_reference_ = v8::TracedReference(isolate, obj); - v8::Object::Wrap(isolate, obj, ptr); - // Keep the layout consistent with BaseObjects. - obj->SetAlignedPointerInInternalField( - kEmbedderType, env->isolate_data()->embedder_id_for_cppgc()); - obj->SetAlignedPointerInInternalField(kSlot, ptr); - } + static inline void Wrap(T* ptr, Realm* realm, v8::Local obj); + template + static inline void Wrap(T* ptr, Environment* env, v8::Local obj); - v8::Local object() const { - return traced_reference_.Get(env_->isolate()); + inline v8::Local object() const; + inline Environment* env() const; + inline Realm* realm() const { return realm_; } + inline v8::Local object(v8::Isolate* isolate) const { + return traced_reference_.Get(isolate); } - Environment* env() const { return env_; } - template - static T* Unwrap(v8::Local 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(obj->GetAlignedPointerFromInternalField(T::kSlot)); - return ptr; - } + static inline T* Unwrap(v8::Local obj); // Subclasses are expected to invoke CppgcMixin::Trace() in their own Trace() // methods. @@ -88,9 +81,36 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin { visitor->Trace(traced_reference_); } + // 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 Clean() override, and then call this->Finalize() from their + // destructor. Outside of Finalize(), subclasses should avoid calling + // into JavaScript or perform any operation that can trigger garbage + // collection during the destruction. + void Finalize() { + if (realm_ == nullptr) return; + this->Clean(realm_); + 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. + virtual void Clean(Realm* realm) {} + + friend class CppgcWrapperList; + private: - Environment* env_; + 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 @@ -105,7 +125,8 @@ 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; } /** * Similar to ASSIGN_OR_RETURN_UNWRAP() but works on cppgc-managed types diff --git a/src/memory_tracker-inl.h b/src/memory_tracker-inl.h index 31ed6fad98ccc8..56f4a180633d39 100644 --- a/src/memory_tracker-inl.h +++ b/src/memory_tracker-inl.h @@ -3,6 +3,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "cppgc_helpers.h" #include "memory_tracker.h" #include "util-inl.h" @@ -36,6 +37,22 @@ class MemoryRetainerNode : public v8::EmbedderGraph::Node { detachedness_ = retainer_->GetDetachedness(); } + inline MemoryRetainerNode(MemoryTracker* tracker, const CppgcMixin* mixin) + : retainer_(mixin) { + // In this case, the MemoryRetainerNode is merely a wrapper + // to be used in the NodeMap and stack. The actual node being using to add + // edges is always the merged wrapper node of the cppgc-managed wrapper. + CHECK_NOT_NULL(retainer_); + v8::Isolate* isolate = tracker->isolate(); + v8::HandleScope handle_scope(isolate); + v8::Local obj = mixin->object(isolate); + wrapper_node_ = tracker->graph()->V8Node(obj.As()); + + name_ = retainer_->MemoryInfoName(); + size_ = 0; + detachedness_ = retainer_->GetDetachedness(); + } + inline MemoryRetainerNode(MemoryTracker* tracker, const char* name, size_t size, @@ -60,6 +77,11 @@ class MemoryRetainerNode : public v8::EmbedderGraph::Node { } return is_root_node_; } + + bool IsCppgcWrapper() const { + return retainer_ != nullptr && retainer_->IsCppgcWrapper(); + } + v8::EmbedderGraph::Node::Detachedness GetDetachedness() override { return detachedness_; } @@ -94,7 +116,7 @@ void MemoryTracker::TrackInlineFieldWithSize(const char* edge_name, const char* node_name) { if (size > 0) AddNode(GetNodeName(node_name, edge_name), size, edge_name); CHECK(CurrentNode()); - CurrentNode()->size_ -= size; + AdjustCurrentNodeSize(-static_cast(size)); } void MemoryTracker::TrackField(const char* edge_name, @@ -155,7 +177,7 @@ void MemoryTracker::TrackField(const char* edge_name, // Fall back to edge name if node names are not provided if (CurrentNode() != nullptr && subtract_from_self) { // Shift the self size of this container out to a separate node - CurrentNode()->size_ -= sizeof(T); + AdjustCurrentNodeSize(-static_cast(sizeof(T))); } PushNode(GetNodeName(node_name, edge_name), sizeof(T), edge_name); for (Iterator it = value.begin(); it != value.end(); ++it) { @@ -186,7 +208,7 @@ void MemoryTracker::TrackField(const char* edge_name, const T& value, const char* node_name) { // For numbers, creating new nodes is not worth the overhead. - CurrentNode()->size_ += sizeof(T); + AdjustCurrentNodeSize(static_cast(sizeof(T))); } template @@ -273,6 +295,22 @@ void MemoryTracker::TrackInlineField(const char* name, TrackInlineFieldWithSize(name, sizeof(value), "uv_async_t"); } +void MemoryTracker::Track(const CppgcMixin* retainer, const char* edge_name) { + v8::HandleScope handle_scope(isolate_); + auto it = seen_.find(retainer); + if (it != seen_.end()) { + if (CurrentNode() != nullptr) { + graph_->AddEdge(CurrentNode(), it->second, edge_name); + } + return; // It has already been tracked, no need to call MemoryInfo again + } + MemoryRetainerNode* n = PushNode(retainer, edge_name); + retainer->MemoryInfo(this); + CHECK_EQ(CurrentNode(), n->JSWrapperNode()); + // This is a dummy MemoryRetainerNode. The real graph node is wrapper_node_. + PopNode(); +} + void MemoryTracker::Track(const MemoryRetainer* retainer, const char* edge_name) { v8::HandleScope handle_scope(isolate_); @@ -294,7 +332,7 @@ void MemoryTracker::TrackInlineField(const MemoryRetainer* retainer, const char* edge_name) { Track(retainer, edge_name); CHECK(CurrentNode()); - CurrentNode()->size_ -= retainer->SelfSize(); + AdjustCurrentNodeSize(-(static_cast(retainer->SelfSize()))); } template @@ -315,12 +353,33 @@ inline void MemoryTracker::TraitTrackInline(const T& retainer, const char* edge_name) { TraitTrack(retainer, edge_name); CHECK(CurrentNode()); - CurrentNode()->size_ -= MemoryRetainerTraits::SelfSize(retainer); + AdjustCurrentNodeSize( + -(static_cast(MemoryRetainerTraits::SelfSize(retainer)))); } -MemoryRetainerNode* MemoryTracker::CurrentNode() const { +v8::EmbedderGraph::Node* MemoryTracker::CurrentNode() const { if (node_stack_.empty()) return nullptr; - return node_stack_.top(); + MemoryRetainerNode* n = node_stack_.top(); + if (n->IsCppgcWrapper()) { + return n->JSWrapperNode(); + } + return n; +} + +MemoryRetainerNode* MemoryTracker::AddNode(const CppgcMixin* retainer, + const char* edge_name) { + auto it = seen_.find(retainer); + if (it != seen_.end()) { + return it->second; + } + + MemoryRetainerNode* n = new MemoryRetainerNode(this, retainer); + seen_[retainer] = n; + if (CurrentNode() != nullptr) { + graph_->AddEdge(CurrentNode(), n->JSWrapperNode(), edge_name); + } + + return n; } MemoryRetainerNode* MemoryTracker::AddNode(const MemoryRetainer* retainer, @@ -354,6 +413,13 @@ MemoryRetainerNode* MemoryTracker::AddNode(const char* node_name, return n; } +MemoryRetainerNode* MemoryTracker::PushNode(const CppgcMixin* retainer, + const char* edge_name) { + MemoryRetainerNode* n = AddNode(retainer, edge_name); + node_stack_.push(n); + return n; +} + MemoryRetainerNode* MemoryTracker::PushNode(const MemoryRetainer* retainer, const char* edge_name) { MemoryRetainerNode* n = AddNode(retainer, edge_name); @@ -373,6 +439,14 @@ void MemoryTracker::PopNode() { node_stack_.pop(); } +void MemoryTracker::AdjustCurrentNodeSize(int diff) { + if (node_stack_.empty()) return; + MemoryRetainerNode* n = node_stack_.top(); + if (!n->IsCppgcWrapper()) { + n->size_ = static_cast(static_cast(n->size_) + diff); + } +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/memory_tracker.h b/src/memory_tracker.h index 4e0a2fbaaac4f6..a6c3fee6a847a1 100644 --- a/src/memory_tracker.h +++ b/src/memory_tracker.h @@ -18,6 +18,8 @@ class BackingStore; namespace node { +class CppgcMixin; + template struct MallocedBuffer; @@ -133,6 +135,7 @@ class MemoryRetainer { } virtual bool IsRootNode() const { return false; } + virtual bool IsCppgcWrapper() const { return false; } virtual v8::EmbedderGraph::Node::Detachedness GetDetachedness() const { return v8::EmbedderGraph::Node::Detachedness::kUnknown; } @@ -267,6 +270,8 @@ class MemoryTracker { // Put a memory container into the graph, create an edge from // the current node if there is one on the stack. + inline void Track(const CppgcMixin* retainer, + const char* edge_name = nullptr); inline void Track(const MemoryRetainer* retainer, const char* edge_name = nullptr); @@ -299,9 +304,14 @@ class MemoryTracker { typedef std::unordered_map NodeMap; - inline MemoryRetainerNode* CurrentNode() const; + inline void AdjustCurrentNodeSize(int diff); + inline v8::EmbedderGraph::Node* CurrentNode() const; + inline MemoryRetainerNode* AddNode(const CppgcMixin* retainer, + const char* edge_name = nullptr); inline MemoryRetainerNode* AddNode(const MemoryRetainer* retainer, const char* edge_name = nullptr); + inline MemoryRetainerNode* PushNode(const CppgcMixin* retainer, + const char* edge_name = nullptr); inline MemoryRetainerNode* PushNode(const MemoryRetainer* retainer, const char* edge_name = nullptr); inline MemoryRetainerNode* AddNode(const char* node_name, diff --git a/src/node_contextify.h b/src/node_contextify.h index ba964811bb6740..7e5d90459b572f 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -4,7 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "base_object-inl.h" -#include "cppgc_helpers.h" +#include "cppgc_helpers-inl.h" #include "node_context_data.h" #include "node_errors.h" @@ -77,6 +77,7 @@ class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) { public: SET_CPPGC_NAME(ContextifyContext) void Trace(cppgc::Visitor* visitor) const final; + SET_NO_MEMORY_INFO() ContextifyContext(Environment* env, v8::Local wrapper, @@ -204,6 +205,7 @@ class ContextifyScript final : CPPGC_MIXIN(ContextifyScript) { public: SET_CPPGC_NAME(ContextifyScript) void Trace(cppgc::Visitor* visitor) const final; + SET_NO_MEMORY_INFO() ContextifyScript(Environment* env, v8::Local object); ~ContextifyScript() override; diff --git a/src/node_realm-inl.h b/src/node_realm-inl.h index 9eea4e5703e33b..d1c3f18cc6e3c8 100644 --- a/src/node_realm-inl.h +++ b/src/node_realm-inl.h @@ -130,6 +130,11 @@ void Realm::TrackBaseObject(BaseObject* bo) { ++base_object_count_; } +void Realm::TrackCppgcWrapper(CppgcMixin* handle) { + DCHECK_EQ(handle->realm(), this); + cppgc_wrapper_list_.PushFront(handle); +} + void Realm::UntrackBaseObject(BaseObject* bo) { DCHECK_EQ(bo->realm(), this); --base_object_count_; diff --git a/src/node_realm.cc b/src/node_realm.cc index cd2b4c0107594a..c9ea23df32f485 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -35,6 +35,7 @@ void Realm::MemoryInfo(MemoryTracker* tracker) const { #undef V tracker->TrackField("base_object_list", base_object_list_); + tracker->TrackField("cppgc_wrapper_list", cppgc_wrapper_list_); tracker->TrackField("builtins_with_cache", builtins_with_cache); tracker->TrackField("builtins_without_cache", builtins_without_cache); } @@ -216,6 +217,7 @@ void Realm::RunCleanup() { binding_data_store_[i].reset(); } base_object_list_.Cleanup(); + cppgc_wrapper_list_.Cleanup(); } void Realm::PrintInfoForSnapshot() { diff --git a/src/node_realm.h b/src/node_realm.h index be18f39c5bf78e..2c662f14ddb407 100644 --- a/src/node_realm.h +++ b/src/node_realm.h @@ -6,6 +6,7 @@ #include #include #include "cleanup_queue.h" +#include "cppgc_helpers.h" #include "env_properties.h" #include "memory_tracker.h" #include "node_snapshotable.h" @@ -25,6 +26,17 @@ using BindingDataStore = std::array, static_cast(BindingDataType::kBindingDataTypeCount)>; +class CppgcWrapperList + : public ListHead, + public MemoryRetainer { + public: + void Cleanup(); + + SET_MEMORY_INFO_NAME(CppgcWrapperList) + SET_SELF_SIZE(CppgcWrapperList) + void MemoryInfo(MemoryTracker* tracker) const override; +}; + /** * node::Realm is a container for a set of JavaScript objects and functions * that associated with a particular global environment. @@ -113,6 +125,9 @@ class Realm : public MemoryRetainer { // Base object count created after the bootstrap of the realm. inline int64_t base_object_created_after_bootstrap() const; + inline void TrackCppgcWrapper(CppgcMixin* handle); + inline CppgcWrapperList* cppgc_wrapper_list() { return &cppgc_wrapper_list_; } + #define V(PropertyName, TypeName) \ virtual v8::Local PropertyName() const = 0; \ virtual void set_##PropertyName(v8::Local value) = 0; @@ -154,6 +169,7 @@ class Realm : public MemoryRetainer { BindingDataStore binding_data_store_; BaseObjectList base_object_list_; + CppgcWrapperList cppgc_wrapper_list_; }; class PrincipalRealm : public Realm { From bdd34b130d46ffd792cd77dbfaf926cca279648a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 19 Mar 2025 23:05:46 +0100 Subject: [PATCH 2/2] 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 9a1a927891443f..745ecab746f7c7 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 2d09abc19baee2..4e3ffb678c2d09 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 bdaef203ad50ce..fb2af584a4ae77 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 d1c3f18cc6e3c8..f162d1506c990a 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 c9ea23df32f485..b7ac5d74c3b7cc 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 2c662f14ddb407..9ad38be761df06 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);