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: use V8 graph heap snapshot API #21741

Closed
wants to merge 6 commits into from
Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jul 10, 2018

Addresses a pending V8 deprecation and generally makes our heap snapshot memory reporting much more accurate. I’ll cherry-pick the first commit into its own PR (edit: #21742). Also, since this is somewhat large-ish, I’d recommend per-commit reviewing :)

  • src: enable more detailed memory tracking

    This will enable more detailed heap snapshots based on
    a newer V8 API.

    This commit itself is not tied to that API and could
    be backported.

  • src: add iteration over all base objects to Environment

  • Cherry-pick v8/v8@555c811

  • src: use V8 graph heap snapshot API

    Transition to a newer, more flexible API for
    heap snapshot creation.

    This addresses a currently pending deprecation in the V8 API.

    Fixes: Introduce BuildEmbedderGraphCallback #21633

  • src: make heap snapshot & embedder graph accessible for tests

    Add methods that allow inspection of heap snapshots and a JS
    version of our own embedder graph.

    These can be used in tests and might also prove useful for
    ad-hoc debugging. Usage requires --expose-internals and
    prints a warning similar to our other modules whose primary
    purpose is test support.

  • test: add heap snapshot tests

    Add a number of tests that validate that heap snapshots
    contain what we expect them to contain, and cross-check
    against a JS version of our own embedder graphs.

/cc @targos @joyeecheung @danbev @nodejs/diagnostics (and maybe @Trott as somebody to look at the last commit?)

@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. memory Issues and PRs related to the memory management or memory footprint. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 10, 2018
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 10, 2018
@Trott
Copy link
Member

Trott commented Jul 10, 2018

@nodejs/testing on the b5e3c32 commit. LGTM from a quick scan. Will try to look a bit closer later but a review on that from @apapirovski, @BridgeAR, @cjihrig, or @refack might be better than a review from me.

@addaleax
Copy link
Member Author

addaleax commented Jul 10, 2018

@@ -0,0 +1,232 @@
#include "node_internals.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to have to be in src? Can we create a directory in test that specifically build this as an addon so we don't need to build it into the binary and use internalBinding to load it?

Copy link
Member Author

@addaleax addaleax Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does use Environment::BuildEmbedderGraph … would we want to expose that instead?

Edit: Also, I was kinda wondering whether buildEmbedderGraph() could be useful for ad-hoc debugging … even with it printing a warning, I think this might sometimes be useful for inspecting Node?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax It would be convenient to have helper methods to take a heap dump and get the subgraph of embedder nodes/edges, yes. (Hopefully no one in the user land would attempt to abuse them even with the warning..)

let type = types[j];
if (Array.isArray(type)) {
item[name] = type[raw[i + j]];
} else if (type === 'string_or_number') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name === 'name_or_index' seems more natural? This basically implies that item is an edge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

ChannelWrap* channel;
ares_socket_t sock;
uv_poll_t poll_watcher;

void MemoryInfo(MemoryTracker* tracker) const override;
Copy link
Member

@joyeecheung joyeecheung Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these subclasses of MemoryRetainer override their MemoryInfoName() as well? If I am reading the code correctly, this prefers using the class names of the target nodes as edge names?

EDIT: the EmbedderGraph API doesn't seem to allow you to specify a name for the edge, even though the class HeapGraphEdge defined in v8-profiler.h has something like that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am reading the code correctly, this prefers using the class names of the target nodes as edge names?

Yes… I don’t think it makes much difference in this case, at least as long as we can’t name edges?

{
edges: [
{ name: 'FSReqPromise' },
{ name: 'Float64Array' } // Stat array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am reading the code correctly...this means that there will be edges named with class names? Aren't those supposed to be something like property names? (e.g. something like fs_stats_field_array here)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you edited above, we don’t currently have any ways to name the edges as embedders :/

Copy link
Member

@joyeecheung joyeecheung Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax Yeah..I looked into the source a bit, looks like those edges all appear as element edges with auto-incremented indexes (which is kind of odd because then our nodes would appear somewhat like arrays, whereas other internal nodes in V8 usually have internal edges with more meaningful names), it's the validation here that use the target node names for inputs.

I opened a feature request in the upstream: https://bugs.chromium.org/p/v8/issues/detail?id=7938 but until it's sorted out it's still worth landing this with indexed edges IMO. It's better than our current integration anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but until it's sorted out it's still worth landing this with indexed edges IMO

@joyeecheung Can you explain, what does “indexed edges” mean here? i.e. how is it different from what this PR currently does? Or am I missing something? 😄

Copy link
Member

@joyeecheung joyeecheung Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax The current EmbedderGraph API will create edges with type kElement and with indexes that are automatically incremented (based on the count of edges of the from node). The internal V8 edges, for example, the ones pointing to shared function info or context nodes from JS functions, have type kInternal and corresponding string names. That's why those references show up in the DevTools as gray and will have proper names displayed (otherwise you may see a function has a reference named with a number to a shared function info node, instead of a reference named shared, which is odd because you rarely see a function with a number-keyed property and you couldn't tell if it's added by you in the JS land).

Copy link
Member

@joyeecheung joyeecheung Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax Yeah, if I understand correctly the snapshots generated with the current approach will show up with number indexed references from the embedder nodes to other nodes (e.g. it looks as if FSREQPROMISE[1] = Float64Array(...) instead of FSREQPROMISE.stats_field_array = Float64Array(...), the members will have their names overridden from Float64Array to stats_field_array), which would look a bit confusing but still it's better than no references tracked in the snapshots at all.

I opened https://chromium-review.googlesource.com/c/v8/v8/+/926362 BTW

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened https://chromium-review.googlesource.com/c/v8/v8/+/926362 BTW

@joyeecheung Tbh, I don’t really understand the semantics of NamePrefix either. :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@joyeecheung joyeecheung Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screen shot 2018-07-12 at 12 36 22 am

By the way this is what I meant by "indexed edges", which are also created by the deprecated RetainedObjectInfo. Making those numbers human-readable names would help users navigate in the DevTools.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, yeah, makes sense – not the end of the world but would definitely be very nice to have.

@addaleax addaleax force-pushed the heap branch 2 times, most recently from 044d423 to 01f050d Compare July 11, 2018 16:24
@addaleax
Copy link
Member Author

@joyeecheung Should we wait for your CL to be merged, and then update this? I think that makes sense at this point?

if (expectedNode.edges) {
for (const expectedEdge of expectedNode.edges) {
const check = typeof expectedEdge === 'function' ?
expectedEdge : (node) => node.name === expectedEdge.name;
Copy link
Member

@joyeecheung joyeecheung Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit confusing to name that expectedEdge.name since the name is actually from the target node and not the edge itself. Can we change the format of expected a bit to be clearer? I think referring to those as children and child would be enough?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, sounds good. Done!

While addressing this, it occurred to me that expectedChild may or may not be a sensible variable name to use (because of the real-world usage of that term), but I’ll leave it that way until somebody has something better. :)

@joyeecheung
Copy link
Member

joyeecheung commented Jul 12, 2018

Should we wait for your CL to be merged, and then update this? I think that makes sense at this point?

@addaleax I don't think we need to wait until the CL is merged, since currently the snapshot generated from Node.js have indexed edges anyway and this is still an improvement since it tracks more stuff and gets rid of usage of a deprecated API. The naming of the edges can be left as a future exercise.

}
}

const char* Name() override { return name_.c_str(); }
Copy link
Member

@joyeecheung joyeecheung Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to override NamePrefix() as well? Like using Node / so our nodes would appear with names Node / Zlib and they would be more distinguishable that way (the node names are more like class names instead of reference names). V8 tend to use system / as a prefix for internal nodes like contexts and Chromium sometimes use Window / (although they seem to be just prepending the prefixes manually instead of using this API).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, done! :)

@addaleax
Copy link
Member Author

addaleax commented Jul 12, 2018

Should we wait for your CL to be merged, and then update this? I think that makes sense at this point?

@addaleax I don't think we need to wait until the CL is merged, since currently the snapshot generated from Node.js have indexed edges anyway and this is still an improvement since it tracks more stuff and gets rid of usage of a deprecated API. The naming of the edges can be left as a future exercise.

@joyeecheung I’m asking because we do need to adjust src/heap_utils.cc when your CL lands, and in particular V8 will have to update their copy of Node to account for it. It’s trivial to work around the method signature change, but, you know, might be easier that way? Otherwise I completely agree. :)

And in any case, thanks for working through this with me! 🙂

(Edit: Just remembering, your CL is ABI-breaking. We could ignore that but maybe we shouldn’t.)

kisg pushed a commit to paul99/v8mips that referenced this pull request Jul 12, 2018
This patch adds a variant of EmbedderGraph::AddEdge() which
allows the embedder to specify the name of an edge. The edges
added without name are element edges with auto-incremented indexes
while the edges added with names will be internal edges with
the specified names for more meaningful output in the heap
snapshot.

Refs: nodejs/node#21741
Bug: v8:7938
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
Change-Id: I8feefa2cf6911743e24b3b2024e0e849b0c65cd3
Reviewed-on: https://chromium-review.googlesource.com/1133299
Commit-Queue: Ulan Degenbaev <[email protected]>
Reviewed-by: Ulan Degenbaev <[email protected]>
Cr-Commit-Position: refs/heads/master@{#54412}
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit. Thanks for working on this!

```js
validateSnapshotNodes('TLSWRAP', [
{
edges: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now children

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done!

@joyeecheung
Copy link
Member

@addaleax Regarding the ABI breakage: I think if we want stay on the safe side, we'd better cherry-pick the CL and use the new signature in another PR and mark it as dont-land-on-v10.x? I couldn't think of a way to avoid the breakage given how EmbedderGraph::Node is designed :(

@addaleax addaleax force-pushed the heap branch 2 times, most recently from 8a0cf5c to e354714 Compare July 13, 2018 13:00
@@ -538,6 +539,41 @@ Returns the result of
Returns the result of
`fs.readFileSync(path.join(fixtures.fixturesDir, 'keys', arg), 'enc')`.

## Heap dump cheker module

This provides utilities for checking the validity of heap dumps.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment that this requires --expose-internals

'use strict';
const assert = require('assert');
const util = require('util');
const { createJSHeapDump, buildEmbedderGraph } = require('internal/test/heap');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add:

try {
  require.resolve('internal/test/heap');
} catch (e) {
  console.log("using `test/common/heap.js` requires `--expose-internals`");
  throw e;
}

@addaleax
Copy link
Member Author

@refack Thanks for the review, done in 175b272426a7b9e5fe228316adb0ade613aab997!

@refack
Copy link
Contributor

refack commented Jul 13, 2018

👍 seems like a very helpful tool.

Regarding the ABI breakage

Doesn't that necessitate semver-major?

@addaleax
Copy link
Member Author

@refack The ABI breakage was referring to v8/v8@6ee8345, and we decided against including it for that reason :)

@targos targos mentioned this pull request Jul 17, 2018
targos pushed a commit to targos/node that referenced this pull request Jul 25, 2018
Original commit message:

    [api] Switch from `SetBuildEmbedderGraphCallback` to `AddBuildEmbedderGraphCallback`

    `SetBuildEmbedderGraphCallback`, unlike `SetWrapperClassInfoProvider`,
    assumes a monolithic embedder that can provide all necessary information.
    That is not the case for e.g. Node.js, which can e.g. provide multiple Node.js
    instances per V8 Isolate, as well as native addons that may allocate resources
    on their own.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Ib53dfde82416dd69934b08623e27d674a483ac2d
    Reviewed-on: https://chromium-review.googlesource.com/1082441
    Commit-Queue: Ulan Degenbaev <[email protected]>
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#53545}

Refs: v8/v8@555c811

PR-URL: nodejs#21741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message:

    [api] Switch from `SetBuildEmbedderGraphCallback` to `AddBuildEmbedderGraphCallback`

    `SetBuildEmbedderGraphCallback`, unlike `SetWrapperClassInfoProvider`,
    assumes a monolithic embedder that can provide all necessary information.
    That is not the case for e.g. Node.js, which can e.g. provide multiple Node.js
    instances per V8 Isolate, as well as native addons that may allocate resources
    on their own.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Ib53dfde82416dd69934b08623e27d674a483ac2d
    Reviewed-on: https://chromium-review.googlesource.com/1082441
    Commit-Queue: Ulan Degenbaev <[email protected]>
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#53545}

Refs: v8/v8@555c811

PR-URL: nodejs#21741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message:

    [api] Switch from `SetBuildEmbedderGraphCallback` to `AddBuildEmbedderGraphCallback`

    `SetBuildEmbedderGraphCallback`, unlike `SetWrapperClassInfoProvider`,
    assumes a monolithic embedder that can provide all necessary information.
    That is not the case for e.g. Node.js, which can e.g. provide multiple Node.js
    instances per V8 Isolate, as well as native addons that may allocate resources
    on their own.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Ib53dfde82416dd69934b08623e27d674a483ac2d
    Reviewed-on: https://chromium-review.googlesource.com/1082441
    Commit-Queue: Ulan Degenbaev <[email protected]>
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#53545}

Refs: v8/v8@555c811

PR-URL: nodejs#21741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 10, 2018
Original commit message:

    [api] Switch from `SetBuildEmbedderGraphCallback` to `AddBuildEmbedderGraphCallback`

    `SetBuildEmbedderGraphCallback`, unlike `SetWrapperClassInfoProvider`,
    assumes a monolithic embedder that can provide all necessary information.
    That is not the case for e.g. Node.js, which can e.g. provide multiple Node.js
    instances per V8 Isolate, as well as native addons that may allocate resources
    on their own.

    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: Ib53dfde82416dd69934b08623e27d674a483ac2d
    Reviewed-on: https://chromium-review.googlesource.com/1082441
    Commit-Queue: Ulan Degenbaev <[email protected]>
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#53545}

Refs: v8/v8@555c811

Backport-PR-URL: #21668
PR-URL: #21741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Aug 17, 2018
Original commit message:

    [heap-profiler] Allow embedder to specify edge names

    This patch adds a variant of EmbedderGraph::AddEdge() which
    allows the embedder to specify the name of an edge. The edges
    added without name are element edges with auto-incremented indexes
    while the edges added with names will be internal edges with
    the specified names for more meaningful output in the heap
    snapshot.

    Refs: nodejs#21741
    Bug: v8:7938
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I8feefa2cf6911743e24b3b2024e0e849b0c65cd3
    Reviewed-on: https://chromium-review.googlesource.com/1133299
    Commit-Queue: Ulan Degenbaev <[email protected]>
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54412}

Refs: v8/v8@6ee8345
joyeecheung added a commit that referenced this pull request Aug 18, 2018
Original commit message:

    [heap-profiler] Allow embedder to specify edge names

    This patch adds a variant of EmbedderGraph::AddEdge() which
    allows the embedder to specify the name of an edge. The edges
    added without name are element edges with auto-incremented indexes
    while the edges added with names will be internal edges with
    the specified names for more meaningful output in the heap
    snapshot.

    Refs: #21741
    Bug: v8:7938
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I8feefa2cf6911743e24b3b2024e0e849b0c65cd3
    Reviewed-on: https://chromium-review.googlesource.com/1133299
    Commit-Queue: Ulan Degenbaev <[email protected]>
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#54412}

Refs: v8/v8@6ee8345

PR-URL: #22106
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this pull request Sep 23, 2018
We have migrated from the deprecated RetainedObjectInfo API to
the new EmbedderGraph API, so there is no need to take care
of wrapper class ids anymore since they are dedicated to the
deprecated API (the new API uses a graph instead of ids to retrieve
info about nodes).

PR-URL: #22975
Refs: #21741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Sep 24, 2018
We have migrated from the deprecated RetainedObjectInfo API to
the new EmbedderGraph API, so there is no need to take care
of wrapper class ids anymore since they are dedicated to the
deprecated API (the new API uses a graph instead of ids to retrieve
info about nodes).

PR-URL: #22975
Refs: #21741
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. memory Issues and PRs related to the memory management or memory footprint. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants