worker: allow specifying resource limits #26628

addaleax · 2019-03-13T09:42:32Z

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

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

nodejs-github-bot · 2019-03-13T09:42:36Z

@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

targos · 2019-03-13T09:58:20Z

What happens if the worker itself starts a new worker?

joyeecheung

LGTM though I have some questions

doc/api/worker_threads.md

src/node_worker.cc

joyeecheung · 2019-03-13T10:11:17Z

src/node_worker.cc

+          Null(env()->isolate()).As<Value>(),
+    };
+
+    MakeCallback(env()->onexit_string(), arraysize(args), args);


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

src/node_worker.cc

src/node_worker.h

test/parallel/test-worker-resource-limits.js

src/node_worker.cc

targos · 2019-03-13T11:38:13Z

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?

gireeshpunathil · 2019-03-13T11:57:25Z

@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

jasnell · 2019-03-13T14:43:22Z

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.

YurySolovyov · 2019-03-13T19:09:35Z

Any way to limit CPU usage?

addaleax · 2019-03-13T20:47:20Z

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

richardlau · 2019-03-13T20:50:28Z

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

addaleax · 2019-03-13T20:52:12Z

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


        process: implement resource limits.

- see nodejs/node#26628


        worker: pass resource limits to child.

preparing for: nodejs/node#26628 (comment)

BridgeAR · 2019-03-25T17:14:47Z

@addaleax seems like this requires a rebase.

addaleax · 2019-04-02T12:33:45Z

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

joyeecheung · 2019-04-02T12:42:24Z

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?

fhinkel · 2019-10-28T23:40:27Z

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

gireeshpunathil · 2019-10-29T07:48:54Z

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.

addaleax · 2019-11-01T18:31:28Z

@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?

nodejs-github-bot · 2019-11-01T18:31:52Z

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

gireeshpunathil · 2019-11-02T14:45:07Z

still LGTM

nodejs-github-bot · 2019-11-02T14:55:17Z

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

addaleax · 2019-11-02T14:57:27Z

@targos This is going to have a minor conflict with my commit in #30020 … I hope it’s okay to force-push your PR if this lands first?

targos · 2019-11-02T15:06:31Z

@addaleax sure :)

nodejs-github-bot · 2019-11-02T15:07:38Z

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

benjamingr

I am scared people won't understand what the limits... limit but the code looks good to me and this is genuinely useful.

joyeecheung · 2019-11-03T19:13:44Z

lib/internal/worker.js

@@ -294,10 +311,35 @@ function pipeWithoutWarning(source, dest) {
  dest._maxListeners = destMaxListeners;
 }

+const resourceLimitsArray = new Float64Array(kTotalResourceLimitCount);


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

joyeecheung

LGTM (the comment above was a nit)

joyeecheung · 2019-11-03T19:22:01Z

src/node_worker.cc

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


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

nodejs-github-bot · 2019-11-04T07:30:31Z

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


        worker: allow specifying resource limits

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>

addaleax · 2019-11-05T18:57:57Z

Landed in d855904 🎉


        worker: allow specifying resource limits

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>


        2019-11-20, Version 13.2.0 (Current)

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 * 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 * stream: * Add `writable.writableCorked` property (Robert Nagy) nodejs#29012 * worker: * Allow specifying resource limits (Anna Henningsen) nodejs#26628


        2019-11-20, Version 13.2.0 (Current)

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 * 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


        2019-11-19, Version 13.2.0 (Current)

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 * 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


        2019-11-19, Version 13.2.0 (Current)

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 `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


        2019-11-19, Version 13.2.0 (Current)

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 `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


        2019-11-19, Version 13.2.0 (Current)

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 `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


        2019-11-21, Version 13.2.0 (Current)

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


        2019-11-21, Version 13.2.0 (Current)

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


        2019-11-21, Version 13.2.0 (Current)

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


        2019-11-21, Version 13.2.0 (Current)

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


        2019-11-21, Version 13.2.0 (Current)

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


        2019-11-21, Version 13.2.0 (Current)

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

addaleax added semver-minor worker labels Mar 13, 2019

nodejs-github-bot added C++ errors labels Mar 13, 2019

joyeecheung approved these changes Mar 13, 2019

View changes

gireeshpunathil reviewed Mar 13, 2019

View changes

test/parallel/test-worker-resource-limits.js Outdated Show resolved Hide resolved

src/node_worker.cc Show resolved Hide resolved

gireeshpunathil approved these changes Mar 15, 2019

View changes

Trott force-pushed the nodejs:master branch from 1ecc406 to 49cf67e Sep 17, 2019

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

Open

addaleax force-pushed the addaleax:worker-resource-limits branch from 8b01037 to 6fe4678 Nov 1, 2019

addaleax added the author ready label Nov 2, 2019

benjamingr approved these changes Nov 3, 2019

View changes

joyeecheung reviewed Nov 3, 2019

View changes

joyeecheung approved these changes Nov 3, 2019

View changes

fhinkel approved these changes Nov 3, 2019

View changes

addaleax closed this Nov 5, 2019

addaleax deleted the addaleax:worker-resource-limits branch Nov 5, 2019

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 #8769

Open

nodejs/node

Perfect your code

worker: allow specifying resource limits #26628

Conversation

addaleax commented Mar 13, 2019

Checklist

This comment has been minimized.

nodejs-github-bot commented Mar 13, 2019

This comment has been minimized.

targos commented Mar 13, 2019

joyeecheung left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

targos commented Mar 13, 2019

This comment has been minimized.

gireeshpunathil commented Mar 13, 2019

This comment has been minimized.

jasnell commented Mar 13, 2019

This comment has been minimized.

YurySolovyov commented Mar 13, 2019

This comment has been minimized.

addaleax commented Mar 13, 2019

This comment has been minimized.

richardlau commented Mar 13, 2019

This comment has been minimized.

addaleax commented Mar 13, 2019

This comment has been minimized.

BridgeAR commented Mar 25, 2019

This comment has been minimized.

addaleax commented Apr 2, 2019

This comment has been minimized.

joyeecheung commented Apr 2, 2019

This comment has been minimized.

fhinkel commented Oct 28, 2019

This comment has been minimized.

gireeshpunathil commented Oct 29, 2019

This comment has been minimized.

addaleax commented Nov 1, 2019

This comment has been minimized.

nodejs-github-bot commented Nov 1, 2019

This comment has been minimized.

gireeshpunathil commented Nov 2, 2019

This comment has been minimized.

nodejs-github-bot commented Nov 2, 2019

This comment has been minimized.

addaleax commented Nov 2, 2019

This comment has been minimized.

targos commented Nov 2, 2019

This comment has been minimized.

nodejs-github-bot commented Nov 2, 2019

benjamingr left a comment

This comment has been minimized.

This comment has been minimized.

joyeecheung left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Nov 4, 2019

This comment has been minimized.

addaleax commented Nov 5, 2019