◐ Shell
reader mode source ↗
Skip to content

gh-72894: Dynamically allocate select fd_sets on Windows based on inputs size#13842

Open
andzn wants to merge 8 commits into
python:mainfrom
andzn:fix-issue-28708
Open

gh-72894: Dynamically allocate select fd_sets on Windows based on inputs size#13842
andzn wants to merge 8 commits into
python:mainfrom
andzn:fix-issue-28708

Conversation

@andzn

@andzn andzn commented Jun 5, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown

@andzn andzn changed the title bpo-28708: Raise FD_SETSIZE limit to 16384 Jun 5, 2019
@asvetlov

asvetlov commented Jun 5, 2019

Copy link
Copy Markdown
Contributor

By this PR you basically increased the memory consumption for every select call from 12 KiB to 384 KiB.
I believe the number is too high.

See the issue's comment added recently

@andzn andzn changed the title bpo-28708: Raise FD_SETSIZE limit to 16384 on windows Jun 10, 2019
jdemeyer
jdemeyer requested changes Jun 12, 2019
View reviewed changes

@jdemeyer jdemeyer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

To test the new seqsize() code path, I would like to see a test added where select.select() is called with an iterable which is not a tuple or list, for example iter([fd]) instead of [fd].

jdemeyer
jdemeyer requested changes Jun 12, 2019
View reviewed changes

@jdemeyer jdemeyer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

You are also missing error checks where you call seqsize().

@bedevere-bot

bedevere-bot commented Jun 12, 2019

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.

And if you don't make the requested changes, you will be put in the comfy chair!

@andzn

andzn commented Jun 14, 2019

Copy link
Copy Markdown
Author

I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented Jun 14, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

@andzn

andzn commented Jun 14, 2019

Copy link
Copy Markdown
Author

@jdmeyer I removed the seqsize function completely and replaced calls with PySequence_Length.

@vstinner vstinner mentioned this pull request Jun 14, 2019
Closed
@NewUserHa

NewUserHa commented Jun 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

hope this pass soon.
select() limit really is pain ass.

@csabella csabella requested review from asvetlov and jdemeyer January 25, 2020 20:16
@csabella

csabella commented Jan 25, 2020

Copy link
Copy Markdown
Contributor

@andzn, please resolve the merge conflict. Thank you!

@andzn

andzn commented Jan 27, 2020

Copy link
Copy Markdown
Author

@csabella , merge conflicts are now resolved.

@andzn

andzn commented Jul 28, 2020

Copy link
Copy Markdown
Author

@asvetlov, @jdemeyer can you please take a look again at the PR? The requested changes have already been implemented a year ago.

@dhirschfeld dhirschfeld mentioned this pull request Jan 25, 2021
Closed
@arhadthedev arhadthedev changed the title bpo-28708: Dynamically allocate select fd_sets on Windows based on inputs size Feb 17, 2023
@arhadthedev arhadthedev added performance OS-windows C modules in the Modules dir labels Feb 17, 2023
@arhadthedev

arhadthedev commented Feb 17, 2023

Copy link
Copy Markdown
Member

@andzn Could you sign the new CLA by clicking not signed button in the cpython-cla-bot's message, please?

@andzn

andzn commented Feb 17, 2023

Copy link
Copy Markdown
Author

@andzn Could you sign the new CLA by clicking not signed button in the cpython-cla-bot's message, please?

@arhadthedev done.

@python python deleted a comment Apr 7, 2025
@github-actions

github-actions Bot commented Mar 4, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments

@jdemeyer jdemeyer Awaiting requested review from jdemeyer

@asvetlov asvetlov Awaiting requested review from asvetlov

Assignees

No one assigned

Labels

awaiting change review extension-modules C modules in the Modules dir OS-windows performance stale Stale PR or inactive for long period of time. topic-IO

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@andzn @asvetlov @bedevere-bot @NewUserHa @csabella @arhadthedev @vstinner @jdemeyer @the-knights-who-say-ni @ezio-melotti