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

inspector: implement --heap-prof #27596

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
8 participants
@joyeecheung
Copy link
Member

commented May 7, 2019

In addition implements --heap-prof-name, --heap-prof-dir and
--heap-prof-interval.
These flags are similar to --cpu-prof flags but they are meant
for the V8 sampling heap profiler instead of the CPU profiler.

Fixes: #27421

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

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@nodejs-github-bot

This comment has been minimized.

@jkrems

jkrems approved these changes May 8, 2019

Copy link
Contributor

left a comment

LGTM

Show resolved Hide resolved src/node_options.cc
@ofrobots
Copy link
Contributor

left a comment

Thanks for working on getting this feature into Node! Is there a reason we are plumbing this over the intespector rather than using the V8 API directly? There will be less overhead if we were to use the v8-profiler.h API directly.

Show resolved Hide resolved src/node_options.h Outdated
Show resolved Hide resolved doc/api/cli.md Outdated
Show resolved Hide resolved doc/node.1 Outdated
@ofrobots

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

It would be quite useful to expose a JS API for this as well. Would you consider doing that as part of this PR? BTW, that might be one of the stronger reasons to avoid implementing this on top of the Inspector API. The v8-profiler.h has the ability to query the profile at various points during the execution of the program. The inspector has an oversight in their API today: that the only way to get a profile back is by stopping profiling. When profiling is stopped, all currently tracked objects are drained, which means that subsequent profiles won't have them represented in the profile.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Thanks for working on getting this feature into Node! Is there a reason we are plumbing this over the intespector rather than using the V8 API directly? There will be less overhead if we were to use the v8-profiler.h API directly.

Because the lack of serialization v8-profiler.h. I think it's best to implement this in V8 and make sure the serialization code is tested there, instead of maintaining the serialization by ourselves, since it could get broken by upstream changes and it's best that the upstream fixes it within the CL that breaks it (theoretically v8 would notice as they also run Node.js tests but that would be a much longer roundtrip).

I opened https://bugs.chromium.org/p/v8/issues/detail?id=9182 and I am working on it, but before that happens and gets backported, I think it's fine to use the inspector protocol as a start. The flags are all experimental at the moment.

It would be quite useful to expose a JS API for this as well. Would you consider doing that as part of this PR?

I would prefer to do that in a future PR, as we may need a more thought-out API design for this. See #27421 (comment). What I had in mind was something like v8.heapProflier.start({samplingInterval: 1024}) and v8.heapProflier.stop() but I also see that users may prefer a more powerful API that handles triggers and serializations for these (see #27421 (comment) for an example)

BTW, that might be one of the stronger reasons to avoid implementing this on top of the Inspector API. The v8-profiler.h has the ability to query the profile at various points during the execution of the program. The inspector has an oversight in their API today: that the only way to get a profile back is by stopping profiling. When profiling is stopped, all currently tracked objects are drained, which means that subsequent profiles won't have them represented in the profile.

Thanks for the explanation, I think with the current CLI interface, we could just rewrite the internals after https://bugs.chromium.org/p/v8/issues/detail?id=9182 gets fixed? At the moment if when the process exits, the profiler has already been stopped, it would just fail silently with a message printed to the stderr.

@ofrobots

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@joyeecheung can you elaborate on the JSON.parse/serialize issue you mention in https://bugs.chromium.org/p/v8/issues/detail?id=9182. I am not sure I am following. (Apologies in advance for asking for explanation, it is quite possible that I am missing something obvious in the light of the following).

FYI, we have a user-space implementation of the heap profiler here: https://github.com/google/pprof-nodejs/blob/master/ts/src/heap-profiler.ts. This is usable for production workloads with ongoing monitoring. The API doesn't do any JSON encoding, and provides direct access to the profile tree to user-space. Converting the result to DevTools format is quite simple, see here.

return env()->heap_prof_name();
}

MaybeLocal<Object> V8HeapProfilerConnection::GetProfile(Local<Object> result) {

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung May 10, 2019

Author Member

@ofrobots See here and V8ProfilerConnection:ParseProfile(). Because the response returned from the protocol is:

{
  "result": {
    "profile": {
      // ... what we need to write to the file
    }
  }
}

We need to do JSON::Parse and then JSON::Stringify to get to the profile. It would be better if the profile is formatted directly from the C++ data and since we already know about the schema of the returned object, we can serialize it more efficiently (similar to how heap snapshots are serialized in the API). That may be useful for a particularly big profile.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

FYI, we have a user-space implementation of the heap profiler here: https://github.com/google/pprof-nodejs/blob/master/ts/src/heap-profiler.ts. This is usable for production workloads with ongoing monitoring. The API doesn't do any JSON encoding, and provides direct access to the profile tree to user-space.

Thanks for the information! Although I believe in the end if the user wants to serialize the object, the would still need to do a JSON.stringify() on the object returned (though this time in JS). It does not need JSON.parse() like what we do here for the response returned by the inspector, as the profile returned by the API is not nested in a bigger object.

I think the question here is whether the user wants to consume the profile as a JS object in-memory, or whether they just want to serialize the profile and write it / send it to somewhere else. I believe in most cases, the user would want to do the analysis offline, so simply writing the serialized profile would be more common.

@ak239

This comment has been minimized.

Copy link
Member

commented May 10, 2019

I moved my comment about using inspector instead of separate flag to #27421. Please take a look on it before merging.

joyeecheung and others added some commits Apr 23, 2019

inspector: implement --heap-prof
In addition implements --heap-prof-name, --heap-prof-dir and
--heap-prof-interval.
These flags are similar to --cpu-prof flags but they are meant
for the V8 sampling heap profiler instead of the CPU profiler.
fixup! inspector: implement --heap-prof
Co-Authored-By: Ali Ijaz Sheikh <ofrobots@google.com>

@joyeecheung joyeecheung force-pushed the joyeecheung:heap-prof branch from 595c4ed to da3cfad May 20, 2019

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Updated the docs and bumped the default sampling interval to 512KiB. @ofrobots PTAL.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented May 25, 2019

Ping @ofrobots

If there are no more reviews, I plan to land this later this week.

@addaleax

This comment has been minimized.

Copy link
Member

commented May 26, 2019

Landed in 4b74dae

@addaleax addaleax closed this May 26, 2019

addaleax added a commit that referenced this pull request May 26, 2019

inspector: implement --heap-prof
In addition implements --heap-prof-name, --heap-prof-dir and
--heap-prof-interval.
These flags are similar to --cpu-prof flags but they are meant
for the V8 sampling heap profiler instead of the CPU profiler.

PR-URL: #27596
Fixes: #27421
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

targos added a commit that referenced this pull request May 28, 2019

inspector: implement --heap-prof
In addition implements --heap-prof-name, --heap-prof-dir and
--heap-prof-interval.
These flags are similar to --cpu-prof flags but they are meant
for the V8 sampling heap profiler instead of the CPU profiler.

PR-URL: #27596
Fixes: #27421
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@targos targos added the semver-minor label Jun 3, 2019

targos added a commit that referenced this pull request Jun 3, 2019

2019-06-04, Version 12.4.0 (Current)
Notable changes:

* doc:
  * The JSON variant of the API documentation is no longer experimental
    (Rich Trott) #27842.
* esm:
  * JSON module support is always enabled under
    `--experimental-modules`. The `--experimental-json-modules` flag
    has been removed (Myles Borins)
    #27752.
* http,http2:
  * A new flag has been added for overriding the default HTTP server
    socket timeout (which is two minutes). Pass
    `--http-server-default-timeout=milliseconds`
    or `--http-server-default-timeout=0` to respectively change or
    disable the timeout. Starting with Node.js 13.0.0, the timeout will
    be disabled by default
    (Ali Ijaz Sheikh) #27704.
* inspector:
  * Added an experimental `--heap-prof` flag to start the V8 heap
    profiler on startup and write the heap profile to disk before exit
    (Joyee Cheung) #27596.
* stream:
  * The `readable.unshift()` method now correctly converts strings to
    buffers. Additionally, a new optional argument is accepted to
    specify the string's encoding, such as `'utf8'` or `'ascii'`
    (Marcos Casagrande) #27194.
* v8:
  * The object returned by `v8.getHeapStatistics()` has two new
    properties: `number_of_native_contexts` and
    `number_of_detached_contexts` (Yuriy Vasiyarov)
    #27933.

PR-URL: TODO

@targos targos referenced this pull request Jun 3, 2019

Merged

v12.4.0 release proposal #28040

targos added a commit that referenced this pull request Jun 3, 2019

2019-06-04, Version 12.4.0 (Current)
Notable changes:

* doc:
  * The JSON variant of the API documentation is no longer experimental
    (Rich Trott) #27842.
* esm:
  * JSON module support is always enabled under
    `--experimental-modules`. The `--experimental-json-modules` flag
    has been removed (Myles Borins)
    #27752.
* http,http2:
  * A new flag has been added for overriding the default HTTP server
    socket timeout (which is two minutes). Pass
    `--http-server-default-timeout=milliseconds`
    or `--http-server-default-timeout=0` to respectively change or
    disable the timeout. Starting with Node.js 13.0.0, the timeout will
    be disabled by default
    (Ali Ijaz Sheikh) #27704.
* inspector:
  * Added an experimental `--heap-prof` flag to start the V8 heap
    profiler on startup and write the heap profile to disk before exit
    (Joyee Cheung) #27596.
* stream:
  * The `readable.unshift()` method now correctly converts strings to
    buffers. Additionally, a new optional argument is accepted to
    specify the string's encoding, such as `'utf8'` or `'ascii'`
    (Marcos Casagrande) #27194.
* v8:
  * The object returned by `v8.getHeapStatistics()` has two new
    properties: `number_of_native_contexts` and
    `number_of_detached_contexts` (Yuriy Vasiyarov)
    #27933.

PR-URL: #28040

pull bot pushed a commit to Rachelmorrell/node that referenced this pull request Jun 4, 2019

2019-06-04, Version 12.4.0 (Current)
Notable changes:

* doc:
  * The JSON variant of the API documentation is no longer experimental
    (Rich Trott) nodejs#27842.
* esm:
  * JSON module support is always enabled under
    `--experimental-modules`. The `--experimental-json-modules` flag
    has been removed (Myles Borins)
    nodejs#27752.
* http,http2:
  * A new flag has been added for overriding the default HTTP server
    socket timeout (which is two minutes). Pass
    `--http-server-default-timeout=milliseconds`
    or `--http-server-default-timeout=0` to respectively change or
    disable the timeout. Starting with Node.js 13.0.0, the timeout will
    be disabled by default
    (Ali Ijaz Sheikh) nodejs#27704.
* inspector:
  * Added an experimental `--heap-prof` flag to start the V8 heap
    profiler on startup and write the heap profile to disk before exit
    (Joyee Cheung) nodejs#27596.
* stream:
  * The `readable.unshift()` method now correctly converts strings to
    buffers. Additionally, a new optional argument is accepted to
    specify the string's encoding, such as `'utf8'` or `'ascii'`
    (Marcos Casagrande) nodejs#27194.
* v8:
  * The object returned by `v8.getHeapStatistics()` has two new
    properties: `number_of_native_contexts` and
    `number_of_detached_contexts` (Yuriy Vasiyarov)
    nodejs#27933.

PR-URL: nodejs#28040
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.