v8: Expose statistics about heap spaces #4463

bripkens · 2015-12-29T10:57:39Z

Provide means to inspect information about the separate heap spaces via
a callable API. This is helpful to analyze memory issues.

As discussed with @bnoordhuis in #2079, this adds the ability to inspect heap spaces via a new function in the v8 module:

v8.getHeapSpaceStatistics()

Fixes #2079

cjihrig · 2015-12-29T15:50:49Z

test/parallel/test-v8-stats.js

+const actualHeapSpaceNames = heapSpaceStatistics.map(s => s.space_name);
+assert.deepEqual(actualHeapSpaceNames.sort(), expectedHeapSpaces.sort());
+heapSpaceStatistics.forEach(heapSpace => {
+  assert.equal(typeof heapSpace.space_name, 'string');


Could you change these to assert.strictEqual().

bnoordhuis · 2015-12-29T17:03:54Z

The current approach isn't wrong but I would advocate a slightly more efficient approach.

The number of heap spaces is fixed (i.e. the return value of v8::Isolate::NumberOfHeapSpaces() doesn't change over the lifetime of the program) so I would suggest allocating a Uint32Array upfront that is big enough to hold all HeapSpaceStatistics records (similar to how v8.getHeapStatistics() is implemented), then have lib/v8.js turn that into an array of objects; that way V8 can optimize array and object creation.

Uint32Array may no longer be the right choice now that V8 can create heaps > 4 GB but we can fix that up later. For now, I'd argue for consistency.

bripkens · 2015-12-29T17:21:48Z

@bnoordhuis / @cjihrig: Thanks for the review, I updated the PR.

@bnoordhuis: I will look into this optimisation. Since my C++ skills are real rusty, this could a while :).

ofrobots · 2015-12-29T17:25:32Z

@bnoordhuis I am not sure if the optimization is worth it. I don't imagine a scenario where the performance of v8.getHeapSpaceStatistics should matter enough for it to be worth it. I would argue for simplicity. Just my $0.02.

bnoordhuis · 2015-12-29T17:44:10Z

I don't have a strong opinion except that it would be more consistent with existing code.

The reason I implemented v8.getHeapStatistics() the way I did is that it was lifted verbatim-ish from StrongLoop's strong-agent product, where it was called frequently enough to make the optimization worthwhile. (Although strong-agent doesn't turn it into an array, it deals with the typed memory directly.)

Maybe one or two other collaborators can chime in?

ofrobots · 2015-12-29T19:09:12Z

It is entirely possible that I am not imaginative enough. If StrongLoop's agent has a valid need to call v8.getHeapStatistics more frequently than, say, 100 Hz then this might be worth optimizing too. Otherwise, I'd vote for keeping it simple. Can you define 'frequently enough'?

bnoordhuis · 2015-12-30T10:44:15Z

It doesn't even call it that frequently, maybe a few times per second worst case, but the 'create everything in C++' approach was slow enough to make the JS -> C++ entry point show up during function tracing.

trevnorris · 2015-12-30T20:06:01Z

I can see two ways to optimize this that'll gain at least 2x. First is as @bnoordhuis mentioned, creating a single Uint32Array reference from both JS and C++. Second is a bit tricker. The call into C++ is about the same cost as a single Set() operation. So JS calls into C++ then creates a new object for the array until there are no more values. Something like:

function getHeapSpaceStatistics() {
  const ret = [];
  var name;
  while (name = nativeGetHeapSpaceStatistics()) {
    ret.push({
      space_name: name,
      // tarr[] is the Uint8Array that's shared in JS and C++
      space_size: tarr[0],
      space_used_size: tarr[1],
      space_available_size: tarr[2],
      physical_space_size: tarr[3],
    });
  }
  return ret;
}

That make sense?

Not sure all this is necessary, but the gains here will be noticeable.

bripkens · 2015-12-30T20:43:15Z

@trevnorris: That make sense?

This would mean that nativeGetHeapSpaceStatistics() is stateful and remembers the last returned heap space index, right?

@trevnorris: Not sure all this is necessary

Use cases for this API might be useful to determine the performance requirements. I'd personally like to integrate heap space data into our monitoring product. This means that we would call this API at most once per second. This being said I have a hard time imagining more frequent API calls.

Now, I don't know whether this optimization will truly be worth the effort, but I am willing to improve it as described by @bnoordhuis and @trevnorris. I can relate to @ofrobots' remarks too though I have limited experience with the performance impact of C++ vs. JS.

bripkens · 2015-12-31T05:33:10Z

PR is now updated with the approach described by @trevnorris. @bnoordhuis: Would this work for you?

trevnorris · 2015-12-31T05:59:10Z

Awesome. That's more or less what I had in mind. I'll do some benchmarking tomorrow.

If we don't want it to be stateful then updateHeapSpaceStatisticsArrayBuffer() could be made to accept the array index right?

bripkens · 2015-12-31T06:02:39Z

@trevnorris: Right, that should improve it. I'll look into that! :)

bripkens · 2015-12-31T06:14:05Z

@trevnorris: PR updated.

trevnorris · 2016-01-04T10:34:12Z

LGTM

bripkens · 2016-01-08T14:43:09Z

Anything I can do to land this in master?

Provide means to inspect information about the separate heap spaces via a callable API. This is helpful to analyze memory issues. Fixes: #2079 PR-URL: #4463 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

cjihrig · 2016-01-18T16:51:11Z

Landed in 5f57005.

Provide means to inspect information about the separate heap spaces via a callable API. This is helpful to analyze memory issues. Fixes: #2079 PR-URL: #4463 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

bripkens · 2016-01-18T18:28:39Z

Awesome, thank you guys! :)

jasnell · 2016-01-18T18:49:46Z

@bripkens .. thank you for the contribution! :-)

Notable changes: * events: make sure console functions exist (Dave) #4479 * fs: add autoClose option to fs.createWriteStream (Saquib) #3679 * http: improves expect header handling (Daniel Sellers) #4501 * node: allow preload modules with -i (Evan Lucas) #4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) #4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622 - module: cache stat() results more aggressively (Ben Noordhuis) #4575 - querystring: improve parse() performance (Brian White) #4675 PR-URL: #4742

Provide means to inspect information about the separate heap spaces via a callable API. This is helpful to analyze memory issues. Fixes: nodejs#2079 PR-URL: nodejs#4463 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * events: make sure console functions exist (Dave) nodejs#4479 * fs: add autoClose option to fs.createWriteStream (Saquib) nodejs#3679 * http: improves expect header handling (Daniel Sellers) nodejs#4501 * node: allow preload modules with -i (Evan Lucas) nodejs#4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) nodejs#4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - module: cache stat() results more aggressively (Ben Noordhuis) nodejs#4575 - querystring: improve parse() performance (Brian White) nodejs#4675 PR-URL: nodejs#4742

Provide means to inspect information about the separate heap spaces via a callable API. This is helpful to analyze memory issues. Fixes: #2079 PR-URL: #4463 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes: * child_process: add shell option to spawn() (cjihrig) #4598 * crypto: * add ALPN Support (Shigeki Ohtsu) #2564 * allow adding extra certs to well-known CAs (Sam Roberts) #9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) #4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) #5333 * process: * add `externalMemory` to `process` (Fedor Indutny) #9587 * add process.cpuUsage() (Patrick Mueller) #10796

Notable Changes: * child_process: add shell option to spawn() (cjihrig) #4598 * crypto: * add ALPN Support (Shigeki Ohtsu) #2564 * allow adding extra certs to well-known CAs (Sam Roberts) #9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) #4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) #5333 * process: * add `externalMemory` to `process` (Fedor Indutny) #9587 * add process.cpuUsage() (Patrick Mueller) #10796 PR-URL: #10973

Notable Changes: * child_process: add shell option to spawn() (cjihrig) nodejs/node#4598 * crypto: * add ALPN Support (Shigeki Ohtsu) nodejs/node#2564 * allow adding extra certs to well-known CAs (Sam Roberts) nodejs/node#9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) nodejs/node#4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) nodejs/node#5333 * process: * add `externalMemory` to `process` (Fedor Indutny) nodejs/node#9587 * add process.cpuUsage() (Patrick Mueller) nodejs/node#10796 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Dec 29, 2015

cjihrig reviewed Dec 29, 2015
View reviewed changes

bripkens force-pushed the getHeapStatistics branch from a55afa2 to 3e12225 Compare December 29, 2015 17:17

bripkens force-pushed the getHeapStatistics branch 4 times, most recently from 44d00da to 3ac5bc2 Compare December 31, 2015 05:31

bripkens force-pushed the getHeapStatistics branch from 3ac5bc2 to bba8077 Compare December 31, 2015 06:12

bripkens mentioned this pull request Jan 1, 2016

Add GC Insights #4496

Closed

bripkens force-pushed the getHeapStatistics branch 2 times, most recently from bf9e21e to c353200 Compare January 2, 2016 05:35

rvagg mentioned this pull request Jan 4, 2016

doc: explain ClientRequest#setTimeout time unit #4458

Closed

cjihrig closed this Jan 18, 2016

evanlucas mentioned this pull request Jan 18, 2016

v5.5.0 Release Proposal (Stable) #4742

Merged

bripkens deleted the getHeapStatistics branch January 18, 2016 20:25

MylesBorins added lts-agenda labels Jan 6, 2017

MylesBorins mentioned this pull request Jan 6, 2017

Potential Semver Minor Backports nodejs/Release#177

Closed

9 tasks

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 13, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v4.8.0 proposal #10973

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v8: Expose statistics about heap spaces #4463

v8: Expose statistics about heap spaces #4463

bripkens commented Dec 29, 2015

cjihrig Dec 29, 2015

bnoordhuis commented Dec 29, 2015

bripkens commented Dec 29, 2015

ofrobots commented Dec 29, 2015

bnoordhuis commented Dec 29, 2015

ofrobots commented Dec 29, 2015

bnoordhuis commented Dec 30, 2015

trevnorris commented Dec 30, 2015

bripkens commented Dec 30, 2015

bripkens commented Dec 31, 2015

trevnorris commented Dec 31, 2015

bripkens commented Dec 31, 2015

bripkens commented Dec 31, 2015

trevnorris commented Jan 4, 2016

bripkens commented Jan 8, 2016

cjihrig commented Jan 18, 2016

bripkens commented Jan 18, 2016

jasnell commented Jan 18, 2016

v8: Expose statistics about heap spaces #4463

v8: Expose statistics about heap spaces #4463

Conversation

bripkens commented Dec 29, 2015

cjihrig Dec 29, 2015

Choose a reason for hiding this comment

bnoordhuis commented Dec 29, 2015

bripkens commented Dec 29, 2015

ofrobots commented Dec 29, 2015

bnoordhuis commented Dec 29, 2015

ofrobots commented Dec 29, 2015

bnoordhuis commented Dec 30, 2015

trevnorris commented Dec 30, 2015

bripkens commented Dec 30, 2015

bripkens commented Dec 31, 2015

trevnorris commented Dec 31, 2015

bripkens commented Dec 31, 2015

bripkens commented Dec 31, 2015

trevnorris commented Jan 4, 2016

bripkens commented Jan 8, 2016

cjihrig commented Jan 18, 2016

bripkens commented Jan 18, 2016

jasnell commented Jan 18, 2016