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.

Sign up for GitHub

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

Jump to bottom

[v12.x] Backport large pages asm to v12.x #32092

Closed
wants to merge 9 commits into from
Closed

[v12.x] Backport large pages asm to v12.x #32092

wants to merge 9 commits into from

Conversation

gabrielschulhof
Copy link
Contributor

This re-backports the large pages feature as a runtime option, including the fixes that make it work with the gold linker.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Mar 4, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29542/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29557/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29579/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29586/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29591/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29592/

@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 7, 2020
@targos targos mentioned this pull request Mar 7, 2020
Closed
4 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29713/

@gabrielschulhof gabrielschulhof mentioned this pull request Mar 12, 2020
Closed
4 tasks
@codebytere codebytere force-pushed the v12.x-staging branch 2 times, most recently from 63a03d2 to d577190 Compare March 31, 2020 23:57
Gabriel Schulhof and others added 4 commits April 9, 2020 10:48
@devnexen
src: make --use-largepages a runtime option
02ea3b0 
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>
build: re-introduce --use-largepages as no-op
db8f1b0 
Re-introduce the build-time option as a no-op in order to retain
backward compatibility for LTS purposes.

Re: nodejs#31063 (review)
PR_URL: nodejs#31075
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@bcoe
build: switch realpath to pwd
17fa34c 
PR-URL: nodejs#31095
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@lundibundi
src: make large_pages node.cc include conditional
58ab4b5 
The usage of the relevant methods from the file is conditional so make
the include conditional too.

PR-URL: nodejs#31078
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Gabriel Schulhof and others added 2 commits April 9, 2020 10:48
src: start the .text section with an asm symbol
f2572a1 
We create an object file in assembly which introduces the symbol
`__node_text_start` into the .text section and place the resulting
object file as the first file the linker encounters. We do this to
ensure that we can recognize the boundaries of the .text section when
attempting to establish the address range to map to large pages.

Additionally, we rename the section containing the remapping code from
`.lpstub` to `lpstub` so as to take advantage of the linker's feature
whereby it inserts the symbol `__start_lpstub` when the section's name
can be rendered as a valid C variable. We need this symbol in order to
avoid self-mapping the remapping code to large pages, because doing so
would cause the process to crash.

PR-URL: nodejs#31981
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@richardlau
build: fix building with ninja
0c391fb 
The ninja build places objects in a different directory.

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>

PR-URL: nodejs#32071
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@gabrielschulhof gabrielschulhof force-pushed the backport-large-pages-asm-to-v12.x branch from 3f60f7f to 0c391fb Compare April 9, 2020 17:49
@gabrielschulhof
Copy link
Contributor Author

Rebased to address conflict. Interestingly, the rebase showed no conflicts when performed locally 🤷

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30596/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30599/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30633/

@targos
Copy link
Member

targos commented Apr 25, 2020

Thanks. Landed on my release preparation branch: https://github.com/targos/node/commits/prepare-minor

@targos targos closed this Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@devnexen @targos
src: make --use-largepages a runtime option
9d425f0 
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 <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>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@targos
build: re-introduce --use-largepages as no-op
4d71000 
Re-introduce the build-time option as a no-op in order to retain
backward compatibility for LTS purposes.

Backport-PR-URL: nodejs#32092
Re: nodejs#31063 (review)
PR_URL: nodejs#31075
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@bcoe @targos
build: switch realpath to pwd
22ff686 
Backport-PR-URL: nodejs#32092
PR-URL: nodejs#31095
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@lundibundi @targos
src: make large_pages node.cc include conditional
63327e3 
The usage of the relevant methods from the file is conditional so make
the include conditional too.

Backport-PR-URL: nodejs#32092
PR-URL: nodejs#31078
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@targos
build: warn upon --use-largepages config option
a5471cc 
Emit a warning when the user configures with `--use-largepages` and/or
`--use-largepages-script-lld` informing them that the option is now
available as a Node.js runtime option once it is built.

Backport-PR-URL: nodejs#32092
Refs: nodejs#31063 (comment)
PR-URL: nodejs#31103
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@bnoordhuis @targos
src: use __executable_start for linux hugepages
4bae131 
`__executable_start` is provided by GNU's and LLVM's default linker
scripts, obviating the need to plug in a custom linker script.

The problem with our bespoke linker script is that it works with ld.bfd
but not ld.gold and cannot easily be ported because the latter linker
doesn't understand the `INSERT BEFORE` directive.

The /proc/self/maps scanner is updated to account for the fact that
there are a number of sections between `&__executable_start` and
the start of the .text section.

Fortunately, those sections are all mapped into the same memory segment
so we only need to look at the next line to find the start of our text
segment.

Fixes: nodejs#31520

