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.

Sign up for GitHub

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

Jump to bottom

src: include large pages source unconditionally #31904

Closed
wants to merge 3 commits into from
Closed

src: include large pages source unconditionally #31904

wants to merge 3 commits into from

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

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

bnoordhuis
bnoordhuis approved these changes Feb 21, 2020
View reviewed changes
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

src/large_pages/node_large_page.cc Outdated
@@ -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.

src/large_pages/node_large_page.cc Outdated
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?

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

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

bnoordhuis
bnoordhuis approved these changes Feb 21, 2020
View reviewed changes
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

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@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
src: include large pages source unconditionally
0dff851 
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
@codebytere
src: include large pages source unconditionally
776f379 
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
Merged
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 6, 2020
src: include large pages source unconditionally
8a7f607 
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
src: include large pages source unconditionally
2f3d446 
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
@targos
src: include large pages source unconditionally
9066384 
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
@targos
src: include large pages source unconditionally
809d8b5 
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>
@targos targos mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@devnexen devnexen devnexen approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gabrielschulhof @nodejs-github-bot @richardlau @targos @bnoordhuis @devnexen @lundibundi