GeezerVison

I’m adding the semver-major label because this adds a warning where previously none is emitted.

Like I said above, I think it’s worth considering not turning this on by default either… I’ll think about that a bit more.

/cc @nodejs/workers

I'm not a fan of this being activated by default. I think the warning should only be emitted when someone explicitly enables the feature (via command line option or whatever future method).

• it may not be fair to assert that too many threads are created on program error?
• it may not be fair to assume too many threads are indicative of unbounded growth?
• it may not be fair to assume too many threads are indicative of starvation either, it might depend on the workload?
• because of the warning criteria depends on the number of cores, the same code may behave differently in different environments, and can cause concern for users?

I propose:

• print it only once: gives an indication that it is worthwhile to check the thread creation logic
• increase the threshold to a much larger value,
• and make it a constant, or a function of a constant as well

Ok, updated such that:

• The warning is off by default. Setting explicitly to 0 disables it.
• Setting to any value < 0 causes it to be auto-calculated to 4 times the number of CPUs
• Setting to any value > 0 sets the limit explicitly to that value.

@addaleax ... ok, took another round of edits to clean up the atomics usage. Quite a bit more reliable now. Also added a test for the nested workers emitting warning on the main thread. Please take another look when you have a moment :-)

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

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

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

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

Failures in CI appear to be a bug introduced by #32531 that @addaleax is investigating. Once that is fixed I will run CI again.

@jasnell #32623 should unblock this

I can see why having a circuit breaker for runaway threads is useful but:

1. Heuristics based on the number of CPUs just seems wrong. Might as well set it to 1 + rand() % 15, that's probably no worse on average.

2. How does this interact with child_process.fork()?

Heuristics based on the number of CPUs just seems wrong. Might as well set it to 1 + rand() % 15, that's probably no worse on average.

The auto-calculation there is only one of the options in this. I'm kicking off a performance study that is going to be looking at multiple kinds of workloads so we can hopefully narrow in on a better heuristic. One thing we could definitely do to give us wiggle room on the heuristic is to mark this experimental for the time being.

How does this interact with child_process.fork()?

It doesn't for the time being. Definitely open to ideas there.

I spent a lot of time thinking about auto-tuning in the context of libuv's thread pool and I came to the conclusion that it's hopeless.

A program doesn't have enough insight into the system to make educated guesses. It's hard even for the kernel and that has a perfect view of system utilization (but still misses the ability to predict the future.)

I spent a lot of time thinking about auto-tuning in the context of libuv's thread pool and I came to the conclusion that it's hopeless.

Yep, which is why this prioritizes allowing the user to set a threshold and only uses a warning that still allows the Workers to be created. It's a diagnostic option.. which, btw, we could handle in other ways if the warning is not sufficient.

Currently, there is no way of actively tracking the total number of Workers created across the process (async_hooks only provide detail on the Workers created in the current thread). Another approach we could take is to add some diagnostic tracking apis to either the worker_threads or perf_hooks modules that would allow the main thread to report on Workers across the process.

Marking this in-progress while discussion is ongoing to keep it from landing until resolved

All, I'm going to take this a slightly different direction. As I mentioned above, the intent here is largely to improve process-wide visibility of the number of workers that are being created since, currently, they are only visible to the immediate parent thread that created them. Instead of emitting a warning, here's what I'm thinking:

Via the perf_hooks API, introduce a new WorkerHook that will receive a notification for any descendant Workers created and will provide access to a process-wide counter of the total number of active Workers.

The tracker takes it's design inspiration from async hooks. However, unlike async hooks, it is not limited to reporting on only the handles associated with the threads event loop.

const { WorkerHook } = require('perf_hooks');
const hook = new WorkerHook({
  init(id, parent_id, handled) {
    console.log(`Worker ${id} created by thread ${parent_id}.`);
    // handled is described below...
  },
  destroy(id, parent_id, handled) {
    console.log(`Worker ${id} destroyed`);
  }
});
hook.enable();

// Atomically get the number of workers process-wide without creating a hook
console.log(WorkerHook.processWorkerCount);

Let's say that Main Thread creates Worker-1, which in turn creates Worker-2. Assuming both Main Thread and Worker-1 each create their own WorkerHook instances, when Worker-2 is created, the Worker-1 hook will be notified first, then the main thread hook.

The handled argument in the hooks indicates if the event was dispatched successfully to a WorkerHook in a descendant thread. So, in the above case... if we assume:

Worker-1:

const hook = WorkerHook({
  init(id, parent_id, handled) { /** handled will be false **/ }
});
hook.enable()

Main Thread:

const hook = WorkerHook({
  init(id, parent_id, handled) { /** handled will be true **/ }
});
hook.enable()

However... if disable the Worker-1 hook or omit the init() handler, the handled argument in the Main Thread will be false.

If multiple WorkerHook instances are created within a single thread, they are invoked in the order they are created, and the handled argument will reflect the status accordingly.

The API here serves two key goals:

1. Be capable of tracking the total number of active workers process-wide in a cross-platform way.

2. Be capable of having a rough idea of what part of your code is creating those workers.

Several discussion points:

1. It's not super important whether this lives in the perf_hooks, async_hooks, or worker_threads module. I picked perf_hooks because I'm also considering adding a histogram that tracks lifetime duration of workers process-wide, but that's not in-scope currently and might be handled a different way (via trace events)

2. I'm considering an option that would include the stack trace at the point of Worker creation/destruction in the handler. The option would be off by default to limit the performance hit. Essentially: const hook = new WorkerHook({ trace: true, init(id, parent_id, handled, stacktrace) { /** ... **/ }}). The other option here, however, would be to introduce a --trace-worker command-line option that, similar to -trace-sync-io, would emit a stack trace to the console upon worker creation and destruction. Definitely would like opinions on this part. There are OS-specific utilities that can provide this type of insight but none of them have visibility into the JavaScript layer where the Workers are actually created.

Closing this until I can get back to it.