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.

Sign up for GitHub

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

Jump to bottom

v8: add v8.getCppHeapStatistics() method #57146

Merged
merged 1 commit into from
Mar 4, 2025
Merged

v8: add v8.getCppHeapStatistics() method #57146

merged 1 commit into from
Mar 4, 2025
+440 −1

Conversation

Aditi-1400
Copy link
Contributor

Expose CppHeap data via cppgc::CppHeap::CollectStatistics() in the v8 module.
Fixes: #56533

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Feb 20, 2025
@Aditi-1400 Aditi-1400 force-pushed the issue-56533 branch 2 times, most recently from 0f5a16c to dac6b7a Compare February 20, 2025 14:10
joyeecheung
joyeecheung reviewed Feb 20, 2025
View reviewed changes
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.

I think v8: add v8.getCppHeapStatistics() may be more intuitive as a commit message. Otherwise it might get interpreted as something like "exposing the cppgc::CppHeap::CollectStatistics() symbol" in the binary (for addons) / exposing the headers instead.

jasnell
jasnell reviewed Feb 20, 2025
View reviewed changes
jasnell
jasnell reviewed Feb 20, 2025
View reviewed changes
jasnell
jasnell reviewed Feb 20, 2025
View reviewed changes
jasnell
jasnell reviewed Feb 20, 2025
View reviewed changes
jasnell
jasnell reviewed Feb 20, 2025
View reviewed changes
jasnell
jasnell reviewed Feb 20, 2025
View reviewed changes
jasnell
jasnell reviewed Feb 20, 2025
View reviewed changes
src/node_v8.cc Outdated

cppgc::HeapStatistics stats = isolate->GetCppHeap()->CollectStatistics(
static_cast<cppgc::HeapStatistics::DetailLevel>(
args[0].As<v8::Int32>()->Value()));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Add using v8::Int32; to the list of other using v8:... items at the top of the namespace and change this to just .As<Int32>()

Copy link
Member

Choose a reason for hiding this comment

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

Side note... we do this kind of static cast so often we should probably just wrap it in a utility macro.... but no need to do that here

Copy link
Member

@joyeecheung joyeecheung Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(About that side note, maybe it's even better to wrap them in methods with templated return types for a bit of type checking, the integer types and enums can get especially hairy because there are so many of variants of them, I think FromV8Value<T>() would be very cool)

jasnell
jasnell reviewed Feb 20, 2025
View reviewed changes
@codecov Codecov
Copy link

codecov bot commented Feb 20, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 66.48352% with 61 lines in your changes missing coverage. Please review.

Project coverage is 90.20%. Comparing base (6b0af17) to head (70d4908).
Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
src/node_v8.cc 62.11% 50 Missing and 11 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57146      +/-   ##
==========================================
- Coverage   90.24%   90.20%   -0.05%     
==========================================
  Files         630      630              
  Lines      184908   185101     +193     
  Branches    36181    36196      +15     
==========================================
+ Hits       166874   166972      +98     
- Misses      11061    11115      +54     
- Partials     6973     7014      +41
Files with missing lines Coverage Δ
lib/v8.js 99.37% <100.00%> (+0.02%) ⬆️
src/node_v8.cc 80.72% <62.11%> (-9.64%) ⬇️

... and 21 files with indirect coverage changes

jasnell
jasnell reviewed Feb 21, 2025
View reviewed changes
addaleax
addaleax reviewed Feb 21, 2025
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks great! :)

joyeecheung
joyeecheung reviewed Feb 21, 2025
View reviewed changes
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.

Almost there :)

addaleax
addaleax approved these changes Feb 27, 2025
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM with one small grammar fix

doc/api/v8.md Outdated
Comment on lines 276 to 277
Retrieves [CppHeap][] regarding memory consumption and
utilization statistics using the V8 [`CollectStatistics()`][] function which
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Retrieves [CppHeap][] regarding memory consumption and
utilization statistics using the V8 [`CollectStatistics()`][] function which
Retrieves [CppHeap][] statistics regarding memory consumption and
utilization using the V8 [`CollectStatistics()`][] function which

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65471/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6429/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6429/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6429/

Qard
Qard approved these changes Feb 28, 2025
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65486/

@joyeecheung
Copy link
Member

joyeecheung commented Mar 3, 2025 •
edited
Loading

@Aditi-1400 looks like there is a merge conflict. Can you rebase the PR?

@Aditi-1400
v8: add v8.getCppHeapStatistics() method
70d4908 
Expose `CppHeap` data via
`cppgc::CppHeap::CollectStatistics()` in the v8 module.
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65538/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6436/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6436/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6436/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65564/

@joyeecheung joyeecheung changed the title Expose cppgc::CppHeap::CollectStatistics() v8: add v8.getCppHeapStatistics() method Mar 4, 2025
@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 4, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 4, 2025
@nodejs-github-bot nodejs-github-bot merged commit d3064e8 into nodejs:main Mar 4, 2025
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d3064e8

@joyeecheung joyeecheung added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 4, 2025
aduh95 pushed a commit that referenced this pull request Mar 9, 2025
@Aditi-1400 @aduh95
v8: add v8.getCppHeapStatistics() method
8fe6495 
Expose `CppHeap` data via
`cppgc::CppHeap::CollectStatistics()` in the v8 module.

PR-URL: #57146
Fixes: #56533
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Mar 9, 2025
@Aditi-1400 @aduh95
v8: add v8.getCppHeapStatistics() method
a22c21c 
Expose `CppHeap` data via
`cppgc::CppHeap::CollectStatistics()` in the v8 module.

PR-URL: #57146
Fixes: #56533
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@github-actions github-actions bot mentioned this pull request Mar 12, 2025
Draft
nodejs-github-bot added a commit that referenced this pull request Mar 12, 2025
@nodejs-github-bot
2025-03-13, Version 23.10.0 (Current)
a980eab 
Notable changes:

crypto:
  * update root certificates to NSS 3.108 (Node.js GitHub Bot) #57381
doc:
  * add @geeksilva97 to collaborators (Edy Silva) #57241
src:
  * (SEMVER-MINOR) set default config as node.config.json (Marco Ippolito) #57171
  * (SEMVER-MINOR) create THROW_ERR_OPTIONS_BEFORE_BOOTSTRAPPING (Marco Ippolito) #57016
  * (SEMVER-MINOR) add config file support (Marco Ippolito) #57016
test_runner:
  * (SEMVER-MINOR) change ts default glob (Marco Ippolito) #57359
tls:
  * (SEMVER-MINOR) implement tls.getCACertificates() (Joyee Cheung) #57107
v8:
  * (SEMVER-MINOR) add v8.getCppHeapStatistics() method (Aditi) #57146

PR-URL: #57424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@Qard Qard Qard approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. 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.

Expose cppgc::CppHeap::CollectStatistics() in v8 module
6 participants
@Aditi-1400 @nodejs-github-bot @joyeecheung @Qard @jasnell @addaleax