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: munmap(2) upon class instance destructor #32570

Closed
wants to merge 1 commit into from
Closed

src: munmap(2) upon class instance destructor #32570

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

Replace OnScopeLeave with a class whose instance destructor performs
the munmap(2).

Signed-off-by: @gabrielschulhof
Fixes: #32532

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 30, 2020
addaleax
addaleax approved these changes Mar 30, 2020
View reviewed changes
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
src/large_pages/node_large_page.cc Show resolved Hide resolved
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
@gabrielschulhof
Copy link
Contributor Author

@addaleax @jasnell I have addressed your review comments.

@nodejs-github-bot
Copy link
Collaborator

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

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2020
src: further simplify large pages code
c9b1139 
* Clean up and co-locate explanation for what the code does to the top
  of the file.
* Remove extraneous headers.
* Remove the need for `OnScopeLeave` replacing it with a pair of labels
  `fail:` and `done:`.

Re: nodejs#32570
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@gabrielschulhof gabrielschulhof mentioned this pull request Mar 31, 2020
Closed
2 tasks
@gabrielschulhof
Copy link
Contributor Author

@jasnell @addaleax @devnexen having to declare the code for the class in the lpstub section I feel puts the complexity of using such a class beyond its usefulness. It's good to avoid placing too many things into that section, and especially classes. I have cleaned up the comments and includes in addition to what I did so far to make things cleaner, but I believe that #32576 is the simpler, cleaner solution. WDYT?

@nodejs-github-bot
Copy link
Collaborator

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

addaleax
addaleax approved these changes Mar 31, 2020
View reviewed changes
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
@gabrielschulhof
Copy link
Contributor Author

@addaleax I have used FORCE_INLINE from debug_utils-inl.h.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 31, 2020 •
edited by gabrielschulhof
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/30283/ ✔️

bnoordhuis
bnoordhuis reviewed Apr 1, 2020
View reviewed changes
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
@gabrielschulhof gabrielschulhof force-pushed the large-pages-mmap-unique-pointer branch from 74779ed to c747290 Compare April 2, 2020 00:18
@gabrielschulhof
Copy link
Contributor Author

@bnoordhuis I have made the changes you suggested.

@gabrielschulhof gabrielschulhof force-pushed the large-pages-mmap-unique-pointer branch 2 times, most recently from 0022b2e to a4a21f8 Compare April 2, 2020 00:23
@gabrielschulhof gabrielschulhof force-pushed the large-pages-mmap-unique-pointer branch from a4a21f8 to 1f9ee3e Compare April 2, 2020 03:30
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax @bnoordhuis
src: munmap(2) upon class instance destructor
763da86 
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: nodejs#32532
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
@gabrielschulhof gabrielschulhof force-pushed the large-pages-mmap-unique-pointer branch from 6686288 to 763da86 Compare April 3, 2020 17:22
@gabrielschulhof
Copy link
Contributor Author

Rebased.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 6, 2020 •
edited by gabrielschulhof
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/30513/ ✔️

gabrielschulhof pushed a commit that referenced this pull request Apr 6, 2020
@addaleax @bnoordhuis
src: munmap(2) upon class instance destructor
a50745e 
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: #32532
PR-URL: #32570
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@gabrielschulhof
Copy link
Contributor Author

Landed in a50745e.

@gabrielschulhof gabrielschulhof deleted the large-pages-mmap-unique-pointer branch April 6, 2020 19:55
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
@addaleax @bnoordhuis @BethGriggs
src: munmap(2) upon class instance destructor
af6cbf5 
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: #32532
PR-URL: #32570
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request Apr 12, 2020
@addaleax @bnoordhuis @targos
src: munmap(2) upon class instance destructor
d779980 
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: #32532
PR-URL: #32570
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@targos targos mentioned this pull request Apr 13, 2020
Merged
@targos
Copy link
Member

targos commented Apr 22, 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
@addaleax @bnoordhuis @targos
src: munmap(2) upon class instance destructor
4277375 
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: nodejs#32532
PR-URL: nodejs#32570
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@addaleax @bnoordhuis @targos
src: munmap(2) upon class instance destructor
9a010a3 
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: #32532
PR-URL: #32570
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@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

@addaleax addaleax addaleax approved these changes

@devnexen devnexen devnexen approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
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.

Convert mmap()s to smart pointers
7 participants
@gabrielschulhof @nodejs-github-bot @targos @bnoordhuis @jasnell @addaleax @devnexen