[v10.x] Backport large pages to v10.x #31105
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
c++
|
Sources of conflicts:
Other differences
|
gabrielschulhof
changed the title
gabrielschulhof
force-pushed
the
backport-large-pages-to-v10.x
branch
from
4fce04e
to
0c80d6a
Compare
gabrielschulhof
force-pushed
the
backport-large-pages-to-v10.x
branch
from
0c80d6a
to
d47b068
Compare
gabrielschulhof
force-pushed
the
backport-large-pages-to-v10.x
branch
from
b71affe
to
d31ecce
Compare
gabrielschulhof
force-pushed
the
backport-large-pages-to-v10.x
branch
from
d31ecce
to
38a7862
Compare
gabrielschulhof
commented
Dec 27, 2019
| #ifndef MAP_ANONYMOUS | ||
| #define MAP_ANONYMOUS MAP_ANON | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
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', { |
There was a problem hiding this comment.
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', { |
There was a problem hiding this comment.
... the spot below ...
devnexen
approved these changes
Dec 28, 2019
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 <[email protected]>! PR-URL: nodejs#29914 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: David Carlier <[email protected]>
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 <[email protected]>! PR-URL: nodejs#29973 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-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]>
gabrielschulhof
force-pushed
the
backport-large-pages-to-v10.x
branch
from
350784c
to
9b83046
Compare
gabrielschulhof
requested review from
bnoordhuis,
richardlau,
devnexen and
lundibundi
|
@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! |
richardlau
approved these changes
Mar 6, 2020
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#31095 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
PR-URL: nodejs#31196 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: JungMinu - Minwoo Jung <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
`__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 <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: David Carlier <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: David Carlier <[email protected]> Signed-off-by: Gabriel Schulhof <[email protected]>
The ninja build places objects in a different directory. Co-authored-by: Gabriel Schulhof <[email protected]> Signed-off-by: Richard Lau <[email protected]> PR-URL: nodejs#32071 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>
gabrielschulhof
force-pushed
the
backport-large-pages-to-v10.x
branch
from
9b83046
to
c71d059
Compare
bnoordhuis
approved these changes
Mar 6, 2020
codebytere
approved these changes
Mar 6, 2020
|
@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 |
|
@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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesI'm getting an error locally which I'm hoping won't happen on the CI