-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-40611: Adds MAP_POPULATE to the mmap library #20061
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
Conversation
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
- Please sign up the CLA first
- Please add the NEWS.d by using blurb
https://devguide.python.org/committing/#what-s-new-and-news-entries
@corona10 Sorry about that. The CLA is now signed and I added a NEWS.d entry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also update the docs on proper section :).
https://github.com/python/cpython/blob/master/Doc/library/mmap.rst
.. versionchanged:: 3.9
Added MAP_POPULATE constant.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@corona10 Fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am lgtm with the change
Since I am not the native English speaker, I would like to request another reviewer also.
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
Doc/library/mmap.rst
Outdated
These are the various flags that can be passed to :meth:`mmap.mmap`. Note that some options might not be present on some systems. | ||
|
||
.. versionchanged:: 3.10 | ||
Add MAP_POPULATE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correctly rendered? Maybe fix the indentation. I also suggest to add a final "." to your sentence :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. I just fixed this (and changed to past tense to better match other change notices).
Doc/library/mmap.rst
Outdated
@@ -81,7 +81,7 @@ To map anonymous memory, -1 should be passed as the fileno along with the length | |||
private copy-on-write mapping, so changes to the contents of the mmap | |||
object will be private to this process, and :const:`MAP_SHARED` creates a | |||
mapping that's shared with all other processes mapping the same areas of | |||
the file. The default value is :const:`MAP_SHARED`. | |||
the file. The default value is :const:`MAP_SHARED`. Some systems have additional possible flags with the full list specified in :ref:`MAP_* constants <map-constants>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, the doc fits into 80 columns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Fixed!
Misc/NEWS.d/next/Library/2020-05-13-16-28-33.bpo-40611.ZCk0_c.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Victor Stinner <vstinner@python.org>
Thanks for the suggestion. I wasn't aware that was possible. |
Thanks @lalaland, I merged your PR. By the way, I liked your movie! |
Thanks for the help and the reviews! |
This commit adds MAP_POPULATE to the mmap library. MAP_POPULATE is an incredibly useful flag that enables much more efficient IO on linux systems. Currently it is possible to manually pass MAP_POPULATE in using a hardcoded constant, but this change makes it easier by adding it to the set of flags that mmap exposes.
https://bugs.python.org/issue40611