From fcadb9d212e535cf32ad5172b67ca56eb8a8fdd0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 10 Jun 2026 23:10:39 +0200 Subject: [PATCH 1/4] src: allow tracking children in `MemoryTracker` without edges Allowing the addition of child nodes without an explicit edge between them enables expressing a relationship of the "node A keeps track of node B and no other node does, but node A does not keep node B alive" kind, which is the relationship between `Realm`s and weak `BaseObject` instances. This is a prerequisite for the following commit. Signed-off-by: Anna Henningsen --- src/base_object.cc | 2 ++ src/memory_tracker-inl.h | 29 ++++++++++++++++++----------- src/memory_tracker.h | 8 ++++++++ 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/src/base_object.cc b/src/base_object.cc index 102832c58c62ee..3cae25517c3027 100644 --- a/src/base_object.cc +++ b/src/base_object.cc @@ -180,4 +180,6 @@ void BaseObjectList::MemoryInfo(node::MemoryTracker* tracker) const { } } +const char* const MemoryTracker::kNoEdge = ""; + } // namespace node diff --git a/src/memory_tracker-inl.h b/src/memory_tracker-inl.h index 56f4a180633d39..58bb9e848805c8 100644 --- a/src/memory_tracker-inl.h +++ b/src/memory_tracker-inl.h @@ -131,7 +131,7 @@ void MemoryTracker::TrackField(const char* edge_name, if (value == nullptr) return; auto it = seen_.find(value); if (it != seen_.end()) { - graph_->AddEdge(CurrentNode(), it->second, edge_name); + AddEdge(CurrentNode(), it->second, edge_name); } else { Track(value, edge_name); } @@ -253,9 +253,9 @@ void MemoryTracker::TrackField(const char* edge_name, const v8::Local& value, const char* node_name) { if (!value.IsEmpty()) - graph_->AddEdge(CurrentNode(), - graph_->V8Node(value.template As()), - edge_name); + AddEdge(CurrentNode(), + graph_->V8Node(value.template As()), + edge_name); } template @@ -300,7 +300,7 @@ void MemoryTracker::Track(const CppgcMixin* retainer, const char* edge_name) { auto it = seen_.find(retainer); if (it != seen_.end()) { if (CurrentNode() != nullptr) { - graph_->AddEdge(CurrentNode(), it->second, edge_name); + AddEdge(CurrentNode(), it->second, edge_name); } return; // It has already been tracked, no need to call MemoryInfo again } @@ -317,7 +317,7 @@ void MemoryTracker::Track(const MemoryRetainer* retainer, auto it = seen_.find(retainer); if (it != seen_.end()) { if (CurrentNode() != nullptr) { - graph_->AddEdge(CurrentNode(), it->second, edge_name); + AddEdge(CurrentNode(), it->second, edge_name); } return; // It has already been tracked, no need to call MemoryInfo again } @@ -376,7 +376,7 @@ MemoryRetainerNode* MemoryTracker::AddNode(const CppgcMixin* retainer, MemoryRetainerNode* n = new MemoryRetainerNode(this, retainer); seen_[retainer] = n; if (CurrentNode() != nullptr) { - graph_->AddEdge(CurrentNode(), n->JSWrapperNode(), edge_name); + AddEdge(CurrentNode(), n->JSWrapperNode(), edge_name); } return n; @@ -392,11 +392,11 @@ MemoryRetainerNode* MemoryTracker::AddNode(const MemoryRetainer* retainer, MemoryRetainerNode* n = new MemoryRetainerNode(this, retainer); graph_->AddNode(std::unique_ptr(n)); seen_[retainer] = n; - if (CurrentNode() != nullptr) graph_->AddEdge(CurrentNode(), n, edge_name); + if (CurrentNode() != nullptr) AddEdge(CurrentNode(), n, edge_name); if (n->JSWrapperNode() != nullptr) { - graph_->AddEdge(n, n->JSWrapperNode(), "native_to_javascript"); - graph_->AddEdge(n->JSWrapperNode(), n, "javascript_to_native"); + AddEdge(n, n->JSWrapperNode(), "native_to_javascript"); + AddEdge(n->JSWrapperNode(), n, "javascript_to_native"); } return n; @@ -408,7 +408,7 @@ MemoryRetainerNode* MemoryTracker::AddNode(const char* node_name, MemoryRetainerNode* n = new MemoryRetainerNode(this, node_name, size); graph_->AddNode(std::unique_ptr(n)); - if (CurrentNode() != nullptr) graph_->AddEdge(CurrentNode(), n, edge_name); + if (CurrentNode() != nullptr) AddEdge(CurrentNode(), n, edge_name); return n; } @@ -435,6 +435,13 @@ MemoryRetainerNode* MemoryTracker::PushNode(const char* node_name, return n; } +void MemoryTracker::AddEdge(v8::EmbedderGraph::Node* from, + v8::EmbedderGraph::Node* to, + const char* edge_name) { + if (edge_name == kNoEdge) return; + graph_->AddEdge(from, to, edge_name); +} + void MemoryTracker::PopNode() { node_stack_.pop(); } diff --git a/src/memory_tracker.h b/src/memory_tracker.h index a6c3fee6a847a1..ec5ad35a57ebca 100644 --- a/src/memory_tracker.h +++ b/src/memory_tracker.h @@ -300,6 +300,11 @@ class MemoryTracker { v8::EmbedderGraph* graph) : isolate_(isolate), graph_(graph) {} + // Can be passed to Track() if it is _not_ desirable + // to create an edge between nodes, and the target node is + // only supposed to be added to the graph, not (yet) connected. + static const char* const kNoEdge; + private: typedef std::unordered_map NodeMap; @@ -321,6 +326,9 @@ class MemoryTracker { size_t size, const char* edge_name = nullptr); inline void PopNode(); + inline void AddEdge(v8::EmbedderGraph::Node* from, + v8::EmbedderGraph::Node* to, + const char* edge_name = nullptr); v8::Isolate* isolate_; v8::EmbedderGraph* graph_; From 37649aefec522aa1a41c6e04ed4ff2db9903c977 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 10 Jun 2026 23:17:30 +0200 Subject: [PATCH 2/4] src: do not track weak `BaseObject`s as childrens of `Realm`s Heap dumps are used for analyzing the relationship between objects that keep each other alive, so that retainers of memory can be detected and memory leaks fixed. 6e60ab744e6a13 broke this for a wide range of `BaseObject` instances. Marking all `BaseObject`s, including weak ones, as children of `Realm` and `Environment` instances gives the incorrect impression that they are held alive by those objects, effectively hiding the "real" set of strong roots that keep other objects alive through GC. While I still disagree with the decision in 6e60ab744e6a13 not to mark strong `BaseObject`s as proper roots in the object graph, this commit keeps that decision intact, and only removes edges to weak `BaseObject` instances. Refs: https://github.com/nodejs/node/pull/57417 Signed-off-by: Anna Henningsen --- src/base_object.cc | 6 ++++-- test/common/heap.js | 14 ++++++++++++++ test/pummel/test-heapdump-zlib.js | 14 +++++++++++++- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/base_object.cc b/src/base_object.cc index 3cae25517c3027..b8a691be67c575 100644 --- a/src/base_object.cc +++ b/src/base_object.cc @@ -174,9 +174,11 @@ void BaseObjectList::Cleanup() { } } -void BaseObjectList::MemoryInfo(node::MemoryTracker* tracker) const { +void BaseObjectList::MemoryInfo(MemoryTracker* tracker) const { for (auto bo : *this) { - if (bo->IsDoneInitializing()) tracker->Track(bo); + if (bo->IsDoneInitializing()) + tracker->Track( + bo, bo->persistent().IsWeak() ? MemoryTracker::kNoEdge : nullptr); } } diff --git a/test/common/heap.js b/test/common/heap.js index ac7e090326cd18..6c83ca9978270f 100644 --- a/test/common/heap.js +++ b/test/common/heap.js @@ -336,6 +336,19 @@ function validateByRetainingPath(...args) { return validateByRetainingPathFromNodes(nodes, ...args); } +function getRetainingNodes(startingNode, filter) { + const seen = new Set(); + function listNodes(node) { + if (!filter(node) || seen.has(node)) return; + seen.add(node); + for (const edge of node.incomingEdges) { + listNodes(edge.from); + } + } + listNodes(startingNode); + return [...seen]; +} + module.exports = { recordState, validateSnapshotNodes, @@ -343,4 +356,5 @@ module.exports = { validateByRetainingPathFromNodes, getHeapSnapshotOptionTests, createJSHeapSnapshot, + getRetainingNodes, }; diff --git a/test/pummel/test-heapdump-zlib.js b/test/pummel/test-heapdump-zlib.js index 9f11fbcadad306..f6ca5ceaab899b 100644 --- a/test/pummel/test-heapdump-zlib.js +++ b/test/pummel/test-heapdump-zlib.js @@ -3,7 +3,7 @@ const common = require('../common'); const assert = require('assert'); -const { validateByRetainingPath, validateByRetainingPathFromNodes } = require('../common/heap'); +const { validateByRetainingPath, validateByRetainingPathFromNodes, getRetainingNodes } = require('../common/heap'); const zlib = require('zlib'); // Before zlib stream is created, no ZlibStream should be created. @@ -27,6 +27,18 @@ const gzip = zlib.createGzip(); assert.strictEqual(withMemory.length, 0); } +{ + // Assert that the `ZlibStream` has no unexpected connections + // to other `Node / ...` nodes. + const contexts = validateByRetainingPath('Node / ZlibContext', []); + assert.deepStrictEqual( + getRetainingNodes(contexts[0], (node) => node.name?.startsWith('Node /')) + .map((node) => node.name).sort(), [ + 'Node / ZlibContext', + 'Node / ZlibStream', + ]); +} + // After zlib stream is written, zlib_memory should be created. gzip.write('hello world', common.mustCall(() => { validateByRetainingPath('Node / ZlibStream', [ From 2c10ebf766db625b46ce0937d8ab8dfd32cb8f6e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 11 Jun 2026 15:06:25 +0200 Subject: [PATCH 3/4] fixup! src: do not track weak `BaseObject`s as childrens of `Realm`s --- src/cppgc_helpers.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cppgc_helpers.cc b/src/cppgc_helpers.cc index 4e3ffb678c2d09..8fe3f17c9a68f6 100644 --- a/src/cppgc_helpers.cc +++ b/src/cppgc_helpers.cc @@ -16,7 +16,7 @@ void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const { for (auto node : *this) { CppgcMixin* ptr = node->persistent.Get(); if (ptr != nullptr) { - tracker->Track(ptr); + tracker->Track(ptr, MemoryTracker::kNoEdge); } } } From f0f2ac2f1c52aa700c090d7aba02606834116858 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 11 Jun 2026 15:18:42 +0200 Subject: [PATCH 4/4] fixup! fixup! src: do not track weak `BaseObject`s as childrens of `Realm`s --- src/base_object.cc | 6 +++++- src/cppgc_helpers.cc | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/base_object.cc b/src/base_object.cc index b8a691be67c575..d2799597ac3308 100644 --- a/src/base_object.cc +++ b/src/base_object.cc @@ -176,9 +176,13 @@ void BaseObjectList::Cleanup() { void BaseObjectList::MemoryInfo(MemoryTracker* tracker) const { for (auto bo : *this) { - if (bo->IsDoneInitializing()) + if (bo->IsDoneInitializing()) { + // TODO(addaleax): Add weak edges instead of no edges once + // https://github.com/v8/v8/commit/e37cadf1143a8c5bbe44c0408186b5a26cc23863 + // is available for us tracker->Track( bo, bo->persistent().IsWeak() ? MemoryTracker::kNoEdge : nullptr); + } } } diff --git a/src/cppgc_helpers.cc b/src/cppgc_helpers.cc index 8fe3f17c9a68f6..5d6b68620a2a8f 100644 --- a/src/cppgc_helpers.cc +++ b/src/cppgc_helpers.cc @@ -16,6 +16,9 @@ void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const { for (auto node : *this) { CppgcMixin* ptr = node->persistent.Get(); if (ptr != nullptr) { + // TODO(addaleax): Add weak edges instead of no edges once + // https://github.com/v8/v8/commit/e37cadf1143a8c5bbe44c0408186b5a26cc23863 + // is available for us tracker->Track(ptr, MemoryTracker::kNoEdge); } }