-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
src: add percentage support to --max-old-space-size #59082
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
src: add percentage support to --max-old-space-size #59082
Conversation
|
Review requested:
|
There was a problem hiding this 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.
Isn't it dangerous having two different flags for the same underlying v8 option? |
|
We can define that a flag will alway take precedence over another, e.g. See It could also be a good practice to abort if both flags are set. When all three heap size options 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 |
|
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 = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
I have already approached the V8 team, and they have indicated that a contribution to the Node.js project would be more appropriate. |
|
This will be a huge help, appreciate you working on this! |
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
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
a121c70 to
8e5f5ea
Compare
|
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. |
|
Is that difficult to come up with a better name? v8-gc-memlimit-percentage? |
|
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). |
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