Skip to content
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

Closed
wants to merge 4 commits into from

Conversation

joyeecheung
Copy link
Member

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 / ).

Comparison in Chrome DevTools

Node / Http2Session

Before:

screen shot 2018-09-24 at 10 55 22 pm

After:

screen shot 2018-09-24 at 11 05 42 pm

Node / FileHandleReadWrap

Before:

screen shot 2018-09-24 at 11 06 33 pm

After:

screen shot 2018-09-24 at 11 05 18 pm

Node / JSBindingConnection

Before:

screen shot 2018-09-24 at 11 07 28 pm

After:

screen shot 2018-09-24 at 11 07 19 pm

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 25, 2018
@joyeecheung
Copy link
Member Author

joyeecheung commented Sep 25, 2018

@joyeecheung
Copy link
Member Author

cc @addaleax

tracker->TrackThis(this);
}
SET_NO_MEMORY_INFO()
SET_MEMORY_INFO_NAME(AsyncWrapObject);
Copy link
Member

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()
Copy link
Member

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.

Copy link
Member Author

@joyeecheung joyeecheung Sep 25, 2018

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.

Copy link
Member

@addaleax addaleax left a 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!

if (graph.CreateObject().ToLocal(&ret))
args.GetReturnValue().Set(ret);
// Crash if we cannot build a proper graph
args.GetReturnValue().Set(graph.CreateObject().ToLocalChecked());
Copy link
Member

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?

Copy link
Member Author

@joyeecheung joyeecheung Sep 25, 2018

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

@@ -5,25 +5,33 @@

#include "memory_tracker.h"

#define DEFAULT_NODE_NAME \
(node_name == nullptr ? (edge_name == nullptr ? "" : edge_name) : node_name)
Copy link
Member

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 Show resolved Hide resolved
// Shift the self size of this container out to a separate node
CurrentNode()->size_ -= sizeof(T);
}
PushNode(DEFAULT_NODE_NAME, sizeof(T), edge_name);
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

@joyeecheung joyeecheung Sep 25, 2018

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

Copy link
Member

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 👍

auto it = seen_.find(retainer);
if (it != seen_.end()) {
n = it->second;
return n;
Copy link
Member

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?

* // 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);
Copy link
Member

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 … ;)

Copy link
Member Author

@joyeecheung joyeecheung Sep 25, 2018

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 / `).
@joyeecheung
Copy link
Member Author

Rebased and addressed comments. PTAL @addaleax @devsnek

https://ci.nodejs.org/job/node-test-pull-request/17583/

@joyeecheung
Copy link
Member Author

CI is happy and so am I

* std::vector<uv_async_t> vector;
* node::Persistent<Object> target;
*
* node::Persistent<Object> wrapped;
Copy link
Member

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 :)

@joyeecheung
Copy link
Member Author

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.

@hashseed
Copy link
Member

hashseed commented Oct 3, 2018

@ulan

}
return "";
}

class MemoryRetainerNode : public v8::EmbedderGraph::Node {
public:
explicit inline MemoryRetainerNode(MemoryTracker* tracker,
Copy link
Contributor

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.

@ulan
Copy link
Contributor

ulan commented Oct 4, 2018

V8 related parts lgtm

@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

Landed in 92fa0fc

@joyeecheung joyeecheung closed this Oct 4, 2018
joyeecheung added a commit that referenced this pull request Oct 4, 2018
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 <[email protected]>
@joyeecheung
Copy link
Member Author

It should be backportable to v10.x

@joyeecheung
Copy link
Member Author

Nope, it doesn't, because it relies on #22106 which breaks the ABI

@addaleax
Copy link
Member

addaleax commented Oct 4, 2018

@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?

@joyeecheung
Copy link
Member Author

@addaleax Maybe I could...by modify the calls to actual AddEdge. I will try it later.

@TimothyGu
Copy link
Member

@joyeecheung This PR added an empty src/txt file. Is that intended?

@joyeecheung
Copy link
Member Author

@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

joyeecheung added a commit that referenced this pull request Oct 5, 2018
PR-URL: #23273
Refs: #23072
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Oct 5, 2018
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 <[email protected]>
targos pushed a commit that referenced this pull request Oct 7, 2018
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 <[email protected]>
targos pushed a commit that referenced this pull request Oct 7, 2018
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 <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
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 <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23273
Refs: #23072
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants