worker: add cpu profile APIs for worker

[theanarkh]

1. Support getting worker CPU profile in main thread. We need use worker inspector protocol achieve this goal currently.
2. Main thread can get the CPU usage of worker then take CPU profile when load is high.

• 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

[addaleax]

Should this be blocked on #59429?

[theanarkh]

Should this be blocked on #59429?

#59429 is blocked on this PR. This PR is ready for review.

[addaleax]

Should this be blocked on #59429?

#59429 is blocked on this PR. This PR is ready for review.

I'd say the comments on #59429 apply here too, then 🙂 And in the future, I'd recommend making those relationships clear explicitly, since right now neither of the two PRs is clearly built on top of the other (typically the easiest way to do this is to add the commits of one PR to the other)

[theanarkh]

Thank you for your suggestion !

[codecov]

Codecov Report

❌ Patch coverage is 80.11364% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.75%. Comparing base (bdcab71) to head (4797934).
⚠️ Report is 72 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_worker.cc	75.47%	11 Missing and 15 partials ⚠️
lib/internal/worker.js	86.11%	5 Missing ⚠️
src/env.cc	85.18%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59428      +/-   ##
==========================================
- Coverage   89.86%   89.75%   -0.11%     
==========================================
  Files         664      662       -2     
  Lines      194944   194685     -259     
  Branches    38285    38176     -109     
==========================================
- Hits       175180   174736     -444     
- Misses      12229    12467     +238     
+ Partials     7535     7482      -53     

Files with missing lines	Coverage Δ
src/env.h	98.14% <ø> (ø)
src/node_errors.h	87.50% <ø> (ø)
src/node_worker.h	91.66% <ø> (ø)
src/util.h	91.80% <100.00%> (+0.49%)	⬆️
src/env.cc	80.80% <85.18%> (-0.07%)	⬇️
lib/internal/worker.js	98.11% <86.11%> (-0.72%)	⬇️
src/node_worker.cc	82.83% <75.47%> (-1.50%)	⬇️

... and 46 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.

[mcollina]

lgtm

[nodejs-github-bot]

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

[Qard]

Excellent work!

I have a few suggestions, but none that I think should block landing--just forward-thinking and usability ideas.

[Qard]

Wouldn't this just discard the profile then? What use is there in starting a profile only to discard the result? 🤔

[theanarkh]

It makes sure stop the profile in some scenarios. Like this:

const { Worker } = require('worker_threads');

const w = new Worker(`setTimeout(() => {
  console.log("hello world");
}, 1000);`, { eval: true });

w.on("online", async () => {
  // call handle.stop() automatically when return
  await using handle = await w.startCpuProfile("profile");
  // unexpected return here
  if (true) { 
    return // or throw an error
  }
  await handle.stop();
});

process.on('uncaughtException', (err) => {
  console.log('uncaughtException', err);
});

[Qard]

A function that both throws and rejects seems like a bit of a footgun.

It's also not even clear why the start would return a promise--I know why, but an average user might not. I wonder if it would make sense to actually defer the async part to the stop? The start would just store a promise for the ready state and the stop would await that before applying its own logic. Makes the API a bit more intuitive to the end user and also separates the throw aspects of the start from all the reject aspects of waiting on the worker thread.

[theanarkh]

A function that both throws and rejects seems like a bit of a footgun.

I will (try to) remove the name parameter in another PR. However, I found that some Promise APIs in Node.js also throw errors synchronously such as dns promise APIs.

It's also not even clear why the start would return a promise--I know why, but an average user might not. I wonder if it would make sense to actually defer the async part to the stop? The start would just store a promise for the ready state and the stop would await that before applying its own logic. Makes the API a bit more intuitive to the end user and also separates the throw aspects of the start from all the reject aspects of waiting on the worker thread.

I think both are fine. Here, we'll keep the return Promise for now. If necessary, we can modify it later.

[Qard]

Might want to document an actual object for that with its own methods and such. That way if we decide to add more methods or method parameters in the future we would have somewhere to document those additional things. For example, I would like to add support for outputting pprof data in a buffer, or possibly the OTel profiling format (which is heavily based on pprof) in the future if/when that stabilizes at some point.

To that end, and thinking toward the future, it may also make sense for the stop() method to return a container object with a json() method on it instead, similar to fetch, so other format methods could be added in the future. This could probably also help with code-sharing with #59429 too as you could have a container that just receives the profile data object from wherever and then does the serialization logic internally.

[nodejs-github-bot]

Landed in ac92c92