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: resourceLimits overridden by --max-old-space-size #43991

Open
kvakil opened this issue Jul 25, 2022 · 4 comments
Open

worker: resourceLimits overridden by --max-old-space-size #43991

kvakil opened this issue Jul 25, 2022 · 4 comments
Labels
worker Issues and PRs related to Worker support.

Comments

@kvakil
Copy link
Contributor

kvakil commented Jul 25, 2022

Version

v19.0.0-pre (and earlier)

Platform

Linux sylph 5.4.0-56-generic # 62-Ubuntu SMP Mon Nov 23 19:20:19 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

worker

What steps will reproduce the bug?

If --max-old-space-size is passed on the command line, it takes
precedence over resourceLimits.maxOldSpaceSizeMb passed to the worker
thread.

It seems to come from this code in V8:

if (FLAG_max_old_space_size > 0) {

You can repro by modifying test/parallel/test-worker-resource-limits.js.
Adding a value for--max-old-space-size here (even if it is the default)
causes the test to fail.

diff --git a/test/parallel/test-worker-resource-limits.js b/test/parallel/test-worker-resource-limits.js
index f79c31b2a1..1f394eaa4a 100644
--- a/test/parallel/test-worker-resource-limits.js
+++ b/test/parallel/test-worker-resource-limits.js
@@ -1,3 +1,4 @@
+// Flags: --max-old-space-size=1000
 'use strict';
 const common = require('../common');
 const assert = require('assert');

I feel this is unintuitive and should be changed upstream, but it should
definitely be documented in NodeJS too.

How often does it reproduce? Is there a required condition?

always

What is the expected behavior?

resourceLimits.maxOldSpaceSizeMb should override --max-old-space-size.

What do you see instead?

--max-old-space-size overrides resourceLimits.maxOldSpaceSizeMb.

Additional information

No response

@kvakil
Copy link
Contributor Author

kvakil commented Jul 26, 2022

Alternative: introduce a new flag --max-old-space-size-main-thread-only (just an example name).

Use that flag to initialize the main thread's old space size. Deprecate --max-old-space-size.

This has the added advantage of removing a dependency on a V8 internal flag.

@kvakil kvakil changed the title worker: resourceLimits overriden by --max-old-space-size worker: resourceLimits overridden by --max-old-space-size Jul 26, 2022
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: nodejs#43991
@daeyeon daeyeon added the worker Issues and PRs related to Worker support. label Jul 26, 2022
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
kvakil added a commit to kvakil/node that referenced this issue Jul 26, 2022
This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: nodejs#41066
Refs: nodejs#43991
Refs: nodejs#43992
@bnoordhuis
Copy link
Member

ISTM --max-old-space-size=<n> should take precedence over resourceLimits.maxOldSpaceSizeMb when the former is smaller than the latter. It's okay for a worker to have a heap smaller than the global limit, but not bigger.

@kvakil
Copy link
Contributor Author

kvakil commented Jul 27, 2022 via email

@bnoordhuis
Copy link
Member

The behavior I would like it to have / feel is the least surprising.

nodejs-github-bot pushed a commit that referenced this issue Aug 29, 2022
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: #43991
PR-URL: #43992
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this issue Sep 5, 2022
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: #43991
PR-URL: #43992
Reviewed-By: James M Snell <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: nodejs#43991
PR-URL: nodejs#43992
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 3, 2022
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: #43991
PR-URL: #43992
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 4, 2022
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: #43991
PR-URL: #43992
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 4, 2022
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: #43991
PR-URL: #43992
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 7, 2022
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: #43991
PR-URL: #43992
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 10, 2022
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: #43991
PR-URL: #43992
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 11, 2022
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: #43991
PR-URL: #43992
Reviewed-By: James M Snell <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: nodejs/node#43991
PR-URL: nodejs/node#43992
Reviewed-By: James M Snell <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
If `--max-old-space-size` is passed on the command line, it takes
precedence over `resourceLimits.maxOldSpaceSizeMb` passed to the worker
thread. IMO this is a bug, but seems unlikely to change(?), so let's
start by documenting it. See the attached issue for more details.

Refs: nodejs/node#43991
PR-URL: nodejs/node#43992
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

3 participants