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: further simplify large pages code #32576

Conversation

gabrielschulhof
Copy link
Contributor

  • 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: #32570
Signed-off-by: @gabrielschulhof

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

This is an alternative to #32570.

* 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>
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 31, 2020
@nodejs-github-bot
Copy link
Collaborator

return -1;
status = -1;
done:
if (nmem != nullptr && nmem != MAP_FAILED && munmap(nmem, size) == -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe possible to avoid code repetition here what do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right, with a (nmem != nullptr && ... ) || (tmem != nullptr && ... ). @jasnell had suggested that previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, actually, you wanna munmap them both, even if the first one fails. So, you may PrintSystemError() because the first munmap failed, because the second munmap failed, twice because they both failed, or not at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT to avoid this code repetition we have to go all the way to #32570 which I feel is a bit too much.

Copy link
Contributor Author

@gabrielschulhof gabrielschulhof Mar 31, 2020

Choose a reason for hiding this comment

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

We could do __attribute__((cleanup("DeleteMmapPointer"))) for nmem and tmem but I'm not sure how portable that is and we would have two levels of indirection because we'd have to store nmem along with size in some structure which would be passed to the deleter. Same for tmem.

Again, it seems like too much complication for avoiding a single repetition.

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

I’d prefer #32570 here.

@gabrielschulhof
Copy link
Contributor Author

@devnexen I'll go with @addaleax' preference, which will help us avoid the repetition. Thanks for reviewing!

@gabrielschulhof gabrielschulhof deleted the large-pages-remove-on-scope-leave branch April 21, 2020 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants