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

[v10.x] Backport large pages to v10.x #31105

Closed
wants to merge 18 commits into from
Closed

[v10.x] Backport large pages to v10.x #31105

wants to merge 18 commits into from

Conversation

gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Dec 26, 2019 •
edited

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
    I'm getting an error locally which I'm hoping won't happen on the CI
  • 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. v10.x labels Dec 26, 2019
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Dec 26, 2019 •
edited

Sources of conflicts:

  • In master we rearranged #includes to comply with the linter
  • In master we added stuff to node.gyp, changing the diff context of the large-pages-related additions
  • The place at startup where we add the code that attempts the mapping is different in v10.x
  • The way we access processed command line parameters is different in v10.x

Other differences

  • llvm_version is 0 when not building with clang, not "0.0"
  • Since v10.x supports OSX10.10, we must define MAP_ANONYMOUS as MAP_ANON on OSX if MAP_ANONYMOUS is unavailable, because MAP_ANONYMOUS was added in OSX10.11

@nodejs-github-bot
Copy link
Collaborator

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

@gabrielschulhof gabrielschulhof changed the title Backport large pages to v10.x [v10.x] Backport large pages to v10.x Dec 26, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

gabrielschulhof
gabrielschulhof commented Dec 27, 2019
View reviewed changes
src/large_pages/node_large_page.cc
#ifndef MAP_ANONYMOUS
#define MAP_ANONYMOUS MAP_ANON
#endif
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a v10.x-only change

node.gypi Outdated
[ 'OS=="linux" and target_arch=="x64" and node_use_large_pages=="true"', {
[ 'OS=="linux" and '
'target_arch=="x64" and '
'llvm_version==0', {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the spot below is where I replaced "0.0" with 0

node.gypi Outdated
}],
[ 'OS=="linux" and '
'target_arch=="x64" and '
'llvm_version!=0', {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... the spot below ...

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

Gabriel Schulhof and others added 3 commits March 5, 2020 14:06
src: fix largepages regression
2b373bd 
During the addition of macOS support for large pages, a `memcpy` ended
up under the wrong preprocessor directive. As a result, the newly
allocated large pages were not initialized with a copy of the text
section.

Thanks to Suresh Srinivas <suresh.srinivas@intel.com>!

PR-URL: nodejs#29914
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
src: refine maps parsing for large pages
d5b5b22 
Multiple sections may be marked as "r-xp" and with the executable's
path. We use the location of the `__nodetext` symbol added by the linker
script to ensure that the range we retrieve from the maps file does
indeed contain the Node.js text section.

Thanks to Suresh Srinivas <suresh.srinivas@intel.com>!

PR-URL: nodejs#29973
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@devnexen
src: make --use-largepages a runtime option
b4d01ba 
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
Copy link
Contributor Author

@bnoordhuis @devnexen @lundibundi @richardlau I backported all the changes and fixes that have happened on master in the interim. That's effectively a re-backport. Please take another look!

Gabriel Schulhof and others added 10 commits March 5, 2020 16:35
build: re-introduce --use-largepages as no-op
7502e31 
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
ed5e50d 
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
7075e9d 
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>
build: warn upon --use-largepages config option
88b76d1 
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.

Refs: nodejs#31063 (comment)
PR-URL: nodejs#31103
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@lundibundi
src: clean up large_pages code
75947e1 
PR-URL: nodejs#31196
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
src: reduce large pages code duplication
2693510 
Declaring and initializing a `struct text_region` is common to all
three implementations of the large pages mapping code. Let's make it
unconditional.

PR-URL: nodejs#31385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: JungMinu - Minwoo Jung <jmwsoft@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
@bnoordhuis
src: use __executable_start for linux hugepages
56305ee 
`__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

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>
src: include large pages source unconditionally
8a7f607 
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.

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>
src: start the .text section with an asm symbol
f3643bf 
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
c71d059 
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>
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@MylesBorins
Copy link
Member

@gabrielschulhof given the number of regressions that were introduced by this change and the subsequent fixes I have to be honest that I am reluctant to see this land in 10.x just before it goes into maintenance.

Can you share a bit more about the importance of this feature and why we should prioritize for 10.x?

To be clear, not trying to bring stop energy, just want to have a full picture if I'm going to make any sort of recommendation

@gabrielschulhof
Copy link
Contributor Author

@MylesBorins that's fair. Having the feature in v13.x and, hopefully, with #32092 landed, in v12.x should give it enough exposure, as well as shakedown time.

@gabrielschulhof gabrielschulhof deleted the backport-large-pages-to-v10.x branch April 21, 2020 02:58
@richardlau richardlau mentioned this pull request Jul 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@codebytere codebytere codebytere approved these changes

@richardlau richardlau richardlau approved these changes

@addaleax addaleax Awaiting requested review from addaleax

@devnexen devnexen Awaiting requested review from devnexen

@lundibundi lundibundi Awaiting requested review from lundibundi

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.

None yet
10 participants
@gabrielschulhof @nodejs-github-bot @richardlau @BethGriggs @MylesBorins @bnoordhuis @codebytere @devnexen @lundibundi @bcoe