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.

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

Worker threads #9

Closed
carera opened this issue Aug 30, 2019 · 20 comments · Fixed by #39
Closed

Worker threads #9

carera opened this issue Aug 30, 2019 · 20 comments · Fixed by #39
Labels

Comments

@carera
Copy link

carera commented Aug 30, 2019

Hi,
thanks for the lib, next allows us to profile node12 apps!
One question - I noticed job processed on worker_threads is not shown in the profiles. Do you know a way how to work around this?

@hyj1991
Copy link
Owner

hyj1991 commented Aug 31, 2019

Can you please include a repro if you can?

@hyj1991 hyj1991 added the node12 label Aug 31, 2019
@carera
Copy link
Author

carera commented Aug 31, 2019

Sure:

const { Worker, isMainThread, parentPort } = require("worker_threads");
const fs = require("fs");

function work() {
  const fib = n => (n <= 1 ? 1 : fib(n - 1) + fib(n - 2));
  let x;
  // Just do some expensive calculations
  for (let i = 0; i < 1e4; i++) {
    x = fib(1 + (i % 24));
  }
  if (parentPort) parentPort.postMessage(x);
}
const proftitle = "fibonacci";

if (isMainThread) {
  const v8Profiler = require("v8-profiler-next");
  v8Profiler.setSamplingInterval(100);
  v8Profiler.startProfiling(proftitle, true);
  new Worker(__filename).on("message", result => {
    const profiler = v8Profiler.stopProfiling(proftitle);
    profiler.export(function(error, result) {
      fs.writeFileSync("profile.cpuprofile", result);
      profiler.delete();
    });
    console.log(result);
  });
} else {
  work(); // worker thread
}

This is the profile:
Screen Shot 2019-08-31 at 9 56 24 am
(The whole thing takes ~500ms. It looks like only the orchestration on the main thread is captured and then it's idling, until the worker posts message back)

In comparison this is how profile looks when profiling the fibonnaci calculation itself, without using worker_threads:
Screen Shot 2019-08-31 at 9 47 38 am

Flamecharts generated from the cpuprofiles (using http://github.com/brendangregg/FlameGraph):
image

VS standalone fibonacci function, without worker threads:
image

@carera
Copy link
Author

carera commented Aug 31, 2019

I thought, well, if that doesn't work, I will just run the profiler inside the worker, right? But when I try to run the profile inside the worker, it exits with error Module did not self-register..

@carera
Copy link
Author

carera commented Aug 31, 2019

Not sure if it is related in any way, but there was once this nodejs/node#24016

@hyj1991
Copy link
Owner

hyj1991 commented Sep 1, 2019

I think it maybe v8 profiler can't work in work threads because isolate is designed not to be thread-safe. But when the work thread feature gets into Stability 2 (now is Stability 1: Experimental), the kernel may have a solution with it :)

@slonka
Copy link

slonka commented Sep 1, 2019

@carera I have a repo that shows what is compatible with worker threads and what is not in terms of profiling. Bear in mind that it is still work in progress: https://github.com/slonka/pprof-nodejs-and-worker-threads/

@carera
Copy link
Author

carera commented Sep 11, 2019

Thanks @hyj1991 and @slonka . Do you by any chance have insight on whether the NodeJS team moves the worker threads into stability 2 and with what version?

@slonka
Copy link

slonka commented Sep 11, 2019

@carera
Copy link
Author

carera commented Oct 4, 2019

Hi all, so the workers feature is now flagged stable. What are the consequences?

@hyj1991
Copy link
Owner

hyj1991 commented Feb 21, 2020

nodejs/node#31569

@carera since we can take heap snapshot from parent thread in this OR, I think the implementation for cpu profiling will come soon.

@Widdershin
Copy link
Collaborator

@hyj1991 I'm interested in profiling worker threads, and was wondering if there any updates to the state of affairs.

I had a look through the Node repo and couldn't find any issues/prs to add an equivalent CPU profiling API akin to the heap snapshot one, as it seems they're trying to cover that functionality off with the inspector module.

Do you think it would be possible for us to build worker thread profiling on top of the inspector profiling machinery it already has, or do we need something different added to Node?

I'm willing to help out implementing this but I'd need a bit of guidance.

@slonka
Copy link

slonka commented Jun 30, 2020

Disclaimer: I did not work on profiling in node.js since sep 2019.

@Widdershin here is a working example of profiling inside worker threads using inspector module: https://github.com/slonka/nodejs-production-ready-profiling/blob/master/node-inspector.js

@Widdershin
Copy link
Collaborator

That's a great reference, thanks @slonka. Just curious, did you run into any issues with the inspector module at any point? My biggest reluctance is that stability banner.

@slonka
Copy link

slonka commented Jun 30, 2020

Just curious, did you run into any issues with the inspector module at any point? My biggest reluctance is that stability banner.

I vaguely remember talking to some Node.js devs and them saying that it's the preferred method but I can see that the stability did not change: https://nodejs.org/api/inspector.html#inspector_inspector . I would open up an issue and ask them what's the recommended way to profile inside worker threads with an ability to start/stop on demand.

@Widdershin
Copy link
Collaborator

For what it's worth, v8-profiler-node8 seems to reliably profile inside of worker threads as well, with a slightly nicer API and feature set. I'm not sure that there are actually any issues with the node8 version in that regard, so I'll report back after more testing.

@Widdershin
Copy link
Collaborator

Ah, I see the problem now. v8-profiler-node8 works for profiling worker threads, but only if it hasn't first been loaded in the main thread.

Seems like we need to be context aware! schroffl/node-lzo#11

I'm going to have a go at making v8-profiler-node8 context aware, hopefully the same can be done for this library.

@Widdershin
Copy link
Collaborator

I managed to make v8-profiler-node8 work across threads, should be totally viable to do the same here.

hyj1991/v8-profiler-node8#15

@hyj1991
Copy link
Owner

hyj1991 commented Jul 9, 2020

npm i v8-profiler-node8@next --save now supports CPU profile in the worker threads.

Refs:

I will patch it to v8-profiler-next soon.

@hyj1991 hyj1991 closed this as completed Jul 9, 2020
@oscar-gardiazabal
Copy link

this was done?

@hyj1991
Copy link
Owner

hyj1991 commented May 19, 2022

Landed in 8eea608, you can use v8-profiler-next@v1.6.1 to do CPU profiling in worker_threads.

Reference: #37
PR-URL: #39

@hyj1991 hyj1991 mentioned this issue May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants