From 459885b23ce0e262f67b04f73261ea168827e928 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 9 Jan 2025 01:19:57 +0100 Subject: [PATCH 1/3] 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. --- src/README.md | 34 ++++++++++++++++++++++++++++++++++ src/cppgc_helpers.h | 42 +++++++++++++++++++++++++++++++++++++----- src/env.cc | 2 ++ src/env.h | 14 ++++++++++++++ 4 files changed, 87 insertions(+), 5 deletions(-) diff --git a/src/README.md b/src/README.md index c8c647e872da3d..a4a10d16a14513 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 `Environment`, it should implement a +pattern like this: + +```cpp + ~MyWrap() { this->Clean(); } + void CleanEnvResource(Environment* env) override { + // Do cleanup that relies on a living Environemnt. + } +``` + `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,6 +1277,8 @@ referrer->Set( ).ToLocalChecked(); ``` +#### 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 @@ -1277,6 +1290,27 @@ 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. +If the cppgc-managed objects does not need to perform any special cleanup, +it's fine to use the default destructor. If it needs to perform only trivial +cleanup that only affects its own members without calling into JS, potentially +triggering garbage collection or relying on a living `Environment`, then it's +fine to just implement the trivial cleanup in the destructor directly. If it +needs to do any substantial cleanup that involves a living `Environment`, because +the destructor can be called at any time by the garbage collection, even after +the `Environment` is already gone, it must implement the cleanup with this pattern: + +```cpp + ~MyWrap() { this->Clean(); } + void CleanEnvResource(Environment* env) override { + // Do cleanup that relies on a living Environemnt. This would be + // called by CppgcMixin::Clean() first during Environment shutdown, + // while the Environment is still alive. If the destructor calls + // Clean() again later during garbage collection that happens after + // Environment shutdown, CleanEnvResource() would be skipped, preventing + // invalid access to the Environment. + } +``` + ### Callback scopes The public `CallbackScope` and the internally used `InternalCallbackScope` diff --git a/src/cppgc_helpers.h b/src/cppgc_helpers.h index 20b9004917cfbe..6e7385e84fef7d 100644 --- a/src/cppgc_helpers.h +++ b/src/cppgc_helpers.h @@ -25,20 +25,30 @@ 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 `Environment`, it should implement a + * pattern like this: + * + * ~MyWrap() { this->Clean(); } + * void CleanEnvResource(Environment* env) override { + * // Do cleanup that relies on a living Environemnt. + * } */ -class CppgcMixin : public cppgc::GarbageCollectedMixin { +class CppgcMixin : public cppgc::GarbageCollectedMixin, + public CppgcWrapperListNode { public: // To help various callbacks access wrapper objects with different memory // management, cppgc-managed objects share the same layout as BaseObjects. @@ -58,6 +68,7 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin { obj->SetAlignedPointerInInternalField( kEmbedderType, env->isolate_data()->embedder_id_for_cppgc()); obj->SetAlignedPointerInInternalField(kSlot, ptr); + env->cppgc_wrapper_list()->PushFront(ptr); } v8::Local object() const { @@ -88,8 +99,29 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin { visitor->Trace(traced_reference_); } + // This implements CppgcWrapperListNode::Clean and is run for all the + // remaining Cppgc wrappers tracked in the Environment during Environment + // 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 Environment during destruction, they should implment it + // in a CleanEnvResource() override, and then call this->Clean() from their + // destructor. Outside of CleanEnvResource(), subclasses should avoid calling + // into JavaScript or perform any operation that can trigger garbage + // collection during the destruction. + void Clean() override { + if (env_ == nullptr) return; + this->CleanEnvResource(env_); + env_ = nullptr; + } + + // The default implementation of CleanEnvResource() is a no-op. Subclasses + // should override it to perform cleanup that require a living Environment, + // instead of doing these cleanups directly in the destructor. + virtual void CleanEnvResource(Environment* env) {} + private: - Environment* env_; + Environment* env_ = nullptr; v8::TracedReference traced_reference_; }; diff --git a/src/env.cc b/src/env.cc index 5d6102dbdb1a20..6e42cdb92f295c 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1302,6 +1302,8 @@ void Environment::RunCleanup() { CleanupHandles(); } + for (CppgcWrapperListNode* handle : cppgc_wrapper_list_) handle->Clean(); + for (const int fd : unmanaged_fds_) { uv_fs_t close_req; uv_fs_close(nullptr, &close_req, fd, nullptr); diff --git a/src/env.h b/src/env.h index 79d263c65c7907..2561e98dd3cabb 100644 --- a/src/env.h +++ b/src/env.h @@ -602,6 +602,12 @@ class Cleanable { friend class Environment; }; +class CppgcWrapperListNode { + public: + virtual void Clean() = 0; + ListNode wrapper_list_node; +}; + /** * Environment is a per-isolate data structure that represents an execution * environment. Each environment has a principal realm. An environment can @@ -897,12 +903,18 @@ class Environment final : public MemoryRetainer { typedef ListHead HandleWrapQueue; typedef ListHead ReqWrapQueue; typedef ListHead CleanableQueue; + typedef ListHead + CppgcWrapperList; inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; } inline CleanableQueue* cleanable_queue() { return &cleanable_queue_; } inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; } + inline CppgcWrapperList* cppgc_wrapper_list() { + return &cppgc_wrapper_list_; + } // https://w3c.github.io/hr-time/#dfn-time-origin inline uint64_t time_origin() { @@ -1191,6 +1203,8 @@ class Environment final : public MemoryRetainer { CleanableQueue cleanable_queue_; HandleWrapQueue handle_wrap_queue_; ReqWrapQueue req_wrap_queue_; + CppgcWrapperList cppgc_wrapper_list_; + int handle_cleanup_waiting_ = 0; int request_waiting_ = 0; From 1f9ac2c1c2a83daa99a066abb3d2e9ee3c9598cf Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 7 Mar 2025 16:54:31 +0100 Subject: [PATCH 2/3] fixup! src: track cppgc wrappers with a list in Realm --- node.gyp | 1 + src/README.md | 95 ++++++++++++++++++++++++++++------------ src/cppgc_helpers-inl.h | 59 +++++++++++++++++++++++++ src/cppgc_helpers.cc | 18 ++++++++ src/cppgc_helpers.h | 94 +++++++++++++++++---------------------- src/env.cc | 2 - src/env.h | 14 ------ 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 +++++++ 13 files changed, 304 insertions(+), 106 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 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 a4a10d16a14513..d9c9c4b84250c5 100644 --- a/src/README.md +++ b/src/README.md @@ -1113,13 +1113,13 @@ 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 `Environment`, it should implement a +cleanup relies on a living Node.js `Realm`, it should implement a pattern like this: ```cpp - ~MyWrap() { this->Clean(); } - void CleanEnvResource(Environment* env) override { - // Do cleanup that relies on a living Environemnt. + ~MyWrap() { this->Finalize(); } + void Clean(Realm* env) override { + // Do cleanup that relies on a living Realm. } ``` @@ -1277,6 +1277,17 @@ 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 @@ -1285,31 +1296,57 @@ 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. - -If the cppgc-managed objects does not need to perform any special cleanup, -it's fine to use the default destructor. If it needs to perform only trivial -cleanup that only affects its own members without calling into JS, potentially -triggering garbage collection or relying on a living `Environment`, then it's -fine to just implement the trivial cleanup in the destructor directly. If it -needs to do any substantial cleanup that involves a living `Environment`, because -the destructor can be called at any time by the garbage collection, even after -the `Environment` is already gone, it must implement the cleanup with this pattern: -```cpp - ~MyWrap() { this->Clean(); } - void CleanEnvResource(Environment* env) override { - // Do cleanup that relies on a living Environemnt. This would be - // called by CppgcMixin::Clean() first during Environment shutdown, - // while the Environment is still alive. If the destructor calls - // Clean() again later during garbage collection that happens after - // Environment shutdown, CleanEnvResource() would be skipped, preventing - // invalid access to the Environment. - } -``` +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 @@ -1436,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 @@ -1446,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 6e7385e84fef7d..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. @@ -39,16 +43,15 @@ namespace node { * } * * If the wrapper needs to perform cleanups when it's destroyed and that - * cleanup relies on a living Node.js `Environment`, it should implement a + * cleanup relies on a living Node.js `Realm`, it should implement a * pattern like this: * - * ~MyWrap() { this->Clean(); } - * void CleanEnvResource(Environment* env) override { + * ~MyWrap() { this->Destroy(); } + * void Clean(Realm* env) override { * // Do cleanup that relies on a living Environemnt. * } */ -class CppgcMixin : public cppgc::GarbageCollectedMixin, - public CppgcWrapperListNode { +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. @@ -58,40 +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); - env->cppgc_wrapper_list()->PushFront(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. @@ -99,30 +80,36 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, visitor->Trace(traced_reference_); } - // This implements CppgcWrapperListNode::Clean and is run for all the - // remaining Cppgc wrappers tracked in the Environment during Environment - // 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 Environment during destruction, they should implment it - // in a CleanEnvResource() override, and then call this->Clean() from their - // destructor. Outside of CleanEnvResource(), subclasses should avoid calling + // 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 Clean() override { - if (env_ == nullptr) return; - this->CleanEnvResource(env_); - env_ = nullptr; + void Finalize() { + if (realm_ == nullptr) return; + this->Clean(realm_); + realm_ = nullptr; } - // The default implementation of CleanEnvResource() is a no-op. Subclasses - // should override it to perform cleanup that require a living Environment, + // 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 CleanEnvResource(Environment* env) {} + virtual void Clean(Realm* realm) {} + + friend class CppgcWrapperList; private: - Environment* env_ = nullptr; + 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 @@ -137,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/env.cc b/src/env.cc index 6e42cdb92f295c..5d6102dbdb1a20 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1302,8 +1302,6 @@ void Environment::RunCleanup() { CleanupHandles(); } - for (CppgcWrapperListNode* handle : cppgc_wrapper_list_) handle->Clean(); - for (const int fd : unmanaged_fds_) { uv_fs_t close_req; uv_fs_close(nullptr, &close_req, fd, nullptr); diff --git a/src/env.h b/src/env.h index 2561e98dd3cabb..79d263c65c7907 100644 --- a/src/env.h +++ b/src/env.h @@ -602,12 +602,6 @@ class Cleanable { friend class Environment; }; -class CppgcWrapperListNode { - public: - virtual void Clean() = 0; - ListNode wrapper_list_node; -}; - /** * Environment is a per-isolate data structure that represents an execution * environment. Each environment has a principal realm. An environment can @@ -903,18 +897,12 @@ class Environment final : public MemoryRetainer { typedef ListHead HandleWrapQueue; typedef ListHead ReqWrapQueue; typedef ListHead CleanableQueue; - typedef ListHead - CppgcWrapperList; inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; } inline CleanableQueue* cleanable_queue() { return &cleanable_queue_; } inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; } - inline CppgcWrapperList* cppgc_wrapper_list() { - return &cppgc_wrapper_list_; - } // https://w3c.github.io/hr-time/#dfn-time-origin inline uint64_t time_origin() { @@ -1203,8 +1191,6 @@ class Environment final : public MemoryRetainer { CleanableQueue cleanable_queue_; HandleWrapQueue handle_wrap_queue_; ReqWrapQueue req_wrap_queue_; - CppgcWrapperList cppgc_wrapper_list_; - int handle_cleanup_waiting_ = 0; int request_waiting_ = 0; 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 { From b405688d5c29bd64f8b465b33f821cd7471fedb0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 14 Feb 2025 20:36:43 +0100 Subject: [PATCH 3/3] crypto: use cppgc to manage Hash --- src/crypto/crypto_hash.cc | 22 ++++++++++++++++----- src/crypto/crypto_hash.h | 17 ++++++++-------- src/crypto/crypto_util.h | 8 +++++++- test/common/heap.js | 33 +++++++++++++++++++------------ test/pummel/test-heapdump-hash.js | 33 +++++++++++++++++++++++++++++++ 5 files changed, 86 insertions(+), 27 deletions(-) create mode 100644 test/pummel/test-heapdump-hash.js 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/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);