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

src: include large pages source unconditionally #31904

Conversation

gabrielschulhof
Copy link
Contributor

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 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
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM % comments

@@ -294,6 +296,25 @@ static bool IsSuperPagesEnabled() {
}
#endif

int IsLargePagesEnabled() {
Copy link
Member

Choose a reason for hiding this comment

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

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.

struct text_region r = FindNodeTextRegion();
if (r.found_text_region == false) {
PrintWarning("failed to find text region");
return -1;
return ENOENT;
}
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@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
Copy link
Collaborator

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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

@gabrielschulhof
Copy link
Contributor Author

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

@gabrielschulhof
Copy link
Contributor Author

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@gabrielschulhof
Copy link
Contributor Author

@nodejs/build

tmp

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
Copy link
Contributor Author

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.

gabrielschulhof pushed a commit that referenced this pull request Feb 24, 2020
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
Copy link
Contributor Author

Landed in 0dff851.

@richardlau
Copy link
Member

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.

codebytere pushed a commit that referenced this pull request Feb 27, 2020
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>
@codebytere codebytere mentioned this pull request Feb 29, 2020
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 6, 2020
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>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 9, 2020
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
Copy link
Member

targos commented Apr 18, 2020

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

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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>
targos pushed a commit that referenced this pull request Apr 28, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants