v8: add v8.getCppHeapStatistics() method #57146

Aditi-1400 · 2025-02-20T14:00:31Z

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

joyeecheung

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.

doc/api/v8.md

test/parallel/test-cppheap-stats.js

doc/api/v8.md

lib/v8.js

src/node_v8.cc

jasnell · 2025-02-20T15:27:23Z

src/node_v8.cc

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


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>()

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

(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)

src/node_v8.cc

codecov · 2025-02-20T15:30:32Z

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

lib/v8.js

addaleax

Looks great! :)

src/node_v8.cc

joyeecheung

Almost there :)

doc/api/v8.md

addaleax

LGTM with one small grammar fix

addaleax · 2025-02-27T21:27:24Z

doc/api/v8.md

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


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

nodejs-github-bot · 2025-02-27T21:35:45Z

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

nodejs-github-bot · 2025-02-27T21:37:35Z

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

nodejs-github-bot · 2025-02-27T21:57:07Z

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

nodejs-github-bot · 2025-02-27T21:57:22Z

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

src/node_v8.cc

nodejs-github-bot · 2025-02-28T16:17:49Z

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

joyeecheung · 2025-03-03T08:51:08Z

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

Expose `CppHeap` data via `cppgc::CppHeap::CollectStatistics()` in the v8 module.

nodejs-github-bot · 2025-03-03T12:49:22Z

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

nodejs-github-bot · 2025-03-03T12:51:06Z

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

nodejs-github-bot · 2025-03-03T12:52:27Z

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

nodejs-github-bot · 2025-03-03T13:00:57Z

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

nodejs-github-bot · 2025-03-04T04:12:42Z

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

nodejs-github-bot · 2025-03-04T11:14:36Z

Landed in d3064e8

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>

@geeksilva97

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

@geeksilva97

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

Notable changes: config: * (SEMVER-MINOR) add config file support (Marco Ippolito) #57016 * (SEMVER-MINOR) set default config as `node.config.json` (Marco Ippolito) #57171 crypto: * update root certificates to NSS 3.108 (Node.js GitHub Bot) #57381 doc: * add geeksilva97 to collaborators (Edy Silva) #57241 src: * (SEMVER-MINOR) create `THROW_ERR_OPTIONS_BEFORE_BOOTSTRAPPING` (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

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 force-pushed the issue-56533 branch 2 times, most recently from 0f5a16c to dac6b7a Compare February 20, 2025 14:10

joyeecheung reviewed Feb 20, 2025

View reviewed changes

doc/api/v8.md Outdated Show resolved Hide resolved

doc/api/v8.md Outdated Show resolved Hide resolved

test/parallel/test-cppheap-stats.js Show resolved Hide resolved