src: include large pages source unconditionally by gabrielschulhof · Pull Request #31904 · nodejs/node

gabrielschulhof · 2020-02-21T16:37:14Z

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.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2020-02-21T16:49:37Z

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

bnoordhuis

LGTM % comments

bnoordhuis · 2020-02-21T17:31:54Z

src/large_pages/node_large_page.cc

Can you name this e.g. GetLargePagesSupport()? IsLargePagesEnabled() suggests it returns a boolean when it doesn't.

Alternatively, you could just inline it at the call site; there's only one caller.

bnoordhuis · 2020-02-21T17:34:04Z

src/large_pages/node_large_page.cc

Can you drop the braces?

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.

gabrielschulhof · 2020-02-21T17:52:38Z

@lundibundi @devnexen @bnoordhuis I made the requested changes, and I also made the entire implementation conditional on the preprocessor flag being defined. That is, if the flag is not defined as 1, only LargePagesError() will be defined, and MapStaticCodeToLargePages() will be defined as returning ENOTSUP.

nodejs-github-bot · 2020-02-21T17:53:45Z

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

bnoordhuis

LGTM but I suspect the linter will complain about two long lines.

gabrielschulhof · 2020-02-21T18:00:21Z

@bnoordhuis make lint-cpp did not complain when run locally 🤷

gabrielschulhof · 2020-02-21T18:04:00Z

OTOH, cerrno must be included unconditionally, I think, because it's not included on some platforms.

nodejs-github-bot · 2020-02-21T18:04:55Z

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

nodejs-github-bot · 2020-02-21T18:18:45Z

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

nodejs-github-bot · 2020-02-21T18:21:00Z

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

nodejs-github-bot · 2020-02-21T19:43:26Z

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

nodejs-github-bot · 2020-02-24T17:42:41Z

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

gabrielschulhof · 2020-02-24T17:48:48Z

@nodejs/build

This is not part of node-test-pull-request, AFAICT, so I don't know why it's here. How do I get a full-green run if this is not part of node-test-pull-request?

gabrielschulhof · 2020-02-24T21:56:49Z

AFAICT the failing test suite was re-run at https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1804_sharedlibs_shared_x64/18250/, just not updated in the statuses.

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: #31904 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

gabrielschulhof · 2020-02-24T21:59:32Z

Landed in 0dff851.

richardlau · 2020-02-24T23:39:11Z

AFAICT the failing test suite was re-run at https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1804_sharedlibs_shared_x64/18250/, just not updated in the statuses.

Yes, the containered jobs currently only post status on failure which means they don’t get cleared when a subsequent run passes. It’s on my list of things to look at.

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: #31904 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

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>

targos · 2020-04-18T09:28:59Z

depends on the large pages change to land on v12.x

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>

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>

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 21, 2020

gabrielschulhof requested review from bnoordhuis, devnexen and lundibundi February 21, 2020 16:37

bnoordhuis approved these changes Feb 21, 2020

View reviewed changes

gabrielschulhof force-pushed the improve-lp-cpp-flag-handling branch from 8a72d89 to 1ad9e86 Compare February 21, 2020 17:35

devnexen approved these changes Feb 21, 2020

View reviewed changes

lundibundi approved these changes Feb 21, 2020

View reviewed changes

src: include large pages source unconditionally

2d72f17

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.

gabrielschulhof force-pushed the improve-lp-cpp-flag-handling branch from 1ad9e86 to 2d72f17 Compare February 21, 2020 17:49

gabrielschulhof requested review from bnoordhuis, devnexen and lundibundi February 21, 2020 17:50

bnoordhuis approved these changes Feb 21, 2020

View reviewed changes

cerrno must be included unconditionally

cdb7f73

devnexen approved these changes Feb 21, 2020

View reviewed changes

make the opening of the node namespace unconditional

4327245

lundibundi approved these changes Feb 22, 2020

View reviewed changes

gabrielschulhof closed this Feb 24, 2020

codebytere mentioned this pull request Feb 29, 2020

v13.10.0 proposal #32027

Merged

targos added the backport-blocked-v12.x label Apr 18, 2020

targos added backported-to-v12.x and removed backport-blocked-v12.x labels Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Uh oh!

Conversation

gabrielschulhof commented Feb 21, 2020

Checklist

Uh oh!

nodejs-github-bot commented Feb 21, 2020

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Feb 21, 2020

Uh oh!

nodejs-github-bot commented Feb 21, 2020

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Feb 21, 2020

Uh oh!

gabrielschulhof commented Feb 21, 2020

Uh oh!

nodejs-github-bot commented Feb 21, 2020

Uh oh!

nodejs-github-bot commented Feb 21, 2020

Uh oh!

nodejs-github-bot commented Feb 21, 2020

Uh oh!

nodejs-github-bot commented Feb 21, 2020

Uh oh!

nodejs-github-bot commented Feb 24, 2020

Uh oh!

gabrielschulhof commented Feb 24, 2020

Uh oh!

gabrielschulhof commented Feb 24, 2020

Uh oh!

gabrielschulhof commented Feb 24, 2020

Uh oh!

richardlau commented Feb 24, 2020

Uh oh!

targos commented Apr 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants