worker: allow specifying resource limits by addaleax · Pull Request #26628 · nodejs/node · GitHub

Allow specifying resource limits for the JS engine instance created as part of a Worker.

/cc @nodejs/workers

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 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2889/pipeline

What happens if the worker itself starts a new worker?

LGTM though I have some questions

Maybe we can pass the limits back here as well to display them in the error messages?

Do you have suggestions for how/when to pass them back? I don’t think you’re thinking about callback arguments, right?

If we have those as AliasedBuffers we could pass them back here right? (Or is that not guaranteed to be valid at this point?)

In case you missed it, I'll ask again:

What happens if the worker itself starts a new worker?
Can it control the child's limits?
Could the child's limits be higher than the parent's?
Does the child inherit limits from its parent by default?

@targos - by looking at the current design, I guess:

Can it control the child's limits?

yes, through this interface

Could the child's limits be higher than the parent's?

yes

Does the child inherit limits from its parent by default?

no

Definitely like the direction on this. And I like the way the failure condition is handled. The one thing that's missing is: how would a worker know what it's limits are without (as you do in the tests) passing those in out of band.

Any way to limit CPU usage?

@jasnell @joyeecheung I guess we could make the resource limits available on the Worker object in the parent thread and in the child thread on the worker_threads exports object. I think that would cover both of your concerns, right?

In that case the main question for me would be, is there anything we could/should do for non-Worker threads? Should we just (inaccurately) report Infinity everywhere, or not provide a resourceLimits object at all, or somehow try to get the actual limits?

@targos Yeah, the options are independent for each Worker, in the way that @gireeshpunathil explained.

Any way to limit CPU usage?

@YurySolovyov Not in this PR, sorry. That might also be pretty complex to implement, because we don’t have the VM available to do this for us.

How does the diagnostic report work wrt. workers? Are the resource limits being added here something that could be added to the report?

@richardlau The diagnostic report is clueless about Workers (see also #26293). But yes, when they add support, I think it makes sense to include the data.

- see nodejs/node#26628

preparing for: nodejs/node#26628 (comment)

@addaleax seems like this requires a rebase.

@jasnell @joyeecheung Before I rebase this … any thoughts on #26628 (comment)?

Should we just (inaccurately) report Infinity everywhere, or not provide a resourceLimits object at all, or somehow try to get the actual limits?

I would go with not providing the object at all. It's easier to start small and known limitations are better than..known bugs?

I'm cleaning out a few old PRs 🧹. I'm closing this due to inactivity. Please re-open if needed!

I don't really see any reason why this could not have landed, don't see any blockers. On the other hand, I find great value for this PR as it provides fine grained control over the worker's execution environment - something that can be leveraged in workloads that means a lot (such as cloud)

pinging @addaleax to see if I missed something.

@joyeecheung @gireeshpunathil I’ve rebased this and made the resource constraints available as an object on both the worker_threads module and the Worker instance as suggested above… Could you take another look?

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

Is there a reason not to reuse resourceLimitsRaw here?

Although if we expose makeResourceLimits from this file and calculate the publicly exposed resourceLimits from lib/worker_threads.js instead it makes more sense to use two arrays in case someone created a worker internally with a resource limit..somehow.

Practically speaking, yes, the reason is that resourceLimitsRaw is currently not defined in the main thread. And given that this is a very small typed array, I’m okay with that. If you’re concerned about the extra resource usage, I’d probably prefer changing this to not be a static variable and instead generate one for each call to parseResourceLimits()?

LGTM (the comment above was a nit)

For my own understanding...WorkerThreadData is actually closer to NodeMainInstance for the main thread even though we don't have an equivalent abstraction of Worker for the main thread at the moment? (This was my impression when I tried with #29925 locally but I decided to take some more time to figure out a proper class hierarchy before moving forward..)

I wonder whether it makes sense to have Environment points to a base class of Worker and MainThread (?) if they have a longer life time than Environment.

For my own understanding...WorkerThreadData is actually closer to NodeMainInstance for the main thread even though we don't have an equivalent abstraction of Worker for the main thread at the moment? (This was my impression when I tried with #29925 locally but I decided to take some more time to figure out a proper class hierarchy before moving forward..)

It is somewhat similar, yes.

I wonder whether it makes sense to have Environment points to a base class of Worker and MainThread (?) if they have a longer life time than Environment.

What would we use that for? And how would that fit into the embedder API? Both Workers and the main thread have a lifetime longer than the Environment, but only very slightly so…

For example, Worker, NodeMainInstance and Environment all keep copies of arguments and exec arguments. We could reduce the duplication by only keeping them in Worker and MainThread. There seem to be a lot of duplication along the hierarchy at the moment..

I would be careful about that… deduplication is nice but right now moving towards a better embedder API is my primary goal, and linking from Environment to a superclass of Worker and NodeMainInstance sounds a bit fragile

If anything...I think a public version of that super class could be a good base for a newer set of better embedder APIs? (but maybe it's just a crazy idea, I have not given too much thought into it).

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

Allow specifying resource limits for the JS engine instance created as part of a Worker. PR-URL: #26628 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Landed in d855904 🎉

Allow specifying resource limits for the JS engine instance created as part of a Worker. PR-URL: #26628 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Notable changes: * addons: * Deprecate one- and two-argument `AtExit()`. Use the three-argument variant of `AtExit()` or `AddEnvironmentCleanupHook()` instead (Anna Henningsen) nodejs#30227 * child_process,cluster: * The `serialization` option is added that allows child process IPC to use the V8 serialization API (to e.g., pass through data types like sets or maps) (Anna Henningsen) nodejs#30162 * deps: * Update V8 to 7.9 * Update `npm` to 6.13.0 (Ruy Adorno) nodejs#30271 * embedder: * Exposes the ability to pass cli flags / options through an API as embedder (Shelley Vohr) nodejs#30466 * Allow adding linked bindings to Environment (Anna Henningsen) nodejs#30274 * esm: * Unflag --experimental-modules (Guy Bedford) nodejs#29866 * stream: * Add `writable.writableCorked` property (Robert Nagy) nodejs#29012 * worker: * Allow specifying resource limits (Anna Henningsen) nodejs#26628 * v8: * The Serialization API is now stable (Anna Henningsen) nodejs#30234 PR-URL: nodejs#30547

Notable changes: * addons: * Deprecate one- and two-argument `AtExit()`. Use the three-argument variant of `AtExit()` or `AddEnvironmentCleanupHook()` instead (Anna Henningsen) #30227 * child_process,cluster: * The `serialization` option is added that allows child process IPC to use the V8 serialization API (to e.g., pass through data types like sets or maps) (Anna Henningsen) #30162 * deps: * Update V8 to 7.9 * Update `npm` to 6.13.0 (Ruy Adorno) #30271 * embedder: * Exposes the ability to pass cli flags / options through an API as embedder (Shelley Vohr) #30466 * Allow adding linked bindings to Environment (Anna Henningsen) #30274 * esm: * Unflag --experimental-modules (Guy Bedford) #29866 * stream: * Add `writable.writableCorked` property (Robert Nagy) #29012 * worker: * Allow specifying resource limits (Anna Henningsen) #26628 * v8: * The Serialization API is now stable (Anna Henningsen) #30234 PR-URL: #30547

Allow specifying resource limits for the JS engine instance created as part of a Worker. PR-URL: #26628 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

A current problem with this approach is that you can't have a custom heap size for the parent process and the worker_threads. To customize the worker's heaps, you cannot customize the parent process's heap via CLI in any way.

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to Worker support. labels Mar 13, 2019

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Mar 13, 2019

joyeecheung approved these changes Mar 13, 2019

View reviewed changes

gireeshpunathil reviewed Mar 13, 2019

View reviewed changes

Comment thread test/parallel/test-worker-resource-limits.js Outdated

Comment thread src/node_worker.cc Outdated

chjj added a commit to chjj/bthreads that referenced this pull request Mar 14, 2019

process: implement resource limits.

2d6b7ab

- see nodejs/node#26628

chjj added a commit to chjj/bthreads that referenced this pull request Mar 14, 2019

worker: pass resource limits to child.

c5af49f

preparing for: nodejs/node#26628 (comment)

gireeshpunathil approved these changes Mar 15, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

fhinkel closed this Oct 28, 2019

gireeshpunathil reopened this Oct 29, 2019

addaleax mentioned this pull request Oct 30, 2019

doc: Worker and heap usage #30173

Closed

addaleax force-pushed the worker-resource-limits branch from 8b01037 to 6fe4678 Compare November 1, 2019 18:31

joyeecheung reviewed Nov 3, 2019

View reviewed changes

joyeecheung approved these changes Nov 3, 2019

View reviewed changes

fhinkel approved these changes Nov 3, 2019

View reviewed changes

addaleax closed this Nov 5, 2019

addaleax deleted the worker-resource-limits branch November 5, 2019 18:58

BridgeAR mentioned this pull request Nov 19, 2019

v13.2.0 proposal #30547

Merged

davidje13 mentioned this pull request Nov 30, 2019

Jest failing with "Call retries were exceeded", using --runInBand works-around issue jestjs/jest#8769

Closed

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

Semigradsky mentioned this pull request Oct 2, 2020

[node] Update worker_threads module DefinitelyTyped/DefinitelyTyped#48424

Merged

-                      &loop_,
-                      w->platform_);
-                  CHECK_NOT_NULL(isolate);
+                  Isolate::CreateParams params;

                 dest._maxListeners = destMaxListeners;
               }
+              const resourceLimitsArray = new Float64Array(kTotalResourceLimitCount);

worker: allow specifying resource limits by addaleax · Pull Request #26628 · nodejs/node · GitHub

Uh oh!

Uh oh!

worker: allow specifying resource limits - #26628

worker: allow specifying resource limits#26628addaleax wants to merge 1 commit intonodejs:masternodejs/node:masterfrom addaleax:worker-resource-limitsaddaleax/node:worker-resource-limitsCopy head branch name to clipboard

Conversation

addaleax commented Mar 13, 2019

Checklist

Uh oh!

nodejs-github-bot commented Mar 13, 2019

Uh oh!

targos commented Mar 13, 2019

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

targos commented Mar 13, 2019

Uh oh!

gireeshpunathil commented Mar 13, 2019

Uh oh!

jasnell commented Mar 13, 2019

Uh oh!

YurySolovyov commented Mar 13, 2019

Uh oh!

addaleax commented Mar 13, 2019

Uh oh!

richardlau commented Mar 13, 2019

Uh oh!

addaleax commented Mar 13, 2019

Uh oh!

BridgeAR commented Mar 25, 2019

Uh oh!

addaleax commented Apr 2, 2019

Uh oh!

joyeecheung commented Apr 2, 2019

Uh oh!

fhinkel commented Oct 28, 2019

Uh oh!

gireeshpunathil commented Oct 29, 2019

Uh oh!

addaleax commented Nov 1, 2019

Uh oh!

nodejs-github-bot commented Nov 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Nov 4, 2019

Uh oh!

addaleax commented Nov 5, 2019

Uh oh!

dwinrick-lever commented Dec 7, 2022

Uh oh!

Reviewers

worker: allow specifying resource limits#26628
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:worker-resource-limits