Skip to content

Conversation

@Asaf-Federman
Copy link
Contributor

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
Copy link
Collaborator

Review requested:

  • @nodejs/config
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 16, 2025
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Member

legendecas commented Jul 16, 2025

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
Copy link
Contributor Author

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

@Asaf-Federman Asaf-Federman requested a review from legendecas July 17, 2025 12:25
@jasnell
Copy link
Member

jasnell commented Jul 18, 2025

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


// Parse the percentage value
char* end_ptr;
double percentage =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link

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

@codecov
Copy link

codecov bot commented Jul 21, 2025

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.

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
@Asaf-Federman Asaf-Federman force-pushed the feat/max-old-space-size-percentage branch from a121c70 to 8e5f5ea Compare July 23, 2025 18:19
@Asaf-Federman
Copy link
Contributor Author

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.

@Asaf-Federman Asaf-Federman requested a review from legendecas July 24, 2025 07:51
@legendecas legendecas added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 24, 2025
@evrial
Copy link

evrial commented Dec 3, 2025

Is that difficult to come up with a better name? v8-gc-memlimit-percentage?

@joyeecheung
Copy link
Member

Please be respectful in this issue tracker. Whether a name is better or not is subjective, and there's no supporting evidence that the suggested name is universally accepted as being better.

(Also, yes, naming things is one of the hardest problems in computer science).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v22.x Indicate that the PR has an open backport c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.