Skip to content

Commit

Permalink
src: do not track BaseObjects via cleanup hooks
Browse files Browse the repository at this point in the history
Since f59ec2a, `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 #33468).

Thus, this is a partial revert of f59ec2a.

Refs: #33468
Refs: #27018

PR-URL: #33809
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
addaleax committed Jun 11, 2020
1 parent dfdbbd1 commit bafd544
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 71 deletions.
1 change: 1 addition & 0 deletions src/base_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ class BaseObject : public MemoryRetainer {

private:
v8::Local<v8::Object> 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
Expand Down
32 changes: 10 additions & 22 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<Environment*>(data);
tracker.Track(env);
env->ForEachBaseObject([&](BaseObject* obj) {
if (obj->IsDoneInitializing())
tracker.Track(obj);
});
}

inline size_t Environment::SelfSize() const {
Expand All @@ -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_);
Expand Down Expand Up @@ -1124,4 +1108,8 @@ Local<Object> BaseObject::WrappedObject() const {
return object();
}

bool BaseObject::IsRootNode() const {
return !persistent_handle_.IsWeak();
}

} // namespace node
7 changes: 0 additions & 7 deletions src/memory_tracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
42 changes: 0 additions & 42 deletions test/pummel/test-heapdump-env.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

0 comments on commit bafd544

Please sign in to comment.