src: add percentage support to --max-old-space-size

[Asaf-Federman]

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

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/startup

[legendecas]

I'm -1 on re-interpreting a V8 flag with different semantic in Node.js as it could lead to confusions. I'd be open to a new flag though.

[Asaf-Federman]

I'm -1 on re-interpreting a V8 flag with different semantic in Node.js as it could lead to confusions. I'd be open to a new flag though.

Isn't it dangerous having two different flags for the same underlying v8 option?
If user will specify both flags, which one will take precedence? Or should we throw exception in this case?

[legendecas]

We can define that a flag will alway take precedence over another, e.g. --max-old-space-size-percentage always override --max-old-space-size. I'd only consider it is harmful if a flag conditionally take precedence.

See node --v8-options:

  --max-heap-size (max size of the heap (in Mbytes) both max_semi_space_size and max_old_space_size take precedence. All three flags cannot be specified at the same time.)

It could also be a good practice to abort if both flags are set. When all three heap size options --max-heap-size=20 --max-old-space-size=20 --max-semi-space-size=20 are set at the same time, V8 aborts.

And, like mentioned, there are three heap size options. I'm -0 on introduce 3 new flags on each of them, TBH.

[Asaf-Federman]

We can define that a flag will alway take precedence over another, e.g. --max-old-space-size-percentage always override --max-old-space-size. I'd only consider it is harmful if a flag conditionally take precedence.

See node --v8-options:

  --max-heap-size (max size of the heap (in Mbytes) both max_semi_space_size and max_old_space_size take precedence. All three flags cannot be specified at the same time.)

It could also be a good practice to abort if both flags are set. When all three heap size options --max-heap-size=20 --max-old-space-size=20 --max-semi-space-size=20 are set at the same time, V8 aborts.

And, like mentioned, there are three heap size options. I'm -0 on introduce 3 new flags on each of them, TBH.

As long as there's an agreed way of action, I don't mind changing the implementation

[jasnell]

Another approach would be to see if v8 would accept a patch adding this as an additional v8 flag... I can see it potentially being generically useful for all v8 users

[jasnell]

Why double? It's likely best to just allow integer values here between 0 and 100. Not critical, of course, I'd just find it a bit cleaner

[Asaf-Federman]

Why double? It's likely best to just allow integer values here between 0 and 100. Not critical, of course, I'd just find it a bit cleaner

Should this be an integer, or would a float provide better flexibility for nodes with a large amount of memory?

[jasnell]

Really depends on how granular we want it to be. Personally I'd be fine with integer and just not worry about fractional percentages but I'd be interested in what others think

[MoLow]

if it was a breaking change I would agree, but this is a new feature, why not support more usecases even if they are rare?

[Asaf-Federman]

Another approach would be to see if v8 would accept a patch adding this as an additional v8 flag... I can see it potentially being generically useful for all v8 users

#57447

I have already approached the V8 team, and they have indicated that a contribution to the Node.js project would be more appropriate.
Please see here

[staskorz]

This will be a huge help, appreciate you working on this!

[codecov]

Codecov Report

❌ Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.06%. Comparing base (6bb08f7) to head (8e5f5ea).
⚠️ Report is 395 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_options.cc	86.36%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59082      +/-   ##
==========================================
+ Coverage   90.04%   90.06%   +0.02%     
==========================================
  Files         648      648              
  Lines      190978   191006      +28     
  Branches    37434    37438       +4     
==========================================
+ Hits       171964   172028      +64     
+ Misses      11641    11606      -35     
+ Partials     7373     7372       -1     

Files with missing lines	Coverage Δ
src/node.cc	74.93% <100.00%> (+0.20%)	⬆️
src/node_options.h	97.87% <ø> (ø)
src/node_options.cc	84.65% <86.36%> (+0.04%)	⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Asaf-Federman]

I've just force-pushed an update to this branch. I had to amend the commit history to unify the author identity across all commits.

The underlying code logic has not changed.

Apologies for any inconvenience this may cause.