doc: add new useful V8 option #42575
Conversation
Add the '--max_semi_space_size' flag into useful V8 option. Fixes: nodejs#42511
nodejs-github-bot
added
cli
Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
doc/api/cli.md
Outdated
| The actual throughput improvement and memory consumption | ||
| are relevant to your application | ||
| ([reference](https://github.com/nodejs/node/issues/42511)). |
There was a problem hiding this comment.
I'm not sure I understand this sentence, do you mean it's relevant to any Node.js application, and if so, how do you know?
There was a problem hiding this comment.
I tuned the max_semi_space_size value and tested it on two workloads, this is the results:
From the figure we can see that the trend of throughput improve are similar on both workloads, but the absolute value are different. I just want the user to test and choose the optimal configuration for their own application.
Co-authored-by: Antoine du Hamel <[email protected]>
doc/api/cli.md
Outdated
|
|
||
| _The default value is | ||
| 16MiB for 64-bit systems and 8MiB for 32-bit systems. | ||
| The recommended value is 64MiB or 128MiB if your system has enough |
There was a problem hiding this comment.
I'm wondering if there a rational we can include for choosing these as the recommendation?
There was a problem hiding this comment.
Actually, there isn't too much optional value for the 'max_semi_space_size', because V8 did the round up operation to make sure the 'max_semi_space_size' is the power of 2. So, only if we set the value of 'max_semi_space_size' to 16MiB, 32MiB, 64MiB, 128MiB, 256MiB and so on, the behavior is what we expected. I tuned these value on WebTooling and Ghost.js workloads, the result shows that 128MiB is the optimal configuration for both of these workloads.
I don't know if it is appropriate to put this image into the document. So I just put the link of issue #42511 in the document.
There was a problem hiding this comment.
I'd refrain from recommending specific values, just that higher values can give better throughput but the reader should benchmark that.
It should also mention that the size is effectively tripled because there is more than one semi space. Observe:
$ for MB in 16 32 64 128; do
node --max_semi_space_size=$MB --max_old_space_size=16 -p \
'const MB = 1<<20; (v8.getHeapStatistics().heap_size_limit - 16*MB) / MB';
done
48
96
192
384
There was a problem hiding this comment.
@bnoordhuis Thanks for your suggestion! I have updated the patch to guide users to choose the max_semi_space_size for themselves.
Co-authored-by: Michael Dawson <[email protected]>
Add benchmark example to choose the best configuration instead.
There was a problem hiding this comment.
Hi @JialuZhang-intel, my humble 2 cents.
In addition, perhaps should use reference-style links instead of inline links.
More detailed explanation about memory consumption and semi-space size.
Co-authored-by: Lam Wei Li <[email protected]> Co-authored-by: Antoine du Hamel <[email protected]>
use reference-style links instead of inline links.
@peteriman Thanks for your advice! I changed the links styles in this patch. |
|
It's been a long time and a lot of updates since this PR created. Thanks for the suggestions from all the reviewers @mhdawson @aduh95 @bnoordhuis @VoltrexMaster @peteriman @joyeecheung. Is there anything else need to fix about this PR? |
Co-authored-by: James M Snell <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Fix make lint errors in cli.md.
…luZhang-intel/node into document_max_semi_space_size
Add link for issue nodejs#42511.
aduh95
added
the
author ready
aduh95
added
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/42575 ✔ Done loading data for nodejs/node/pull/42575 ----------------------------------- PR info ------------------------------------ Title doc: add new useful V8 option (#42575) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch JialuZhang-intel:document_max_semi_space_size -> nodejs:main Labels doc, cli, author ready, commit-queue-squash Commits 21 - doc: add new useful V8 option - Update doc/api/cli.md - Update doc/api/cli.md - Update doc/api/cli.md - Update doc/api/cli.md - Apply suggestions from code review - Update doc/api/cli.md - Remove recommended value for max_semi_space_size - Merge branch 'nodejs:master' into document_max_semi_space_size - doc: update format for cli.md - doc: add new useful V8 option - Apply suggestions from code review - doc: add new useful V8 option - doc: add new useful V8 option - Apply suggestions from code review - Update doc/api/cli.md - Update doc/api/cli.md - Merge branch 'nodejs:master' into document_max_semi_space_size - doc: add new useful V8 option - Merge branch 'document_max_semi_space_size' of https://github.com/Jia… - doc: add new useful V8 option Committers 2 - JialuZhang-intel - GitHub PR-URL: https://github.com/nodejs/node/pull/42575 Fixes: https://github.com/nodejs/node/issues/42511 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Michael Dawson ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/42575 Fixes: https://github.com/nodejs/node/issues/42511 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Michael Dawson -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 02 Apr 2022 10:49:25 GMT ✔ Approvals: 3 ✔ - Ben Noordhuis (@bnoordhuis): https://github.com/nodejs/node/pull/42575#pullrequestreview-1000860391 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/42575#pullrequestreview-1003513324 ✔ - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/42575#pullrequestreview-1004903706 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD 7f195189f0..7fd4cf4673 main -> origin/main ✔ origin/main is now up-to-date main is out of sync with origin/main. Mismatched commits: - 7fd4cf4673 loader: make `require.resolve` throw for unknown builtin modules -------------------------------------------------------------------------------- HEAD is now at 7fd4cf4673 loader: make `require.resolve` throw for unknown builtin modules ✔ Reset to origin/main - Downloading patch for 42575 From https://github.com/nodejs/node * branch refs/pull/42575/merge -> FETCH_HEAD ✔ Fetched commits as b6c6cc70009d..5411215a072a -------------------------------------------------------------------------------- Auto-merging doc/api/cli.md [main 1fea61e77a] doc: add new useful V8 option Author: JialuZhang-intel Date: Sat Apr 2 18:35:30 2022 +0800 1 file changed, 16 insertions(+) Auto-merging doc/api/cli.md [main 1a6964161e] Update doc/api/cli.md Author: JialuZhang-intel Date: Sat Apr 2 20:08:45 2022 +0800 1 file changed, 1 insertion(+), 1 deletion(-) Auto-merging doc/api/cli.md [main 73a0b57a4b] Update doc/api/cli.md Author: JialuZhang-intel Date: Sat Apr 2 20:09:00 2022 +0800 1 file changed, 1 insertion(+), 1 deletion(-) Auto-merging doc/api/cli.md [main adc451fecd] Update doc/api/cli.md Author: JialuZhang-intel Date: Sat Apr 2 20:09:15 2022 +0800 1 file changed, 2 insertions(+), 1 deletion(-) Auto-merging doc/api/cli.md [main c6690802c2] Update doc/api/cli.md Author: JialuZhang-intel Date: Sat Apr 2 20:17:06 2022 +0800 1 file changed, 3 insertions(+), 2 deletions(-) Auto-merging doc/api/cli.md [main 2cd6076c8c] Apply suggestions from code review Author: JialuZhang-intel Date: Sun Apr 3 13:06:22 2022 +0800 1 file changed, 8 insertions(+), 11 deletions(-) Auto-merging doc/api/cli.md [main fba60b1b8e] Update doc/api/cli.md Author: JialuZhang-intel Date: Fri Apr 8 10:15:09 2022 +0800 1 file changed, 1 insertion(+), 1 deletion(-) Auto-merging doc/api/cli.md error: commit d1defe3c16419451925e4e4f672a279cbcf1185d is a merge but no -m option was given. fatal: cherry-pick failed [main 7de3968c3a] Remove recommended value for max_semi_space_size Author: JialuZhang-intel Date: Thu May 5 10:58:55 2022 +0800 1 file changed, 15 insertions(+), 6 deletions(-) ✖ Failed to apply patcheshttps://github.com/nodejs/node/actions/runs/2514254971 |
|
Landed in f7770ed |
Add the `--max_semi_space_size` flag into useful V8 option. Fixes: #42511 PR-URL: #42575 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Add the `--max_semi_space_size` flag into useful V8 option. Fixes: #42511 PR-URL: #42575 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Add the `--max_semi_space_size` flag into useful V8 option. Fixes: nodejs/node#42511 PR-URL: nodejs/node#42575 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
|
@JialuZhang-intel the default See #55487 |
Add the '--max_semi_space_size' flag into useful V8 option.
Fixes: #42511