| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@serhiy-storchaka hello, you were kind to review my first PR for buffering performance (118037), would you be able to review this PR too? |
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
Thank you for reviewing, The CLA check is green now and I added a news entry. |
Sorry, something went wrong.
|
@eendebakpt @benjaminp could you review? |
Sorry, something went wrong.
@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). |
Sorry, something went wrong.
|
Requesting Serhiy's review, since he reviewd the other linked (and merged) PR #118037. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
Please do not use rebase and force-push. It makes reviewing more difficult. |
Sorry, something went wrong.
|
alright, I've removed the constants _MAXIMUM_BUFFER_SIZE. build green. I think we're good to merge |
Sorry, something went wrong.
Please use git merge --no-ff main instead of rebasing. This has already been pointed out in earlier review. |
Sorry, something went wrong.
|
I've updated the branch to main, anything else you want? |
Sorry, something went wrong.
|
@cmaloney @gpshead @erlend-aasland the PR is approved and passing builds. can this be merged? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@gpshead I've made the changes, would you be able to review again? |
Sorry, something went wrong.
|
hello @gpshead @cmaloney @serhiy-storchaka @eendebakpt would you be able to review and unblock the PR? |
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
See discussion gh-117151
This patch adjusts the buffer size. That gives 3 to 5 times I/O performance improvement on modern hardware.