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..d9c9c4b84250c5 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. Note that 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..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..3b83ccf1f4bff6 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,18 @@ 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 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 +80,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 +124,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/crypto/crypto_hash.cc b/src/crypto/crypto_hash.cc index 851847483327c1..406768ab0241fc 100644 --- a/src/crypto/crypto_hash.cc +++ b/src/crypto/crypto_hash.cc @@ -1,6 +1,8 @@ #include "crypto/crypto_hash.h" #include "async_wrap-inl.h" #include "base_object-inl.h" +#include "cppgc/allocation.h" +#include "cppgc_helpers-inl.h" #include "env-inl.h" #include "memory_tracker-inl.h" #include "string_bytes.h" @@ -31,14 +33,23 @@ using v8::Object; using v8::Uint32; using v8::Value; +#ifdef ASSIGN_OR_RETURN_UNWRAP +#undef ASSIGN_OR_RETURN_UNWRAP +#endif + +#define ASSIGN_OR_RETURN_UNWRAP ASSIGN_OR_RETURN_UNWRAP_CPPGC namespace crypto { -Hash::Hash(Environment* env, Local wrap) : BaseObject(env, wrap) { - MakeWeak(); +Hash::Hash(Environment* env, Local wrap) { + CppgcMixin::Wrap(this, env, wrap); +} + +void Hash::Trace(cppgc::Visitor* visitor) const { + CppgcMixin::Trace(visitor); } void Hash::MemoryInfo(MemoryTracker* tracker) const { - tracker->TrackFieldWithSize("mdctx", mdctx_ ? kSizeOf_EVP_MD_CTX : 0); - tracker->TrackFieldWithSize("md", digest_ ? md_len_ : 0); + tracker->TrackFieldWithSize("mdctx", mdctx_ ? kSizeOf_EVP_MD_CTX : 0, "EVP_MD_CTX"); + tracker->TrackFieldWithSize("md", digest_ ? md_len_ : 0, "ByteSource"); } #if OPENSSL_VERSION_MAJOR >= 3 @@ -322,7 +333,8 @@ void Hash::New(const FunctionCallbackInfo& args) { xof_md_len = Just(args[1].As()->Value()); } - Hash* hash = new Hash(env, args.This()); + Hash* hash = cppgc::MakeGarbageCollected( + env->isolate()->GetCppHeap()->GetAllocationHandle(), env, args.This()); if (md == nullptr || !hash->HashInit(md, xof_md_len)) { return ThrowCryptoError(env, ERR_get_error(), "Digest method not supported"); diff --git a/src/crypto/crypto_hash.h b/src/crypto/crypto_hash.h index 85da86dba98d5a..59c5c5221c4cca 100644 --- a/src/crypto/crypto_hash.h +++ b/src/crypto/crypto_hash.h @@ -3,7 +3,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -#include "base_object.h" +#include "cppgc_helpers.h" #include "crypto/crypto_keys.h" #include "crypto/crypto_util.h" #include "env.h" @@ -12,15 +12,16 @@ namespace node { namespace crypto { -class Hash final : public BaseObject { + +class Hash final : CPPGC_MIXIN(Hash) { public: + SET_CPPGC_NAME(Hash) + void Trace(cppgc::Visitor* visitor) const final; + void MemoryInfo(MemoryTracker* tracker) const override; + static void Initialize(Environment* env, v8::Local target); static void RegisterExternalReferences(ExternalReferenceRegistry* registry); - void MemoryInfo(MemoryTracker* tracker) const override; - SET_MEMORY_INFO_NAME(Hash) - SET_SELF_SIZE(Hash) - bool HashInit(const EVP_MD* md, v8::Maybe xof_md_len); bool HashUpdate(const char* data, size_t len); @@ -28,13 +29,13 @@ class Hash final : public BaseObject { static void GetCachedAliases(const v8::FunctionCallbackInfo& args); static void OneShotDigest(const v8::FunctionCallbackInfo& args); + Hash(Environment* env, v8::Local wrap); + protected: static void New(const v8::FunctionCallbackInfo& args); static void HashUpdate(const v8::FunctionCallbackInfo& args); static void HashDigest(const v8::FunctionCallbackInfo& args); - Hash(Environment* env, v8::Local wrap); - private: ncrypto::EVPMDCtxPointer mdctx_{}; unsigned int md_len_ = 0; diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h index 8015e1ab00dab6..365345e691c8bc 100644 --- a/src/crypto/crypto_util.h +++ b/src/crypto/crypto_util.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "async_wrap.h" +#include "cppgc_helpers.h" #include "env.h" #include "node_errors.h" #include "node_external_reference.h" @@ -74,7 +75,12 @@ void Decode(const v8::FunctionCallbackInfo& args, void (*callback)(T*, const v8::FunctionCallbackInfo&, const char*, size_t)) { T* ctx; - ASSIGN_OR_RETURN_UNWRAP(&ctx, args.This()); + if constexpr (std::is_base_of_v) { + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.This()); + } else { + ctx = CppgcMixin::Unwrap(args.This()); + if (ctx == nullptr) return; + } if (args[0]->IsString()) { StringBytes::InlineDecoder decoder; 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..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 ead7f02f6cbd5a..dc2eab32028977 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 { diff --git a/test/common/heap.js b/test/common/heap.js index bec3b3208c0295..bf0d95fa6f145e 100644 --- a/test/common/heap.js +++ b/test/common/heap.js @@ -231,11 +231,13 @@ function validateSnapshotNodes(...args) { * node_type?: string, * edge_type?: string, * }]} retainingPath The retaining path specification to search from the root nodes. + * @param {boolean} allowEmpty Whether the function should fail if no matching nodes can be found. + * * @returns {[object]} All the leaf nodes matching the retaining path specification. If none can be found, * logs the nodes found in the last matching step of the path (if any), and throws an * assertion error. */ -function findByRetainingPath(rootName, retainingPath) { +function findByRetainingPath(rootName, retainingPath, allowEmpty = false) { const nodes = createJSHeapSnapshot(); let haystack = nodes.filter((n) => n.name === rootName && n.type !== 'string'); @@ -269,19 +271,23 @@ function findByRetainingPath(rootName, retainingPath) { } if (newHaystack.length === 0) { - const format = (val) => util.inspect(val, { breakLength: 128, depth: 3 }); - console.error('#'); - console.error('# Retaining path to search for:'); - for (let j = 0; j < retainingPath.length; ++j) { - console.error(`# - '${format(retainingPath[j])}'${i === j ? '\t<--- not found' : ''}`); - } - console.error('#\n'); - console.error('# Nodes found in the last step include:'); - for (let j = 0; j < haystack.length; ++j) { - console.error(`# - '${format(haystack[j])}`); + if (allowEmpty) { + return []; + } else { + const format = (val) => util.inspect(val, { breakLength: 128, depth: 3 }); + console.error('#'); + console.error('# Retaining path to search for:'); + for (let j = 0; j < retainingPath.length; ++j) { + console.error(`# - '${format(retainingPath[j])}'${i === j ? '\t<--- not found' : ''}`); + } + console.error('#\n'); + console.error('# Nodes found in the last step include:'); + for (let j = 0; j < haystack.length; ++j) { + console.error(`# - '${format(haystack[j])}`); + } + + assert.fail(`Could not find target edge ${format(expected)} in the heap snapshot.`); } - - assert.fail(`Could not find target edge ${format(expected)} in the heap snapshot.`); } haystack = newHaystack; @@ -326,4 +332,5 @@ module.exports = { validateSnapshotNodes, findByRetainingPath, getHeapSnapshotOptionTests, + createJSHeapSnapshot, }; diff --git a/test/pummel/test-heapdump-hash.js b/test/pummel/test-heapdump-hash.js new file mode 100644 index 00000000000000..3328bd3ed90e6b --- /dev/null +++ b/test/pummel/test-heapdump-hash.js @@ -0,0 +1,33 @@ +'use strict'; +require('../common'); +const { findByRetainingPath, createJSHeapSnapshot } = require('../common/heap'); +const { createHash } = require('crypto'); +const assert = require('assert'); + +// In case the bootstrap process creates any Hash objects, capture a snapshot first +// and save the initial length. +const originalNodes = findByRetainingPath('Node / Hash', [ + { edge_name: 'mdctx' }, +], true); + +const count = 5; +const arr = []; +for (let i = 0; i < count; ++i) { + arr.push(createHash('sha1')); +} + +const nodesWithCtx = findByRetainingPath('Node / Hash', [ + { edge_name: 'mdctx', node_name: 'Node / EVP_MD_CTX' }, +]); + +assert.strictEqual(nodesWithCtx.length - originalNodes.length, count); + +for (let i = 0; i < count; ++i) { + arr[i].update('test').digest('hex'); +} + +const nodesWithDigest = findByRetainingPath('Node / Hash', [ + { edge_name: 'md', node_name: 'Node / ByteSource' }, +]); + +assert.strictEqual(nodesWithDigest.length - originalNodes.length, count);