diff --git a/node.gyp b/node.gyp index 726403ae0bb9f4..90182c0ad939e2 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 c8c647e872da3d..fa6d5396113bd6 100644 --- a/src/README.md +++ b/src/README.md @@ -1112,6 +1112,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 @@ -1266,16 +1277,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 @@ -1402,6 +1473,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 @@ -1412,6 +1484,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..745ecab746f7c7 --- /dev/null +++ b/src/cppgc_helpers-inl.h @@ -0,0 +1,65 @@ +#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(); +} + +CppgcMixin::~CppgcMixin() { + if (realm_ != nullptr) { + realm_->set_should_purge_empty_cppgc_wrappers(true); + } +} + +} // 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..4e3ffb678c2d09 --- /dev/null +++ b/src/cppgc_helpers.cc @@ -0,0 +1,37 @@ +#include "cppgc_helpers.h" +#include "env-inl.h" + +namespace node { + +void CppgcWrapperList::Cleanup() { + for (auto node : *this) { + CppgcMixin* ptr = node->persistent.Get(); + if (ptr != nullptr) { + ptr->Finalize(); + } + } +} + +void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const { + 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 20b9004917cfbe..fb2af584a4ae77 100644 --- a/src/cppgc_helpers.h +++ b/src/cppgc_helpers.h @@ -6,14 +6,19 @@ #include // std::remove_reference #include "cppgc/garbage-collected.h" #include "cppgc/name-provider.h" -#include "env.h" +#include "cppgc/persistent.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 CppgcWrapperListNode; + /** * This is a helper mixin with a BaseObject-like interface to help * implementing wrapper objects managed by V8's cppgc (Oilpan) library. @@ -25,20 +30,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 +62,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,8 +82,38 @@ 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. 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) {} + + inline ~CppgcMixin(); + + friend class CppgcWrapperListNode; + private: - Environment* env_; + Realm* realm_ = nullptr; v8::TracedReference traced_reference_; }; @@ -105,7 +129,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 de69c22b0ebaed..ef63dc8fdc282d 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..f162d1506c990a 100644 --- a/src/node_realm-inl.h +++ b/src/node_realm-inl.h @@ -130,6 +130,13 @@ 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(new CppgcWrapperListNode(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 ead7f02f6cbd5a..2791cfc15a8dfe 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()); @@ -35,6 +54,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 +236,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..9ad38be761df06 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,40 @@ 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 MemoryRetainer { + public: + void Cleanup(); + void PurgeEmpty(); + + 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 +148,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; @@ -126,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(); @@ -135,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); @@ -154,6 +206,7 @@ class Realm : public MemoryRetainer { BindingDataStore binding_data_store_; BaseObjectList base_object_list_; + CppgcWrapperList cppgc_wrapper_list_; }; class PrincipalRealm : public Realm {