| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Review requested:
|
Sorry, something went wrong.
|
cc @aduh95 |
Sorry, something went wrong.
Sorry, something went wrong.
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? |
Sorry, something went wrong.
My concern is that we don't run tests on 32-bit platforms on main/Node.js 24.x, so this is the first time we've tried this feature on those platforms. And while allowances might be made for the comparison in these tests to not be exactly half, the tests would seem to indicate that a 50% value is leading in some cases to a larger heap being allocated than 100% in some cases which indicates the opposite of the desired effect? |
Sorry, something went wrong.
|
Hi @richardlau, Following up on the discussion, I've run some tests on the behavior of --max-old-space-size on a 32-bit ARM (armv7l) system. The results show some significant inconsistencies that likely explain why --max-old-space-size-percentage would also be unreliable on this architecture. Test Environment
ResultsHere is a summary of the requested heap size versus the actual limit reported by V8. root@e0332c781f29:/# node --version v22.19.0 root@e0332c781f29:/# uname -m armv7l root@e0332c781f29:/# node --max-old-space-size=2000 heap_check.js V8 Heap Statistics: - Used Heap Size: 2.00 MB - Total Heap Available: 2009.87 MB - Heap Size Limit: 2012.00 MB root@e0332c781f29:/# node --max-old-space-size=5000 heap_check.js V8 Heap Statistics: - Used Heap Size: 1.99 MB - Total Heap Available: 913.87 MB - Heap Size Limit: 916.00 MB root@e0332c781f29:/# node --max-old-space-size=13000 heap_check.js V8 Heap Statistics: - Used Heap Size: 1.99 MB - Total Heap Available: 721.87 MB - Heap Size Limit: 724.00 MB root@e0332c781f29:/# node --max-old-space-size=14000 heap_check.js V8 Heap Statistics: - Used Heap Size: 1.99 MB - Total Heap Available: 1721.87 MB - Heap Size Limit: 1724.00 MB 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 13000 MB results in a smaller heap limit than requesting 5000 MB. This underlying instability confirms that the heap size is not handled predictably on this architecture. It strongly suggests that the --max-old-space-size-percentage flag would be subject to the same unreliable behavior, as it's ultimately bound by this same allocation logic. I strongly believe we should skip those tests for 32bit architecture. WDYT? |
Sorry, something went wrong.
Sorry, something went wrong.
|
The tests seem to fail due to flakiness |
Sorry, something went wrong.
Sorry, something went wrong.
|
Hi @richardlau, Thanks in advance :) |
Sorry, something went wrong.
|
I think 8733b91 should land on main first before we can proceed |
Sorry, something went wrong.
|
Hi @richardlau @aduh95, |
Sorry, something went wrong.
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>
PR-URL: nodejs#60144 Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sorry, something went wrong.
|
Hi @richardlau, |
Sorry, something went wrong.
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: #57447 PR-URL: #59082 Backport-PR-URL: #59631 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: #59460 Backport-PR-URL: #59631 Reviewed-By: theanarkh <theratliter@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
| Back | FazBrowse Home | New Git URL |
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