Skip to content
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

Closed

Conversation

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Dec 13, 2019

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), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Copy link
Member

addaleax left a comment

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

doc/api/cli.md Outdated Show resolved Hide resolved
Copy link
Member

bnoordhuis left a comment

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.
@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:runtime-large-pages-2 branch from c88750e to a83c5df Dec 14, 2019
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Dec 14, 2019

@addaleax I added the test and I fixed the doc.
@bnoordhuis I replaced the constant with its value and added the man page entry.

src/node.cc Outdated Show resolved Hide resolved
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

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 stderr.

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:runtime-large-pages-2 branch from 6c9b3d5 to 6e0fd1c Dec 15, 2019
doc/node.1 Outdated Show resolved Hide resolved
Copy link
Member

lundibundi left a comment

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.

src/node.cc Outdated Show resolved Hide resolved
test/parallel/test-startup-large-pages.js Outdated Show resolved Hide resolved
src/node.cc Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Dec 17, 2019

@addaleax @joyeecheung I updated the possible values to strings: "off", "silent", "verbose".
@lundibundi I applied your nits, and I added reporting for the case where mapping is not supported. I also placed the whole chunk into an ifdef to prevent compiling the code on unsupported platforms. On those platforms and in the verbose case, a message will be written to stderr that they're not supported.

@nodejs-github-bot

This comment has been minimized.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Dec 17, 2019

@devnexen thanks, updated.

@nodejs-github-bot

This comment has been minimized.

doc/api/cli.md Outdated Show resolved Hide resolved
src/node_options.cc Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Dec 19, 2019

FreeBSD crash is legit.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Dec 19, 2019

@devnexen any ideas?

@devnexen

This comment has been minimized.

Copy link
Contributor

devnexen commented Dec 19, 2019

will look into it within the week end most likely.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Dec 19, 2019

@devnexen Thanks!

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Dec 19, 2019

@devnexen AFAICT the original implementation also segfaults: https://ci.nodejs.org/job/node-test-commit-freebsd/30323/nodes=freebsd11-x64/consoleFull

Might be because we're using 11.2, which, I'm told on the #freebsd IRC channel, is EOL.

Copy link
Member

lundibundi left a comment

LGTM once FreeBSD issue is fixed.

@devnexen

This comment has been minimized.

Copy link
Contributor

devnexen commented Dec 21, 2019

sent him a patch, he ought to be able to take it from here.

Co-authored-by: David Carlier <devnexen@gmail.com>
@nodejs-github-bot

This comment has been minimized.

gabrielschulhof added a commit that referenced this pull request Dec 22, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Dec 22, 2019

Landed in 8952105. Thanks for the help with FreeBSD, @devnexen!

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Dec 23, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Dec 23, 2019

Dang! I made a copy/paste error while landing. Sorry I omitted you as a reviewer @jasnell!

@bcoe

This comment has been minimized.

Copy link
Member

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 while in /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?).

@bcoe bcoe mentioned this pull request Dec 26, 2019
0 of 4 tasks complete
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Dec 26, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Dec 26, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Dec 27, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Dec 27, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Dec 27, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Dec 28, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Dec 31, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
BridgeAR added a commit that referenced this pull request Jan 3, 2020
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
@sam-github sam-github mentioned this pull request Jan 6, 2020
3 of 3 tasks complete
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
MylesBorins added a commit that referenced this pull request Jan 8, 2020
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 <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Co-authored-by: David Carlier <devnexen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.