-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
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
Merged
nodejs-github-bot
merged 8 commits into
nodejs:main
from
Asaf-Federman:feat/max-old-space-size-percentage
Jul 28, 2025
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
c837d14
src: add percentage support to --max-old-space-size
Asaf-Federman 90f8288
doc: remove unrelated changes
Asaf-Federman 84d3433
src: introduce max-old-space-size-percentage as a new cli flag
Asaf-Federman ebaa850
doc: remove doc related to modified implementation
Asaf-Federman c377756
doc: change max-old-space-size-percentage type to be number
Asaf-Federman a23ece3
doc: revert max-old-space-size-percentage type to be a string
Asaf-Federman b919043
doc: fix linting and doc issues
Asaf-Federman 8e5f5ea
src: handle UINT64_MAX (according to libuv doc) and remove redundant …
Asaf-Federman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,134 @@ | ||
'use strict'; | ||
|
||
// This test validates the --max-old-space-size-percentage flag functionality | ||
|
||
require('../common'); | ||
const assert = require('node:assert'); | ||
const { spawnSync } = require('child_process'); | ||
const os = require('os'); | ||
|
||
// Valid cases | ||
const validPercentages = [ | ||
'1', '10', '25', '50', '75', '99', '100', '25.5', | ||
]; | ||
|
||
// Invalid cases | ||
const invalidPercentages = [ | ||
['', /--max-old-space-size-percentage= requires an argument/], | ||
['0', /--max-old-space-size-percentage must be greater than 0 and up to 100\. Got: 0/], | ||
['101', /--max-old-space-size-percentage must be greater than 0 and up to 100\. Got: 101/], | ||
['-1', /--max-old-space-size-percentage must be greater than 0 and up to 100\. Got: -1/], | ||
['abc', /--max-old-space-size-percentage must be greater than 0 and up to 100\. Got: abc/], | ||
['1%', /--max-old-space-size-percentage must be greater than 0 and up to 100\. Got: 1%/], | ||
]; | ||
|
||
// Test valid cases | ||
validPercentages.forEach((input) => { | ||
const result = spawnSync(process.execPath, [ | ||
`--max-old-space-size-percentage=${input}`, | ||
], { stdio: ['pipe', 'pipe', 'pipe'] }); | ||
assert.strictEqual(result.status, 0, `Expected exit code 0 for valid input ${input}`); | ||
assert.strictEqual(result.stderr.toString(), '', `Expected empty stderr for valid input ${input}`); | ||
}); | ||
|
||
// Test invalid cases | ||
invalidPercentages.forEach((input) => { | ||
const result = spawnSync(process.execPath, [ | ||
`--max-old-space-size-percentage=${input[0]}`, | ||
], { stdio: ['pipe', 'pipe', 'pipe'] }); | ||
assert.notStrictEqual(result.status, 0, `Expected non-zero exit for invalid input ${input[0]}`); | ||
assert(input[1].test(result.stderr.toString()), `Unexpected error message for invalid input ${input[0]}`); | ||
}); | ||
|
||
// Test NODE_OPTIONS with valid percentages | ||
validPercentages.forEach((input) => { | ||
const result = spawnSync(process.execPath, [], { | ||
stdio: ['pipe', 'pipe', 'pipe'], | ||
env: { ...process.env, NODE_OPTIONS: `--max-old-space-size-percentage=${input}` } | ||
}); | ||
assert.strictEqual(result.status, 0, `NODE_OPTIONS: Expected exit code 0 for valid input ${input}`); | ||
assert.strictEqual(result.stderr.toString(), '', `NODE_OPTIONS: Expected empty stderr for valid input ${input}`); | ||
}); | ||
|
||
// Test NODE_OPTIONS with invalid percentages | ||
invalidPercentages.forEach((input) => { | ||
const result = spawnSync(process.execPath, [], { | ||
stdio: ['pipe', 'pipe', 'pipe'], | ||
env: { ...process.env, NODE_OPTIONS: `--max-old-space-size-percentage=${input[0]}` } | ||
}); | ||
assert.notStrictEqual(result.status, 0, `NODE_OPTIONS: Expected non-zero exit for invalid input ${input[0]}`); | ||
assert(input[1].test(result.stderr.toString()), `NODE_OPTIONS: Unexpected error message for invalid input ${input[0]}`); | ||
}); | ||
|
||
// Test percentage calculation validation | ||
function getHeapSizeForPercentage(percentage) { | ||
const result = spawnSync(process.execPath, [ | ||
'--max-old-space-size=3000', // This value should be ignored, since percentage takes precedence | ||
`--max-old-space-size-percentage=${percentage}`, | ||
'--max-old-space-size=1000', // This value should be ignored, since percentage take precedence | ||
'-e', ` | ||
const v8 = require('v8'); | ||
const stats = v8.getHeapStatistics(); | ||
legendecas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const heapSizeLimitMB = Math.floor(stats.heap_size_limit / 1024 / 1024); | ||
console.log(heapSizeLimitMB); | ||
`, | ||
], { | ||
stdio: ['pipe', 'pipe', 'pipe'], | ||
env: { | ||
...process.env, | ||
NODE_OPTIONS: `--max-old-space-size=2000` // This value should be ignored, since percentage takes precedence | ||
} | ||
}); | ||
|
||
if (result.status !== 0) { | ||
throw new Error(`Failed to get heap size for ${percentage}: ${result.stderr.toString()}`); | ||
} | ||
|
||
return parseInt(result.stdout.toString(), 10); | ||
} | ||
|
||
const testPercentages = [25, 50, 75, 100]; | ||
const heapSizes = {}; | ||
|
||
// Get heap sizes for all test percentages | ||
testPercentages.forEach((percentage) => { | ||
heapSizes[percentage] = getHeapSizeForPercentage(percentage); | ||
}); | ||
|
||
// Test relative relationships between percentages | ||
// 50% should be roughly half of 100% | ||
const ratio50to100 = heapSizes[50] / heapSizes[100]; | ||
assert( | ||
ratio50to100 >= 0.4 && ratio50to100 <= 0.6, | ||
`50% heap size should be roughly half of 100% (got ${ratio50to100.toFixed(2)}, expected ~0.5)` | ||
); | ||
|
||
// 25% should be roughly quarter of 100% | ||
const ratio25to100 = heapSizes[25] / heapSizes[100]; | ||
assert( | ||
ratio25to100 >= 0.15 && ratio25to100 <= 0.35, | ||
`25% heap size should be roughly quarter of 100% (got ${ratio25to100.toFixed(2)}, expected ~0.25)` | ||
); | ||
|
||
// 75% should be roughly three-quarters of 100% | ||
const ratio75to100 = heapSizes[75] / heapSizes[100]; | ||
assert( | ||
ratio75to100 >= 0.65 && ratio75to100 <= 0.85, | ||
`75% heap size should be roughly three-quarters of 100% (got ${ratio75to100.toFixed(2)}, expected ~0.75)` | ||
); | ||
|
||
// Validate heap sizes against system memory | ||
const totalMemoryMB = Math.floor(os.totalmem() / 1024 / 1024); | ||
const margin = 10; // 5% margin | ||
testPercentages.forEach((percentage) => { | ||
const upperLimit = totalMemoryMB * ((percentage + margin) / 100); | ||
assert( | ||
heapSizes[percentage] <= upperLimit, | ||
`Heap size for ${percentage}% (${heapSizes[percentage]} MB) should not exceed upper limit (${upperLimit} MB)` | ||
); | ||
const lowerLimit = totalMemoryMB * ((percentage - margin) / 100); | ||
assert( | ||
heapSizes[percentage] >= lowerLimit, | ||
`Heap size for ${percentage}% (${heapSizes[percentage]} MB) should not be less than lower limit (${lowerLimit} MB)` | ||
); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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?