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

gabrielschulhof · 2019-12-23T06:23:30Z

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

gabrielschulhof · 2019-12-23T09:15:46Z

src/large_pages/node_large_page.cc

@@ -333,9 +333,6 @@ MoveTextRegionToLargePages(const text_region& r) {
    PrintSystemError(errno);
    return -1;
  }
-  OnScopeLeave munmap_on_return([nmem, size]() {


This was the only conflict. It was expecting to remove auto munmap_on_return = OnScopeLeave(...); and not OnScopeLeave munmap_on_return(...);

configure.py

Re-introduce the build-time option as a no-op in order to retain backward compatibility for LTS purposes. Re: nodejs#31063 (review)

gabrielschulhof · 2019-12-24T17:09:59Z

@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 · 2019-12-24T18:08:19Z

@thangktran ^^^ fyi

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

gabrielschulhof · 2019-12-26T02:45:11Z

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

Backport-PR-URL: #31063 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>

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

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 <riclau@uk.ibm.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

MylesBorins · 2020-01-08T01:06:01Z

landed in 54635f5...f5cd6d7

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>

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

Backport-PR-URL: #31063 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>

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

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 <riclau@uk.ibm.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

MylesBorins · 2020-02-13T23:29:05Z

Reopening as I am proposing a reversion of this change

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>

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>

targos · 2020-03-07T09:15:03Z

New PR: #32092

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>

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>

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>

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>

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>

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>

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 changed the title ~~src: make --use-largepages a runtime option~~ [v12.x] src: make --use-largepages a runtime option Dec 23, 2019

gabrielschulhof requested review from devnexen, bnoordhuis and addaleax December 23, 2019 09:13

gabrielschulhof commented Dec 23, 2019

View reviewed changes

devnexen approved these changes Dec 23, 2019

View reviewed changes

addaleax approved these changes Dec 24, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

richardlau reviewed Dec 24, 2019

View reviewed changes

configure.py Show resolved Hide resolved

gabrielschulhof mentioned this pull request Dec 24, 2019

build: re-introduce --use-largepages as no-op #31075

Closed

2 tasks

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 24, 2019

build: re-introduce --use-largepages as no-op

68880f4

Re-introduce the build-time option as a no-op in order to retain backward compatibility for LTS purposes. Re: nodejs#31063 (review)

This comment has been minimized.

Sign in to view

richardlau approved these changes Dec 26, 2019

View reviewed changes

gabrielschulhof mentioned this pull request Dec 26, 2019

build: warn upon --use-largepages config option #31103

Closed

3 tasks

This comment has been minimized.

Sign in to view

MylesBorins closed this Jan 8, 2020

MylesBorins reopened this Feb 13, 2020

MylesBorins mentioned this pull request Feb 13, 2020

12.x reverts #31782

Closed

targos closed this Mar 7, 2020

gabrielschulhof deleted the backport-largepages-runtime-to-v12.x branch April 21, 2020 02:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

gabrielschulhof commented Dec 23, 2019

Uh oh!

gabrielschulhof Dec 23, 2019

Uh oh!

This comment has been minimized.

Uh oh!

gabrielschulhof commented Dec 24, 2019 •

edited

Loading

Uh oh!

addaleax commented Dec 24, 2019

Uh oh!

gabrielschulhof commented Dec 26, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

MylesBorins commented Jan 8, 2020

Uh oh!

MylesBorins commented Feb 13, 2020

Uh oh!

targos commented Mar 7, 2020

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

gabrielschulhof commented Dec 23, 2019

Checklist

Uh oh!

gabrielschulhof Dec 23, 2019

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

gabrielschulhof commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Dec 24, 2019

Uh oh!

gabrielschulhof commented Dec 26, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

MylesBorins commented Jan 8, 2020

Uh oh!

MylesBorins commented Feb 13, 2020

Uh oh!

targos commented Mar 7, 2020

Uh oh!

Uh oh!

gabrielschulhof commented Dec 24, 2019 •

edited

Loading