FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-117151: IO performance improvement, increase io.DEFAULT_BUFFER_SIZE to 128k by morotti · Pull Request #118144 · python/cpython · GitHub

/ cpython Public

gh-117151: IO performance improvement, increase io.DEFAULT_BUFFER_SIZE to 128k - #118144

Merged
gpshead merged 19 commits into
python:mainfrom
man-group:io-buffer-size
Mar 7, 2025
Merged

gh-117151: IO performance improvement, increase io.DEFAULT_BUFFER_SIZE to 128k#118144
gpshead merged 19 commits into
python:mainfrom
man-group:io-buffer-size

Conversation

morotti commented Apr 22, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

See discussion gh-117151

This patch adjusts the buffer size. That gives 3 to 5 times I/O performance improvement on modern hardware.

  • increase io.DEFAULT_BUFFER_SIZE to 128k
  • fix open() to use max(st_blksize, io.DEFAULT_BUFFER_SIZE)

ghost commented Apr 22, 2024
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

bedevere-app Bot commented Apr 22, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

encukou added the performance Performance or resource usage label Apr 22, 2024

bedevere-app Bot commented Apr 22, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

morotti commented Apr 25, 2024

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka hello, you were kind to review my first PR for buffering performance (118037), would you be able to review this PR too?

eendebakpt commented Apr 30, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

@morotti You still need to write a news entry for the PR. For your other PR this is was not required, but due to the performance impact of this PR I think we should. The most convenient way for this is to click on the Details button next to bedevere/news of the CI checks which will open a tool to add the news entry. For more information also see https://devguide.python.org/core-developers/committing/#updating-news-and-what-s-new-in-python

The CLA is not yet working, but it worked for the other PR (which was created after this one), so maybe it will resolve itself automatically in a couple of days

Comment thread Lib/_pyio.py

Copy link
Copy Markdown
Contributor

Pinging @benjaminp as expert on io. Could you review this PR? Are there other benchmarks (besides the one in the corresponding issue) we can perform to test this PR?

morotti commented Apr 30, 2024

Copy link
Copy Markdown
Contributor Author

Thank you for reviewing,

The CLA check is green now and I added a news entry.

morotti commented May 17, 2024

Copy link
Copy Markdown
Contributor Author

@eendebakpt @benjaminp could you review?

Copy link
Copy Markdown
Contributor

@eendebakpt @benjaminp could you review?

@morotti The PR looks good from my side, but I am no core dev so I cannot approve. Currently many core devs are at pycon US, so I think we should wait a bit more. If in a few weeks there has been no further response, we can post a message to discourse (see https://devguide.python.org/getting-started/pull-request-lifecycle/#reviewing).

itamaro added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 23, 2024

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @itamaro for commit 3668d1a 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 23, 2024
itamaro added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 1, 2024

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @itamaro for commit 28e7bb7 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 1, 2024

Copy link
Copy Markdown
Contributor

Requesting Serhiy's review, since he reviewd the other linked (and merged) PR #118037.

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I was convinced that such a change could be beneficial.

But for the case if it has unexpected effect, please ask on https://discuss.python.org/. Update also the C implementation of open() which is used by default.

Comment thread Lib/_pyio.py Outdated

Copy link
Copy Markdown
Member

Please do not use rebase and force-push. It makes reviewing more difficult.

morotti commented Feb 10, 2025

Copy link
Copy Markdown
Contributor Author

alright, I've removed the constants _MAXIMUM_BUFFER_SIZE. build green.

I think we're good to merge

Copy link
Copy Markdown
Contributor

(rebasing on master again because CI builds no longer work)

Please use git merge --no-ff main instead of rebasing. This has already been pointed out in earlier review.

morotti commented Feb 11, 2025
edited
Loading

Copy link
Copy Markdown
Contributor Author

I've updated the branch to main, anything else you want?

morotti commented Feb 17, 2025

Copy link
Copy Markdown
Contributor Author

@cmaloney @gpshead @erlend-aasland the PR is approved and passing builds. can this be merged?

Comment thread Modules/_io/_iomodule.c Outdated

bedevere-app Bot commented Feb 17, 2025

Copy link
Copy Markdown

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.

morotti requested a review from gpshead February 18, 2025 18:08

morotti commented Feb 20, 2025
edited
Loading

Copy link
Copy Markdown
Contributor Author

@gpshead I've made the changes, would you be able to review again?

morotti commented Mar 6, 2025

Copy link
Copy Markdown
Contributor Author

hello @gpshead @cmaloney @serhiy-storchaka @eendebakpt would you be able to review and unblock the PR?

gpshead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

the change looks good, but this PR was created with the checkbox allowing committers to make direct edits to the PR branch disabled so we can't take care of trivia that may be blocking it ourselves.

can you merge master so that it reruns modern CI?

Comment thread Doc/library/functions.rst Outdated

morotti commented Mar 7, 2025

Copy link
Copy Markdown
Contributor Author

@gpshead merged with master.

sorry, I haven't found any checkbox to allow to make direct edits or I would have ticket. I suspect it's because the fork is in my work organization, maybe that comes with extra restrictions.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Performance or resource usage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants


Back | FazBrowse Home | New Git URL