From bafd5444abc03f093b98160dfff1866163e52e8d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 9 Jun 2020 17:04:41 +0200 Subject: [PATCH] src: do not track BaseObjects via cleanup hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since f59ec2abee, `BaseObject` instances were tracked in heap snapshots through their associated `CleanupHookCallback`s which were stored on the `Environment`; however, this is inaccurate, because: - Edges in heap dumps imply a keeps-alive relationship, but cleanup hooks do not keep the `BaseObject`s that they point to alive. - It loses information about whether `BaseObject` instances are GC roots: Even weak `BaseObject`s are now, practically speaking, showing up as hanging off a GC root when that isn’t actually the case (e.g. in the description of nodejs/node#33468). Thus, this is a partial revert of f59ec2abee. Refs: https://github.com/nodejs/node/issues/33468 Refs: https://github.com/nodejs/node/pull/27018 PR-URL: https://github.com/nodejs/node/pull/33809 Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- src/base_object.h | 1 + src/env.cc | 32 ++++++++---------------- src/memory_tracker.h | 7 ------ test/pummel/test-heapdump-env.js | 42 -------------------------------- 4 files changed, 11 insertions(+), 71 deletions(-) diff --git a/src/base_object.h b/src/base_object.h index 03c69976ee4a68..674eae1f0f7766 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -103,6 +103,7 @@ class BaseObject : public MemoryRetainer { private: v8::Local WrappedObject() const override; + bool IsRootNode() const override; static void DeleteMe(void* data); // persistent_handle_ needs to be at a fixed offset from the start of the diff --git a/src/env.cc b/src/env.cc index 7591ed1c435d82..677faf093a8c1f 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1030,33 +1030,16 @@ Environment* Environment::worker_parent_env() const { return worker_context()->env(); } -void MemoryTracker::TrackField(const char* edge_name, - const CleanupHookCallback& value, - const char* node_name) { - HandleScope handle_scope(isolate_); - // Here, we utilize the fact that CleanupHookCallback instances - // are all unique and won't be tracked twice in one BuildEmbedderGraph - // callback. - MemoryRetainerNode* n = - PushNode("CleanupHookCallback", sizeof(value), edge_name); - // TODO(joyeecheung): at the moment only arguments of type BaseObject will be - // identified and tracked here (based on their deleters), - // but we may convert and track other known types here. - BaseObject* obj = value.GetBaseObject(); - if (obj != nullptr && obj->IsDoneInitializing()) { - TrackField("arg", obj); - } - CHECK_EQ(CurrentNode(), n); - CHECK_NE(n->size_, 0); - PopNode(); -} - void Environment::BuildEmbedderGraph(Isolate* isolate, EmbedderGraph* graph, void* data) { MemoryTracker tracker(isolate, graph); Environment* env = static_cast(data); tracker.Track(env); + env->ForEachBaseObject([&](BaseObject* obj) { + if (obj->IsDoneInitializing()) + tracker.Track(obj); + }); } inline size_t Environment::SelfSize() const { @@ -1083,7 +1066,8 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("should_abort_on_uncaught_toggle", should_abort_on_uncaught_toggle_); tracker->TrackField("stream_base_state", stream_base_state_); - tracker->TrackField("cleanup_hooks", cleanup_hooks_); + tracker->TrackFieldWithSize( + "cleanup_hooks", cleanup_hooks_.size() * sizeof(CleanupHookCallback)); tracker->TrackField("async_hooks", async_hooks_); tracker->TrackField("immediate_info", immediate_info_); tracker->TrackField("tick_info", tick_info_); @@ -1124,4 +1108,8 @@ Local BaseObject::WrappedObject() const { return object(); } +bool BaseObject::IsRootNode() const { + return !persistent_handle_.IsWeak(); +} + } // namespace node diff --git a/src/memory_tracker.h b/src/memory_tracker.h index 4a66e9ce74ccc5..1896624bbeb9dd 100644 --- a/src/memory_tracker.h +++ b/src/memory_tracker.h @@ -216,13 +216,6 @@ class MemoryTracker { inline void TrackField(const char* edge_name, const v8::BackingStore* value, const char* node_name = nullptr); - // We do not implement CleanupHookCallback as MemoryRetainer - // but instead specialize the method here to avoid the cost of - // virtual pointers. - // TODO(joyeecheung): do this for BaseObject and remove WrappedObject() - void TrackField(const char* edge_name, - const CleanupHookCallback& value, - const char* node_name = nullptr); inline void TrackField(const char* edge_name, const uv_buf_t& value, const char* node_name = nullptr); diff --git a/test/pummel/test-heapdump-env.js b/test/pummel/test-heapdump-env.js index 8175797cce4f1e..7030271469c512 100644 --- a/test/pummel/test-heapdump-env.js +++ b/test/pummel/test-heapdump-env.js @@ -5,7 +5,6 @@ require('../common'); const { validateSnapshotNodes } = require('../common/heap'); -const assert = require('assert'); // This is just using ContextifyScript as an example here, it can be replaced // with any BaseObject that we can easily instantiate here and register in @@ -16,51 +15,10 @@ const context = require('vm').createScript('const foo = 123'); validateSnapshotNodes('Node / Environment', [{ children: [ - cleanupHooksFilter, { node_name: 'Node / cleanup_hooks', edge_name: 'cleanup_hooks' }, { node_name: 'process', edge_name: 'process_object' }, { node_name: 'Node / IsolateData', edge_name: 'isolate_data' }, ] }]); -function cleanupHooksFilter(edge) { - if (edge.name !== 'cleanup_hooks') { - return false; - } - if (edge.to.type === 'native') { - verifyCleanupHooksInSnapshot(edge.to); - } else { - verifyCleanupHooksInGraph(edge.to); - } - return true; -} - -function verifyCleanupHooksInSnapshot(node) { - assert.strictEqual(node.name, 'Node / cleanup_hooks'); - const baseObjects = []; - for (const hook of node.outgoingEdges) { - for (const hookEdge of hook.to.outgoingEdges) { - if (hookEdge.name === 'arg') { - baseObjects.push(hookEdge.to); - } - } - } - // Make sure our ContextifyScript show up. - assert(baseObjects.some((node) => node.name === 'Node / ContextifyScript')); -} - -function verifyCleanupHooksInGraph(node) { - assert.strictEqual(node.name, 'Node / cleanup_hooks'); - const baseObjects = []; - for (const hook of node.edges) { - for (const hookEdge of hook.to.edges) { - if (hookEdge.name === 'arg') { - baseObjects.push(hookEdge.to); - } - } - } - // Make sure our ContextifyScript show up. - assert(baseObjects.some((node) => node.name === 'Node / ContextifyScript')); -} - console.log(context); // Make sure it's not GC'ed