-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
src: name EmbededderGraph edges and use class names for nodes #23072
Conversation
cc @addaleax |
src/async_wrap.cc
Outdated
tracker->TrackThis(this); | ||
} | ||
SET_NO_MEMORY_INFO() | ||
SET_MEMORY_INFO_NAME(AsyncWrapObject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra semicolon?
void MemoryInfo(MemoryTracker* tracker) const override { | ||
tracker->TrackThis(this); | ||
} | ||
SET_NO_MEMORY_INFO() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seeing as these three macros are used together quite a lot, maybe there's a better way they could be composed? seems like this introduces some opportunities for forgetfulness and ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devsnek These are pure virtual functions, if the implementer does not implement them a compilation error would be thrown, so I am not sure how this can introduce forgetfulness and ambiguity (if anything, before this patch not all of them were pure virtual functions so that's where forgetfulness and ambiguity used to be possible). The macros are just helpers for the most common cases - some classes, like TCPWrap
, would implement their own methods for better debuggability.
cef975b
to
80f852a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good so far!
src/heap_utils.cc
Outdated
if (graph.CreateObject().ToLocal(&ret)) | ||
args.GetReturnValue().Set(ret); | ||
// Crash if we cannot build a proper graph | ||
args.GetReturnValue().Set(graph.CreateObject().ToLocalChecked()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What’s the reason for this change? If the return value of CreateObject()
is empty, that means we already have a scheduled exception, so returning to JS would be okay, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into this in tests and it somehow returned an undefined so I got an undefined embeddedGraph that can’t be called with filter...hmm now that I come to think of it maybe there was a bug in CreateObject that returned an empty Maybe even when there was no pending exception, the bug is likely gone now
src/memory_tracker-inl.h
Outdated
@@ -5,25 +5,33 @@ | |||
|
|||
#include "memory_tracker.h" | |||
|
|||
#define DEFAULT_NODE_NAME \ | |||
(node_name == nullptr ? (edge_name == nullptr ? "" : edge_name) : node_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid this being a macro? I think I’d prefer a function that takes edge_name
and node_name
arguments, even if that is a bit more to spell out…
src/memory_tracker-inl.h
Outdated
// Shift the self size of this container out to a separate node | ||
CurrentNode()->size_ -= sizeof(T); | ||
} | ||
PushNode(DEFAULT_NODE_NAME, sizeof(T), edge_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines here seem to make the assumption that the outer Node always already accounts for the memory of the outer container… I don’t think that’s guaranteed, though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a comment in the API would be enough? Or a boolean field in case the implementer doesn’t use sizeof as the SelfSize, then they will need to figure out the calculation (so far I haven’t seen any implementer with a container member that doesn’t do this). I believe before this patch all of the container member sizes got double-counted anyway so this just makes the common case a bit more accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, maybe we can lift the size 0 constraint and make the container node a size 0 node then it won’t get double counted..(though I am not sure how it looks in the DevTools..I am 30% sure some internal Nodes in v8 already do this)
EDIT: hmm, nope, I looked into the generator code a bit and I don't think V8 does that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, does that mean that V8 just doesn’t support nodes of size 0…? I guess in that case this is the best possible way to do this 👍
src/memory_tracker-inl.h
Outdated
auto it = seen_.find(retainer); | ||
if (it != seen_.end()) { | ||
n = it->second; | ||
return n; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe return it->second;
, and move the declaration of n
to where it’s initialized below?
src/memory_tracker.h
Outdated
* // elements in the container appear as grandchildren nodes | ||
* tracker->TrackField("vector", this->vector_field); | ||
* // Node name and size come from the JS object | ||
* tracker->TrackField("target", this->target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for going with this->
in the examples / no trailing underscores? It’s not exactly our existing code style … ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I should’ve polished this a bit more...it’s more like a copy-paste from a draft, I will tweak the style
This patch: - Refactors the `MemoryRetainer` API so that the impementer no longer calls `TrackThis()` that sets the size of node on the top of the stack, which may be hard to understand. Instead they implements `SelfSize()` to provide their self sizes. Also documents the API in the header. - Refactors `MemoryTracker` so it calls `MemoryInfoName()` and `SelfSize()` of `MemoryRetainer` to retrieve info about them, and separate `node_names` and `edge_names` so the edges can be properly named with reference names and the nodes can be named with class names. (Previously the nodes are named with reference names while the edges are all indexed and appear as array elements). - Adds `SET_MEMORY_INFO_NAME()`, `SET_SELF_SIZE()` and `SET_NO_MEMORY_INFO()` convenience macros - Fixes a few `MemoryInfo` calls in some `MemoryRetainers` to track their references properly. - Refactors the heapdump tests to check both node names and edge names, distinguishing between wrapped JS nodes (without prefixes) and embedder wrappers (prefixed with `Node / `).
80f852a
to
cd07235
Compare
CI is happy and so am I |
src/memory_tracker.h
Outdated
* std::vector<uv_async_t> vector; | ||
* node::Persistent<Object> target; | ||
* | ||
* node::Persistent<Object> wrapped; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny nit: We use trailing _
for non-static class members :)
Fixed nits. CI: https://ci.nodejs.org/job/node-test-pull-request/17600/ (Not sure who to cc, we don't have a team for V8 integrations, do we? Tentatively cc @nodejs/v8 ) I plan to land this in 24 hours unless there are more reviews to address. |
src/memory_tracker-inl.h
Outdated
} | ||
return ""; | ||
} | ||
|
||
class MemoryRetainerNode : public v8::EmbedderGraph::Node { | ||
public: | ||
explicit inline MemoryRetainerNode(MemoryTracker* tracker, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "explicit" is not needed here and below.
V8 related parts lgtm |
Landed in 92fa0fc |
This patch: - Refactors the `MemoryRetainer` API so that the impementer no longer calls `TrackThis()` that sets the size of node on the top of the stack, which may be hard to understand. Instead now they implements `SelfSize()` to provide their self sizes. Also documents the API in the header. - Refactors `MemoryTracker` so it calls `MemoryInfoName()` and `SelfSize()` of `MemoryRetainer` to retrieve info about them, and separate `node_names` and `edge_names` so the edges can be properly named with reference names and the nodes can be named with class names. (Previously the nodes are named with reference names while the edges are all indexed and appear as array elements). - Adds `SET_MEMORY_INFO_NAME()`, `SET_SELF_SIZE()` and `SET_NO_MEMORY_INFO()` convenience macros - Fixes a few `MemoryInfo` calls in some `MemoryRetainers` to track their references properly. - Refactors the heapdump tests to check both node names and edge names, distinguishing between wrapped JS nodes (without prefixes) and embedder wrappers (prefixed with `Node / `). PR-URL: #23072 Reviewed-By: Anna Henningsen <anna@addaleax.net>
It should be backportable to v10.x |
Nope, it doesn't, because it relies on #22106 which breaks the ABI |
@joyeecheung Just wondering, because this touches so much code all over the place … would it be worth looking into doing an ABI-compatible backport of #22106? |
@addaleax Maybe I could...by modify the calls to actual AddEdge. I will try it later. |
@joyeecheung This PR added an empty src/txt file. Is that intended? |
@TimothyGu that was accidental, sorry. I am away from my laptop now, feel free open a PR to remove it or I will do it later |
This patch: - Refactors the `MemoryRetainer` API so that the impementer no longer calls `TrackThis()` that sets the size of node on the top of the stack, which may be hard to understand. Instead now they implements `SelfSize()` to provide their self sizes. Also documents the API in the header. - Refactors `MemoryTracker` so it calls `MemoryInfoName()` and `SelfSize()` of `MemoryRetainer` to retrieve info about them, and separate `node_names` and `edge_names` so the edges can be properly named with reference names and the nodes can be named with class names. (Previously the nodes are named with reference names while the edges are all indexed and appear as array elements). - Adds `SET_MEMORY_INFO_NAME()`, `SET_SELF_SIZE()` and `SET_NO_MEMORY_INFO()` convenience macros - Fixes a few `MemoryInfo` calls in some `MemoryRetainers` to track their references properly. - Refactors the heapdump tests to check both node names and edge names, distinguishing between wrapped JS nodes (without prefixes) and embedder wrappers (prefixed with `Node / `). PR-URL: nodejs#23072 Reviewed-By: Anna Henningsen <anna@addaleax.net>
This patch: - Refactors the `MemoryRetainer` API so that the impementer no longer calls `TrackThis()` that sets the size of node on the top of the stack, which may be hard to understand. Instead now they implements `SelfSize()` to provide their self sizes. Also documents the API in the header. - Refactors `MemoryTracker` so it calls `MemoryInfoName()` and `SelfSize()` of `MemoryRetainer` to retrieve info about them, and separate `node_names` and `edge_names` so the edges can be properly named with reference names and the nodes can be named with class names. (Previously the nodes are named with reference names while the edges are all indexed and appear as array elements). - Adds `SET_MEMORY_INFO_NAME()`, `SET_SELF_SIZE()` and `SET_NO_MEMORY_INFO()` convenience macros - Fixes a few `MemoryInfo` calls in some `MemoryRetainers` to track their references properly. - Refactors the heapdump tests to check both node names and edge names, distinguishing between wrapped JS nodes (without prefixes) and embedder wrappers (prefixed with `Node / `). Backport-PR-URL: #23295 PR-URL: #23072 Reviewed-By: Anna Henningsen <anna@addaleax.net>
This patch: - Refactors the `MemoryRetainer` API so that the impementer no longer calls `TrackThis()` that sets the size of node on the top of the stack, which may be hard to understand. Instead now they implements `SelfSize()` to provide their self sizes. Also documents the API in the header. - Refactors `MemoryTracker` so it calls `MemoryInfoName()` and `SelfSize()` of `MemoryRetainer` to retrieve info about them, and separate `node_names` and `edge_names` so the edges can be properly named with reference names and the nodes can be named with class names. (Previously the nodes are named with reference names while the edges are all indexed and appear as array elements). - Adds `SET_MEMORY_INFO_NAME()`, `SET_SELF_SIZE()` and `SET_NO_MEMORY_INFO()` convenience macros - Fixes a few `MemoryInfo` calls in some `MemoryRetainers` to track their references properly. - Refactors the heapdump tests to check both node names and edge names, distinguishing between wrapped JS nodes (without prefixes) and embedder wrappers (prefixed with `Node / `). Backport-PR-URL: #23295 PR-URL: #23072 Reviewed-By: Anna Henningsen <anna@addaleax.net>
This patch: - Refactors the `MemoryRetainer` API so that the impementer no longer calls `TrackThis()` that sets the size of node on the top of the stack, which may be hard to understand. Instead now they implements `SelfSize()` to provide their self sizes. Also documents the API in the header. - Refactors `MemoryTracker` so it calls `MemoryInfoName()` and `SelfSize()` of `MemoryRetainer` to retrieve info about them, and separate `node_names` and `edge_names` so the edges can be properly named with reference names and the nodes can be named with class names. (Previously the nodes are named with reference names while the edges are all indexed and appear as array elements). - Adds `SET_MEMORY_INFO_NAME()`, `SET_SELF_SIZE()` and `SET_NO_MEMORY_INFO()` convenience macros - Fixes a few `MemoryInfo` calls in some `MemoryRetainers` to track their references properly. - Refactors the heapdump tests to check both node names and edge names, distinguishing between wrapped JS nodes (without prefixes) and embedder wrappers (prefixed with `Node / `). PR-URL: #23072 Reviewed-By: Anna Henningsen <anna@addaleax.net>
This patch:
MemoryRetainer
API so that the impementer no longercalls
TrackThis()
that sets the size of node on the top of the stack,which may be hard to understand. Instead they implements
SelfSize()
to provide their self sizes. Also documents the API in the header.
MemoryTracker
so it callsMemoryInfoName()
andSelfSize()
of
MemoryRetainer
to retrieve info about them, and separatenode_names
andedge_names
so the edges can be properly named withreference names and the nodes can be named with class names.
(Previously the nodes are named with reference names while the edges
are all indexed and appear as array elements).
SET_MEMORY_INFO_NAME()
,SET_SELF_SIZE()
andSET_NO_MEMORY_INFO()
convenience macrosMemoryInfo
calls in someMemoryRetainers
to tracktheir references properly.
distinguishing between wrapped JS nodes (without prefixes)
and embedder wrappers (prefixed with
Node /
).Comparison in Chrome DevTools
Node / Http2Session
Before:
After:
Node / FileHandleReadWrap
Before:
After:
Node / JSBindingConnection
Before:
After:
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes