Skip to content

Conversation

theanarkh
Copy link
Contributor

Add CPU profile APIs.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 10, 2025
@theanarkh theanarkh force-pushed the add_cpu_profile_api branch from f7114fb to d49b31f Compare August 10, 2025 08:38
src/node_v8.cc Outdated
V8::SetFlagsFromString(*flags, static_cast<size_t>(flags.length()));
}

class JSONOutputStream : public v8::OutputStream {
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
class JSONOutputStream : public v8::OutputStream {
class JSONOutputStream final : public v8::OutputStream {

Copy link
Member

Choose a reason for hiding this comment

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

Btw – we may want to use HeapSnapshotStream here? I know it requires a bit more setup, but it integrates nicely with existing stream primitives in Node.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these are two issues in HeapSnapshotStream:

  1. We should not delete the snapshot(which is created in worker) in main thread.
  2. It will crash when the worker exits.
const worker = require('worker_threads');

const w = new worker.Worker("setInterval(() => {}, 100)", { eval: true})
w.on('online', async () => {
  const stream = await w.getHeapSnapshot()
  stream.on('data', () => {});
  // Crash
  w.terminate();
});

@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. v8 module Issues and PRs related to the "v8" subsystem. labels Aug 11, 2025
Copy link
Contributor

@szegedi szegedi left a comment

Choose a reason for hiding this comment

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

Advocating for using CpuProfiler::Start and thus eliminating the need for users to invent names for the profiler sessions in order to use the API.

@theanarkh theanarkh force-pushed the add_cpu_profile_api branch 6 times, most recently from f4618c5 to e78d71a Compare September 2, 2025 16:59
@theanarkh theanarkh marked this pull request as ready for review September 2, 2025 17:01
@theanarkh theanarkh force-pushed the add_cpu_profile_api branch 2 times, most recently from 5037a4a to b3c64d0 Compare September 2, 2025 17:04
Copy link

codecov bot commented Sep 2, 2025

Codecov Report

❌ Patch coverage is 86.15385% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.95%. Comparing base (961554c) to head (1b66417).
⚠️ Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
src/node_v8.cc 77.41% 2 Missing and 5 partials ⚠️
lib/v8.js 94.11% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59429      +/-   ##
==========================================
+ Coverage   89.93%   89.95%   +0.01%     
==========================================
  Files         667      667              
  Lines      196775   196841      +66     
  Branches    38409    38422      +13     
==========================================
+ Hits       176977   177069      +92     
+ Misses      12247    12200      -47     
- Partials     7551     7572      +21     
Files with missing lines Coverage Δ
src/node_v8.h 40.00% <ø> (ø)
lib/v8.js 99.02% <94.11%> (-0.35%) ⬇️
src/node_v8.cc 79.84% <77.41%> (-0.59%) ⬇️

... and 51 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@theanarkh theanarkh mentioned this pull request Sep 8, 2025
4 tasks
@theanarkh theanarkh closed this Sep 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. 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 module Issues and PRs related to the "v8" subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants