Move bnoordhuis/node-heapdump to nodejs/node-heapdump

[mhdawson]

I'm thinking it would make sense to move bnoordhuis/node-heapdump to nodejs/node-heapdump like we did for nodereport and llnode. It's a tool commonly used by Node.js users to debug memory issues, and bringing it in might make it easier to get more contributors.

I've already talked to ben and he would be ok with moving.

@nodejs/post-mortem

thoughts, comments ?

[sam-github]

Instead (or maybe in addition?) there could be a v8 binding for TakeHeapSnapshot added to node. If that was done, the rest of the node-heapdump could be written in js, couldn't it, and be left to user-land? And user-land modules to take repeated snapshots over time and then print analyses of them would be easy to write. Basically, this would leave creativity to making, storing, processing snapshots be a js user-land problem, but remove the need for user-land to have to write c++ code to get the snapshots.

[indutny]

👍

[sam-github]

@indutny you like my suggestion, or dawson's? ;-)

[indutny]

Both.

[bnoordhuis]

Instead (or maybe in addition?) there could be a v8 binding for TakeHeapSnapshot added to node.

That doesn't work for large heap snapshots (the JS-ification would OOM) so it would need to be in addition to.

One upside to integrating into core is that we could provide better names for objects created by core. I currently don't bother with that because it's too version-specific.

[joshgav]

@sam-github

there could be a v8 binding for TakeHeapSnapshot added to node

Instead of using a V8 API directly we could use HeapProfiler.takeHeapSnapshot from the Inspector Protocol.

I've been exploring the same for CPU Profiles here, will add a heap snapshot function too.

Perhaps such functions could be added to node-inspect, see nodejs/diagnostics#67.

[mhdawson]

See there was some additional discussion in the latest diagnostics meeting. It would be good to make progress on this one. @joshgav do you have any more info on your experimentation so we could compare the pros/cons of using the inpsector protocol versus the existing heapdump module ?

[joshgav]

@mhdawson still just have the proof of concept for profiling here: https://github.com/joshgav/node-inspect-helpers/blob/master/index.js. Been looking into adding that and heap dump support in node-inspect but haven't yet had enough time. cc @jkrems who was thinking of looking into this too.

If we think it would be best to bring in node-heapdump in the meantime I certainly don't object, can still work on node-inspect in the future.

[sam-github]

My 2 cents: I think adding heapdump (and cpu profile) support to the inspect protocol (and possibly as a v8. API) is a better long-term strategy, node-heapdump is incredibly useful, but we should me moving packages into nodejs/ that are strategic, and we don't have to hurry (unless other approaches stall out).

[jkrems]

Both CPU profiles & heap snapshots are supported in node inspect:

> nvm run 7 inspect examples/empty.js
Running node v7.7.1 (npm v4.1.2)
< Debugger listening on port 9229.
< Warning: This is an experimental feature and could change at any time.
< To start debugging, open the following URL in Chrome:
<     chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:9229/4ecaba06-d20a-42d5-90d5-01b388d1f807
< Debugger attached.
break in examples/empty.js:2
  1 (function (exports, require, module, __filename, __dirname) {
> 2 });
debug> takeHeapSnapshot()
Heap snaphost prepared.
Wrote snapshot: /path/to/node-inspect/node.heapsnapshot
debug> takeHeapSnapshot('my-profile.heapsnapshot')
Heap snaphost prepared.
Wrote snapshot: /path/to/my-profile.heapsnapshot

[sam-github]

OK, so the (EDIT:) "only" missing feature is programmatic request of a heap dump (from js)? /cc @rnchamberlain

[jkrems]

Should the JS API follow the same structure as the inspector API?

var snapshot = v8.createHeapSnapshotStream();
snapshot.pipe(fs.createWriteStream('foo.heapsnapshot'));

That would work around the memory limitations mentioned above. For advanced users they could use some sort of streaming JSON parser to interact with the snapshot.

[sam-github]

that looks like a nice API to me

[mhdawson]

What I discussed with @joshgav at the vm summit last week was pulling it in to core and the PR'ing into the shape that we want in terms of the implementation behind the scenes (ie if it should use the inspector API behind the scenes we can make it do that). As the most widely used module for generating heapdumps it seems like a good starting point which we can then improve as opposed to starting from scratch.

[rnchamberlain]

+1 on the 'both' option, i.e. move current project to nodejs/node-heapdump as it is de facto strategic anyway and needs to live for ever, then look at integrated heapdump/improvements.

There is also #13 - longer term, vm neutral

[mhdawson]

@joshgav @bnoordhuis @sam-github @rnchamberlain @jkrems do you have objecgts to my last proposal of moving it under nodejs and then pr'ing into the shape we want (natively in node, remaing as module etc.).

Next step would be to open issue for TSC review which I'll do unless you object in this issue.

[bnoordhuis]

I'm still fine with handing it off.

[jkrems]

👍 for moving it to nodejs/ (and potentially exposing it from core so it no longer requires a native module dependency). There's definitely value in that.

[mhdawson]

Ok, I've not heard objections so I'm going to open the issue in the TSC repo for the request.

[joshgav]

BTW I added documentation for the heap and profile support available through node-inspect: nodejs/node-inspect#39

[richardlau]

This played out in nodejs/TSC#257 so closing for now. We can always revisit later.