All commit authors signed the Contributor License Agreement.

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.

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.

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

@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

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?

Thank you for reviewing,

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

@eendebakpt @benjaminp could you review?

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

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

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

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

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

I think you were looking into the winconsoleio optimization?
you have a bug ticket here #121940
and commit here cmaloney@7adcb0d

I think we should keep it as a separate issue for a separate PR, or we'll never complete anything. ^^

I do have an old branch with comments, from when I was investigating too https://github.com/man-group/cpython/blob/io-buffer-size-archive/Modules/_io/winconsoleio.c#L51 (The comments and the branch are obsolete as some other people have redone some of the buffering already, the 10 year old code was counting characters wrong or something).

TL;DR Old versions of windows were buggy with large writes to the console, theoretically above 65k, practically with as little as 26k, so there is code in winconsole to split writes into smaller writes. This is a windows XP era bug. No longer an issue in windows 8 (oldest windows supported by the interpreter). I think we're in agreement that the old code can be removed. I think you have branches and PR to do that. :)

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

I think we're good to merge

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

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

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

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.

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

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

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