-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
[v22.x backport] src: add percentage support to --max-old-space-size #59631
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
base: v22.x-staging
Are you sure you want to change the base?
[v22.x backport] src: add percentage support to --max-old-space-size #59631
Conversation
Review requested:
|
This commit adds support for specifying --max-old-space-size as a percentage of system memory, in addition to the existing MB format. A new HandleMaxOldSpaceSizePercentage method parses percentage values, validates that they are within the 0-100% range, and provides clear error messages for invalid input. The heap size is now calculated based on available system memory when a percentage is used. Test coverage has been added for both valid and invalid cases. Documentation and the JSON schema for CLI options have been updated with examples for both formats. Refs: nodejs#57447 PR-URL: nodejs#59082 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: theanarkh <theratliter@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Add validation to ensure that --max-old-space-size-percentage cannot be used when available memory cannot be calculated, preventing undefined behavior when memory detection fails. Also enhance test-process-constrained-memory.js to support testing in constrained environments where memory calculation may fail. PR-URL: nodejs#59460 Reviewed-By: theanarkh <theratliter@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
1520d5e
to
dac7895
Compare
cc @aduh95 |
While some of the test failures are flakes, the test added in this backport PR is failing on 32-bit Linux on ARM and 32-bit Windows.
|
Since this flag's behavior depends on V8's complex and platform-specific heap sizing logic, creating a test that perfectly models these heuristics is likely to be brittle. Therefore, I think the most pragmatic solution is to skip these assertions. What are your thoughts? |
My concern is that we don't run tests on 32-bit platforms on |
Hi @richardlau, Following up on the discussion, I've run some tests on the behavior of Test Environment
ResultsHere is a summary of the requested heap size versus the actual limit reported by V8.
Analysis & ConclusionThe data shows that V8's memory allocation on 32-bit ARM is not just capped, but behaves erratically when given large values. The most telling result is that requesting This underlying instability confirms that the heap size is not handled predictably on this architecture. It strongly suggests that the I strongly believe we should skip those tests for 32bit architecture. WDYT? |
The tests seem to fail due to flakiness |
Hi @richardlau, Thanks in advance :) |
These commits add support for specifying --max-old-space-size as a percentage of system memory, in addition to the existing MB format.
PR-URL: #59082
PR-URL: #59460