workers: implement --max-worker-threads command line option

[jasnell]

There are two commits here:

1. Adds a simple CPUInfo utility to make using uv_cpu_info just a tad easier
2. Implements the --max-worker-threads command line flag..

From the second commit message:

 Creating too many active worker threads at one time can
lead to significant performance degradation of the entire
Node.js process. This adds a worker thread counter that
will cause a warning to be emitted if exceeded. Workers
can still be created beyond the limit, however. The warning
is similar in spirit to the too many event handlers warning
emitted by EventEmitter.
By default, the limit is one less than four times the total
number of CPUs available calculated at system start. The
`--max-worker-threads` command-line option can be set to
set a non-default value. The option is permitted in
`NODE_OPTIONS` and must be positive number greater than
zero.
The counter and the option are per-process in order to
account for Workers that create their own Workers.
The warning will be emitted once each time the limit
is exceeded, so may be emitted more than once per process.
That is, if the limit is 2, and 5 workers are created, only
a single warning will be emitted. If the number of active
workers falls back below 2 and is subsequently exceeded
again, the warning will be emitted again.

/cc @addaleax

Checklist

• 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]

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

[mscdex]

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

[gireeshpunathil]

• 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

[jasnell]

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.

[jasnell]

@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 :-)

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[jasnell]

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

[addaleax]

@jasnell#32623 should unblock this

[bnoordhuis]

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

[jasnell]

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.

[bnoordhuis]

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

[jasnell]

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.

[jasnell]

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

[lundibundi]

Definitely +1 on the idea but I agree that the default should be 0 as any guesses here are nothing more than guesses IMO.

Also, perhaps we can name this something "weaker" than "max-worker-threads" as for me the name implies that we won't be able to create more than the specified amount of workers and not that we will just get a warning about it?

[lundibundi]

Suggested change

	// too much CPU contention. The default max-worker-threads is
	//4 times the total number of CPUs available but may be set
	// too much CPU contention. By default max-worker-threads
	//check is disabled but may be set

[lundibundi]

Nit: I think we usually try to put empty lines in between methods. Could you also run make format-cpp for consistency?

[jasnell]

Sigh...

C:\Users\jasne\Projects\node>vcbuild format-cpp
Error: invalid command line option `format-cpp`.

Yeah, when I update this I'll switch over to the linux box and tweak the formatting.

[lundibundi]

Nit: (here and below)

Suggested change

	// number of CPUs
	// number of CPUs.

[lundibundi]

Nit: simplification and a bit fewer promises (and same applies to the other test if you decide to change)

functionmakeWorker(){returnnewPromise((res)=>{constworker=newWorker(expr,{eval: true});worker.once('online',()=>res(worker));});}asyncfunctiondoTest(){constpromises=[];for(letn=0;n<cpu_count;n++)promises.push(makeWorker());returnPromise.all(promises).then((workers)=>workers.forEach((i)=>i.terminate())}

[addaleax]

If we do that, then I’d go one step further and use

asyncfunctionmakeWorker(){constworker=newWorker(expr,{eval: true});awaitonce(worker,'online');returnworker;}

🙂

[jasnell]

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');consthook=newWorkerHook({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 hookconsole.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:

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

Main Thread:

consthook=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.

[jasnell]

Closing this until I can get back to it.