GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — v8: Expose statistics about heap spaces by bripkens · Pull Request #4463 · nodejs/node

Conversation

@bripkens

@bripkens bripkens commented Dec 29, 2015

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Dec 29, 2015
cjihrig
cjihrig reviewed Dec 29, 2015
View reviewed changes
Comment thread test/parallel/test-v8-stats.js Outdated

@cjihrig cjihrig Dec 29, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Could you change these to assert.strictEqual().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bnoordhuis

bnoordhuis commented Dec 29, 2015

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bripkens commented Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

@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 :).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ofrobots

ofrobots commented Dec 29, 2015

Copy link
Copy Markdown
Contributor

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Dec 29, 2015

Copy link
Copy Markdown
Member

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ofrobots

ofrobots commented Dec 29, 2015

Copy link
Copy Markdown
Contributor

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'?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Dec 30, 2015

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@trevnorris

trevnorris commented Dec 30, 2015

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bripkens

bripkens commented Dec 30, 2015

Copy link
Copy Markdown
Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bripkens commented Dec 31, 2015

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@trevnorris

trevnorris commented Dec 31, 2015

Copy link
Copy Markdown
Contributor

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bripkens

bripkens commented Dec 31, 2015

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bripkens commented Dec 31, 2015

Copy link
Copy Markdown
Contributor Author

@trevnorris: PR updated.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bripkens bripkens mentioned this pull request Jan 1, 2016
Add GC Insights #4496
Closed
@bripkens
bripkens force-pushed the getHeapStatistics branch 2 times, most recently from bf9e21e to c353200 Compare January 2, 2016 05:35
@rvagg rvagg mentioned this pull request Jan 4, 2016
doc: explain ClientRequest#setTimeout time unit #4458
Closed
@trevnorris

trevnorris commented Jan 4, 2016

Copy link
Copy Markdown
Contributor

LGTM

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bripkens

bripkens commented Jan 8, 2016

Copy link
Copy Markdown
Contributor Author

Anything I can do to land this in master?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cjihrig

cjihrig commented Jan 18, 2016

Copy link
Copy Markdown
Contributor

Landed in 5f57005.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cjihrig cjihrig closed this Jan 18, 2016
@evanlucas evanlucas mentioned this pull request Jan 18, 2016
v5.5.0 Release Proposal (Stable) #4742
Merged
evanlucas pushed a commit that referenced this pull request Jan 18, 2016
@bripkens @evanlucas
v8,src: expose statistics about heap spaces
4254508
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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@bripkens

bripkens commented Jan 18, 2016

Copy link
Copy Markdown
Contributor Author

Awesome, thank you guys! :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Jan 18, 2016

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bripkens
bripkens deleted the getHeapStatistics branch January 18, 2016 20:25
evanlucas added a commit that referenced this pull request Jan 20, 2016
@evanlucas
2016-01-20, Version 5.5.0 (Stable)
dd88256
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
evanlucas added a commit that referenced this pull request Jan 21, 2016
@evanlucas
2016-01-20, Version 5.5.0 (Stable)
318ded2
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
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@bripkens Michael Scovetta
v8,src: expose statistics about heap spaces
7d29fc2
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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@evanlucas Michael Scovetta
2016-01-20, Version 5.5.0 (Stable)
de2ebef
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
@MylesBorins MylesBorins added lts-agenda labels Jan 6, 2017
@MylesBorins MylesBorins mentioned this pull request Jan 6, 2017
Potential Semver Minor Backports nodejs/Release#177
Closed
9 tasks done
MylesBorins pushed a commit that referenced this pull request Jan 13, 2017
@bripkens @MylesBorins
v8,src: expose statistics about heap spaces
504b01b
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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 13, 2017
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@bripkens @MylesBorins
v8,src: expose statistics about heap spaces
416b6a7
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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
v4.8.0 proposal #10973
Merged
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@bripkens @MylesBorins
v8,src: expose statistics about heap spaces
30d60cf
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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins added a commit that referenced this pull request Feb 21, 2017
@MylesBorins
2017-02-21, Version 4.8.0 'Argon' (LTS)
58ab54f
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
MylesBorins added a commit that referenced this pull request Feb 22, 2017
@MylesBorins
2017-02-21, Version 4.8.0 'Argon' (LTS)
49c5ff4
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
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
@imyller
2017-02-21, Version 4.8.0 'Argon' (LTS)
bddd5cc
    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 <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
@imyller
2017-02-21, Version 4.8.0 'Argon' (LTS)
2ee49f0
    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 <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Feb 20, 2025
v8: add v8.getCppHeapStatistics() method #57146
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

semver-minor PRs that contain new features and should be released in the next minor version. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@bripkens @bnoordhuis @ofrobots @trevnorris @jasnell @cjihrig @mscdex @MylesBorins
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.