Backport-PR-URL: nodejs#32092
PR-URL: nodejs#31547
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@targos
src: include large pages source unconditionally
9066384 
Restrict the usage of the C preprocessor directive enabling large
pages support to the large pages implementation. This cleans up the
code in src/node.cc.

Backport-PR-URL: nodejs#32092
PR-URL: nodejs#31904
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@targos
src: start the .text section with an asm symbol
b3d179e 
We create an object file in assembly which introduces the symbol
`__node_text_start` into the .text section and place the resulting
object file as the first file the linker encounters. We do this to
ensure that we can recognize the boundaries of the .text section when
attempting to establish the address range to map to large pages.

Additionally, we rename the section containing the remapping code from
`.lpstub` to `lpstub` so as to take advantage of the linker's feature
whereby it inserts the symbol `__start_lpstub` when the section's name
can be rendered as a valid C variable. We need this symbol in order to
avoid self-mapping the remapping code to large pages, because doing so
would cause the process to crash.

Backport-PR-URL: nodejs#32092
PR-URL: nodejs#31981
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@richardlau @targos
build: fix building with ninja
924b116 
The ninja build places objects in a different directory.

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>

Backport-PR-URL: nodejs#32092
PR-URL: nodejs#32071
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@devnexen @targos
src: make --use-largepages a runtime option
6dcb868 
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 <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>
targos pushed a commit that referenced this pull request Apr 28, 2020
@targos
build: re-introduce --use-largepages as no-op
0190a62 
Re-introduce the build-time option as a no-op in order to retain
backward compatibility for LTS purposes.

Backport-PR-URL: #32092
Re: #31063 (review)
PR_URL: #31075
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Apr 28, 2020
@bcoe @targos
build: switch realpath to pwd
c3efd2c 
Backport-PR-URL: #32092
PR-URL: #31095
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Apr 28, 2020
@lundibundi @targos
src: make large_pages node.cc include conditional
1e95bb8 
The usage of the relevant methods from the file is conditional so make
the include conditional too.

Backport-PR-URL: #32092
PR-URL: #31078
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@targos
build: warn upon --use-largepages config option
e097980 
Emit a warning when the user configures with `--use-largepages` and/or
`--use-largepages-script-lld` informing them that the option is now
available as a Node.js runtime option once it is built.

Backport-PR-URL: #32092
Refs: #31063 (comment)
PR-URL: #31103
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@bnoordhuis @targos
src: use __executable_start for linux hugepages
5ea3d60 
`__executable_start` is provided by GNU's and LLVM's default linker
scripts, obviating the need to plug in a custom linker script.

The problem with our bespoke linker script is that it works with ld.bfd
but not ld.gold and cannot easily be ported because the latter linker
doesn't understand the `INSERT BEFORE` directive.

The /proc/self/maps scanner is updated to account for the fact that
there are a number of sections between `&__executable_start` and
the start of the .text section.

Fortunately, those sections are all mapped into the same memory segment
so we only need to look at the next line to find the start of our text
segment.

Fixes: #31520

Backport-PR-URL: #32092
PR-URL: #31547
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@targos
src: include large pages source unconditionally
809d8b5 
Restrict the usage of the C preprocessor directive enabling large
pages support to the large pages implementation. This cleans up the
code in src/node.cc.

Backport-PR-URL: #32092
PR-URL: #31904
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@targos
src: start the .text section with an asm symbol
178c682 
We create an object file in assembly which introduces the symbol
`__node_text_start` into the .text section and place the resulting
object file as the first file the linker encounters. We do this to
ensure that we can recognize the boundaries of the .text section when
attempting to establish the address range to map to large pages.

Additionally, we rename the section containing the remapping code from
`.lpstub` to `lpstub` so as to take advantage of the linker's feature
whereby it inserts the symbol `__start_lpstub` when the section's name
can be rendered as a valid C variable. We need this symbol in order to
avoid self-mapping the remapping code to large pages, because doing so
would cause the process to crash.

Backport-PR-URL: #32092
PR-URL: #31981
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@richardlau @targos
build: fix building with ninja
49ddd36 
The ninja build places objects in a different directory.

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>

Backport-PR-URL: #32092
PR-URL: #32071
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@gabrielschulhof gabrielschulhof deleted the backport-large-pages-asm-to-v12.x branch January 28, 2021 00:14
@gabrielschulhof gabrielschulhof restored the backport-large-pages-asm-to-v12.x branch January 28, 2021 05:40
@gabrielschulhof gabrielschulhof deleted the backport-large-pages-asm-to-v12.x branch February 3, 2021 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@richardlau richardlau richardlau approved these changes

@devnexen devnexen devnexen approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@MylesBorins MylesBorins Awaiting requested review from MylesBorins

@bcoe bcoe Awaiting requested review from bcoe

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@gabrielschulhof @nodejs-github-bot @targos @bnoordhuis @devnexen @richardlau @lundibundi @bcoe