Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
src: make --use-largepages a runtime option#30954
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: make --use-largepages a runtime option #30954
Uh oh!
There was an error while loading. Please reload this page.
Conversation
addaleax left a comment
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.
Can you add a basic test that e.g. just spawns node --use-largepages -p 42 and makes sure that the output is correct (ideally verify both stderr and stdout)? That might give us a better idea under which circumstances this is or is not working
Uh oh!
There was an error while loading. Please reload this page.
bnoordhuis left a comment
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.
LGTM but can you also update doc/node.1 and can I suggest applying this patch so the build won't break on older Linux systems?
diff --git a/src/large_pages/node_large_page.cc b/src/large_pages/node_large_page.cc index 68fa178b40..d13af4c11e 100644 --- a/src/large_pages/node_large_page.cc+++ b/src/large_pages/node_large_page.cc@@ -352,7 +352,7 @@ MoveTextRegionToLargePages(const text_region& r){return -1} - ret = madvise(tmem, size, MADV_HUGEPAGE);+ ret = madvise(tmem, size, 14 /* MADV_HUGEPAGE */); if (ret == -1){PrintSystemError(errno); ret = munmap(tmem, size);(MADV_HUGEPAGE was added in Linux 2.6.38 and our baseline for running node is 3.x but I don't know what exactly we require for building node.)
Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build.
c88750e to a83c5dfComparegabrielschulhof commented Dec 14, 2019
@addaleax I added the test and I fixed the doc. |
Uh oh!
There was an error while loading. Please reload this page.
gabrielschulhof commented Dec 15, 2019
@bnoordhuis@addaleax@cjihrig@devnexen@gireeshpunathil I changed the implementation to one that accepts three values: 0 (default) to not map, 1 to map but to not report failure, and 2 to map and to report failure on |
6c9b3d5 to 6e0fd1cCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
lundibundi left a comment
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 think at least IsLargePagesEnabled and MapStaticCodeToLargePages in node_large_page.cc have to be changed as they won't compile/won't be correct on unsupported platforms due to conditional compilation in there. I think just adding conditional #else with correct error ret will be fine. They both won't have a return statement on the unsupported platforms.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Dec 17, 2019
gabrielschulhof commented Dec 17, 2019
@addaleax@joyeecheung I updated the possible values to strings: "off", "silent", "verbose". |
nodejs-github-bot commented Dec 17, 2019
Uh oh!
There was an error while loading. Please reload this page.
gabrielschulhof commented Dec 17, 2019
@devnexen thanks, updated. |
nodejs-github-bot commented Dec 17, 2019
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gabrielschulhof commented Dec 23, 2019
Dang! I made a copy/paste error while landing. Sorry I omitted you as a reviewer @jasnell! |
bcoe commented Dec 25, 2019
@gabrielschulhof we're seeing a bit of a hiccup in our nightly linux tests: 20:01:53 /bin/sh: 1: realpath: not found 20:01:53 gyp: Call to 'realpath src/large_pages/ld.implicit.script' returned exit status 127 whilein /home/iojs/build/workspace/node-test-commit-linux-coverage-daily/nodes/benchmark/node.gyp.Which seems to crop up somewhere between December 21st and December 22nd when this landed. I'm guessing there might be a workaround in changing our compiler flags? Wondering if this is expected however (would guess moving this to a flag was meant to be a noop?). |
Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. PR-URL: nodejs#30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>
Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. PR-URL: #30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>
Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. Backport-PR-URL: #31063 PR-URL: #30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>
Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. Backport-PR-URL: #31063 PR-URL: #30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>
Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. PR-URL: nodejs#30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>
Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. PR-URL: nodejs#30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>
Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. Backport-PR-URL: nodejs#32092 PR-URL: nodejs#30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>
Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. Backport-PR-URL: #32092 PR-URL: #30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>
Moves the option that instructs Node.js to-remap its static code to
large pages from a configure-time option to a runtime option. This
should make it easy to assess the performance impact of such a change
without having to custom-build.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes