Skip to content

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

Merged
merged 8 commits into from
May 26, 2020

Conversation

EthanSteinberg
Copy link
Contributor

@EthanSteinberg EthanSteinberg commented May 13, 2020

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

@the-knights-who-say-ni
Copy link

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 username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@lalaland

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!

Copy link
Member

@corona10 corona10 left a 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.

  1. Please sign up the CLA first
  2. Please add the NEWS.d by using blurb
    https://devguide.python.org/committing/#what-s-new-and-news-entries

@EthanSteinberg
Copy link
Contributor Author

@corona10 Sorry about that. The CLA is now signed and I added a NEWS.d entry.

@EthanSteinberg EthanSteinberg requested a review from corona10 May 13, 2020 16:30
Copy link
Member

@corona10 corona10 left a 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.

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@EthanSteinberg
Copy link
Contributor Author

@corona10 Fixed!

@EthanSteinberg EthanSteinberg requested a review from corona10 May 18, 2020 18:08
Copy link
Member

@corona10 corona10 left a 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>
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
Copy link
Member

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

Copy link
Contributor Author

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

@@ -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>`.
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Fixed!

@EthanSteinberg EthanSteinberg requested a review from vstinner May 21, 2020 21:01
Co-authored-by: Victor Stinner <vstinner@python.org>
@EthanSteinberg
Copy link
Contributor Author

Thanks for the suggestion. I wasn't aware that was possible.

@EthanSteinberg EthanSteinberg requested a review from vstinner May 22, 2020 17:05
@vstinner vstinner merged commit 21fda91 into python:master May 26, 2020
@vstinner
Copy link
Member

Thanks @lalaland, I merged your PR. By the way, I liked your movie!

@EthanSteinberg
Copy link
Contributor Author

Thanks for the help and the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants