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

util: integrate node-heapdump into core #26501

Closed
wants to merge 2 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 7, 2019

Adds v8.heapdump.getHeapdump() and v8.heapdump.triggerHeapdump(filename) methods with impl adapted from the node-heapdump module.

Not included is the SIGUSR2 signal, which can be handled by userland.

/cc nodejs/diagnostics#279
@nodejs/diagnostics

Also see: #26498

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++. tools Issues and PRs related to the tools directory. labels Mar 7, 2019
doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
tools/license-builder.sh Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Code LGTM

v8.heapdump.getHeapdump() data format is missing in the docs.

doc/api/v8.md Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Mar 8, 2019

Based on some conversations around this, I'm considering just removing the getHeapdump() option and just keeping the write-to-disk option only.

doc/api/v8.md Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Mar 9, 2019

Updated the PR to:

  1. Remove the getHeapdump() function
  2. Added a note indicating that the heapdump file format is V8 specific and intended to be loaded in tools like Chrome DevTools
  3. Added a note about heapdumps being isolate/thread specific along with an example of taking heapdumps of the main thread and worker threads.
  4. Since there is only the one method, changed from v8.heapdump.triggerHeapdump() to just v8.triggerHeapdump()

@bnoordhuis and @richardlau ... may I ask you both to reaffirm your sign-off with the updates.

@mcollina ... may I ask you to review the doc updates to see if they address your concerns.

src/heap_utils.cc Show resolved Hide resolved
src/heap_utils.cc Outdated Show resolved Hide resolved
tools/license-builder.sh Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member Author

jasnell commented Mar 9, 2019 via email

doc/api/v8.md Outdated Show resolved Hide resolved
@vsemozhetbyt vsemozhetbyt added v8 engine Issues and PRs related to the V8 dependency. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 9, 2019
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Some doc nits.

doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
@vsemozhetbyt vsemozhetbyt added the memory Issues and PRs related to the memory management or memory footprint. label Mar 9, 2019
@jasnell
Copy link
Member Author

jasnell commented Mar 10, 2019

@joyeecheung ... PTAL, I added a getHeapdump() variant that returns a stream.Readable

doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
src/heap_utils.cc Outdated Show resolved Hide resolved
src/heap_utils.cc Outdated Show resolved Hide resolved
src/heap_utils.cc Outdated Show resolved Hide resolved
src/heap_utils.cc Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Mar 11, 2019

@vsemozhetbyt @addaleax @richardlau ... PTAL

@jasnell
Copy link
Member Author

jasnell commented Mar 11, 2019

@jasnell
Copy link
Member Author

jasnell commented Mar 12, 2019

CI is good

lib/v8.js Outdated Show resolved Hide resolved
src/heap_utils.cc Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Mar 13, 2019

Ok @joyeecheung ... hopefully that'll be the last few nits fixed. This should be ready to go.

@addaleax
Copy link
Member

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. Thanks for following up with the reviews!

@jasnell
Copy link
Member Author

jasnell commented Mar 13, 2019

Resume for flaky failure:https://ci.nodejs.org/job/node-test-pull-request/21505/

jasnell added a commit that referenced this pull request Mar 13, 2019
Adds `v8.writeHeapSnapshot(filename)` with impl adapted
from the `node-heapdump` module.

Also, adds a v8.getHeapSnapshot() alternative that returns
a Readable Stream

PR-URL: #26501
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Mar 13, 2019

Landed in 5f38797 ... thanks all

@jasnell jasnell closed this Mar 13, 2019
richardlau added a commit to richardlau/node-1 that referenced this pull request Mar 15, 2019
Lazy loading `v8` in `lib/internal/error-serdes.js` reduces the number
of loaded modules by the bootstrap code for Worker threads by seven.

Refs: nodejs#26501 (comment)
pull bot pushed a commit to SimenB/node that referenced this pull request Mar 19, 2019
Lazy loading `v8` in `lib/internal/error-serdes.js` reduces the number
of loaded modules by the bootstrap code for Worker threads by seven.

PR-URL: nodejs#26689
Refs: nodejs#26501 (comment)
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
Adds `v8.writeHeapSnapshot(filename)` with impl adapted
from the `node-heapdump` module.

Also, adds a v8.getHeapSnapshot() alternative that returns
a Readable Stream

PR-URL: nodejs#26501
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
Lazy loading `v8` in `lib/internal/error-serdes.js` reduces the number
of loaded modules by the bootstrap code for Worker threads by seven.

PR-URL: #26689
Refs: #26501 (comment)
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos added a commit that referenced this pull request Mar 27, 2019
Notable changes:

* events:
  * Added a `once` function to use `EventEmitter` with promises
    (#26078).
* tty:
  * Added a `hasColors` method to `WriteStream`
    (#26247).
  * Added NO_COLOR and FORCE_COLOR support
    (#26485).
* v8:
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools
    (#26501).
* meta:
  * Gireesh Punathil is now a member of the Technical Steering Committee
    (#26657).
  * Added ZYSzys to collaborators (#26730).

PR-URL: #26949
targos added a commit that referenced this pull request Mar 28, 2019
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
targos added a commit that referenced this pull request Mar 28, 2019
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
@BethGriggs
Copy link
Member

@jasnell, should this land on v10.x? Please add the lts-watch label if so

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++. memory Issues and PRs related to the memory management or memory footprint. semver-minor PRs that contain new features and should be released in the next minor version. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.