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

[v12.x] src: make --use-largepages a runtime option #31063

Conversation

gabrielschulhof
Copy link
Contributor

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: @bnoordhuis
Reviewed-By: @devnexen
Reviewed-By: @addaleax
Reviewed-By: @cjihrig
Reviewed-By: @gireeshpunathil
Reviewed-By: @lundibundi
Co-authored-by: @devnexen

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 Dec 23, 2019
@gabrielschulhof gabrielschulhof changed the title src: make --use-largepages a runtime option [v12.x] src: make --use-largepages a runtime option Dec 23, 2019
@@ -333,9 +333,6 @@ MoveTextRegionToLargePages(const text_region& r) {
PrintSystemError(errno);
return -1;
}
OnScopeLeave munmap_on_return([nmem, size]() {
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 was the only conflict. It was expecting to remove auto munmap_on_return = OnScopeLeave(...); and not OnScopeLeave munmap_on_return(...);

@nodejs-github-bot

This comment has been minimized.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 24, 2019
Re-introduce the build-time option as a no-op in order to retain
backward compatibility for LTS purposes.

Re: nodejs#31063 (review)
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Dec 24, 2019

@addaleax IINM there's still some buffer-freeing-related flakiness: https://ci.nodejs.org/job/node-test-commit-plinux/29916/nodes=centos7-ppcle/console

Edit: Whoops 🙂 Wrong PR 🙂

@addaleax
Copy link
Member

@thangktran ^^^ fyi

gabrielschulhof pushed a commit that referenced this pull request Dec 26, 2019
Re-introduce the build-time option as a no-op in order to retain
backward compatibility for LTS purposes.

Re: #31063 (review)
PR_URL: #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]>
@gabrielschulhof
Copy link
Contributor Author

@richardlau I have landed #31075 and I have cherry-picked it into this backport.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

MylesBorins pushed a commit that referenced this pull request Jan 8, 2020
Backport-PR-URL: #31063
PR-URL: #31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2020
The usage of the relevant methods from the file is conditional so make
the include conditional too.

Backport-PR-URL: #31063
PR-URL: #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]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2020
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: #31063 (comment)
Backport-PR-URL: #31063
PR-URL: #31103
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@MylesBorins
Copy link
Contributor

landed in 54635f5...f5cd6d7

@MylesBorins MylesBorins closed this Jan 8, 2020
BethGriggs pushed a commit that referenced this pull request Feb 6, 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 <[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]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Re-introduce the build-time option as a no-op in order to retain
backward compatibility for LTS purposes.

Re: #31063 (review)
Backport-PR-URL: #31063
PR_URL: #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]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Backport-PR-URL: #31063
PR-URL: #31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
The usage of the relevant methods from the file is conditional so make
the include conditional too.

Backport-PR-URL: #31063
PR-URL: #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]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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: #31063 (comment)
Backport-PR-URL: #31063
PR-URL: #31103
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@MylesBorins
Copy link
Contributor

Reopening as I am proposing a reversion of this change

@MylesBorins MylesBorins reopened this Feb 13, 2020
@MylesBorins MylesBorins mentioned this pull request Feb 13, 2020
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request 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]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 6, 2020
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]>
@targos
Copy link
Member

targos commented Mar 7, 2020

New PR: #32092

@targos targos closed this Mar 7, 2020
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 9, 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]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 9, 2020
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]>
@gabrielschulhof gabrielschulhof deleted the backport-largepages-runtime-to-v12.x branch April 21, 2020 02:58
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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 <[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]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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 <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
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 <[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]>
targos pushed a commit that referenced this pull request Apr 28, 2020
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 <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.