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

Heapdump generation as part of Node core #279

Closed
mhdawson opened this issue Mar 7, 2019 · 13 comments
Closed

Heapdump generation as part of Node core #279

mhdawson opened this issue Mar 7, 2019 · 13 comments

Comments

@mhdawson
Copy link
Member

mhdawson commented Mar 7, 2019

Was discussed in Diagnostic summit today. Consensus was that ability to generate heapdumps should be in core versus needing modules like Heapdump.

We should work on

  • Pulling sampling heap profiler into core
  • Offering ability to generate heapdump as part of core APIs and on events like Unhandled Exception, etc. through command line options.

Lets use this issue to flesh out the APIs' functionality that should be exposed.

@mhdawson
Copy link
Member Author

mhdawson commented Mar 7, 2019

For references, older issue with some past discussions on the topic: nodejs/TSC#257

@sam-github
Copy link

Note that the core capability is already present:

const fs = require('fs');
const inspector = require('inspector');
const session = new inspector.Session();

session.connect();

fs.writeFileSync('_.heapsnapshot', '');

session.on('HeapProfiler.addHeapSnapshotChunk', function(m) {
  fs.appendFileSync('_.heapsnapshot', m.params.chunk);
});

session.post('HeapProfiler.takeHeapSnapshot', null, function(err, r) {
  console.log('Runtime.takeHeapSnapshot done:', err, r)
  session.disconnect();
});

It probably needs an example usage, like: https://nodejs.org/api/inspector.html#inspector_cpu_profiler

@jasnell
Copy link
Member

jasnell commented Mar 7, 2019

I'll be opening a pr that essentially pulls in node-heapdump ... At least most of it. Expect that soonish

@misterdjules
Copy link

@jasnell My experience with node-heapdump has been that very often, it's not usable because it requires the process to be able to allocate as much memory as what is used for the JS heap, and that leads to the process actually crashing. Has there been further discussions during the summit about solving that problem?

I know for instance that there's been discussions in the past about being able to generate heapdumps from core dumps (e.g. in nodejs/post-mortem#13).

I'm not suggesting that this should block moving node-heapdump into node's core, but I'm wondering whether we should also think about a different approach that doesn't have node-heapdump's issues.

That being said, I'm also interested in hearing about the experience that others had with node-heapdump, there may be something that I'm missing that would solve the issues that I've experienced.

@jasnell
Copy link
Member

jasnell commented Mar 7, 2019 via email

@misterdjules
Copy link

@jasnell

There will be parallel efforts to see what is possible around using coredump.

Also just wanted to make it clear that I'm not saying generating heap dumps from core dumps is necessarily what I'm looking for. I'm looking for a robust/reliable way to generate data that gives insights into the content of the JS heap, whether this is done via core dumps or another mechanism is an implementation detail for me.

@joyeecheung
Copy link
Member

joyeecheung commented Mar 7, 2019

Previous discussions stalled on the format in which the heap dump should be returned to the user land: nodejs/node#23328

Note that we already have internalBinding('heap_utils').createHeapDump() that returns the heap snapshot as a JSON.parsed object. So if we are exposing this functionality to the users I'd expect we work on top of that instead of pulling in node-heapdump, which mostly just adds some convenient signal handlers and a file writer for that.

@naugtur
Copy link
Contributor

naugtur commented Mar 7, 2019

The convenience of using signals by just adding a --require heapdump when running existing code without modification was nice though. Being able to do that with a flag (because someone might be using signals for something else I suppose) would be a nice equivalent.
If it doesn't warrant adding a flag, it makes sense to keep a simplistic heapdump module out there to enable the functionality.

@alexkozy
Copy link
Member

I'd like to mention CLI tools that I built over last weekends. It utilizes inspector protocol to capture different profiles and tracing for given node process, it supports child processes, ondemand profiling and will support workers as well soon. It can capture sampling heap profiler.
Please take a look: https://github.com/ak239/thetool . It is npm installable module with only one dependency (update-notifier).

@vmarchaud
Copy link
Contributor

I believe this can be closed since both the heap sampling profiler and the heapsnapshot can be launched with Node core.

@mhdawson
Copy link
Member Author

@vmarchaud would you happen to have the link to the PR for adding the sampling profiler? It would be good to have that in this issue and then I think I can close.

@vmarchaud
Copy link
Contributor

@mhdawson Here is it : nodejs/node#27596

@mhdawson
Copy link
Member Author

@vmarchaud thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants