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

andzn · 2019-06-05T12:41:38Z

https://bugs.python.org/issue28708

Issue: Low FD_SETSIZE limit on Windows #72894

asvetlov · 2019-06-05T13:34:08Z

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

jdemeyer

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

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

Modules/selectmodule.c

bedevere-bot · 2019-06-12T11:05:38Z

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 · 2019-06-14T08:01:25Z

I have made the requested changes; please review again

bedevere-bot · 2019-06-14T08:01:29Z

Thanks for making the requested changes!

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

andzn · 2019-06-14T08:02:10Z

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

NewUserHa · 2019-06-15T18:43:53Z

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

csabella · 2020-01-25T20:17:05Z

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

andzn · 2020-01-27T11:34:54Z

@csabella , merge conflicts are now resolved.

andzn · 2020-07-28T13:40:11Z

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

arhadthedev · 2023-02-17T10:30:01Z

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

andzn · 2023-02-17T20:08:34Z

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

@arhadthedev done.

vstinner · 2023-04-05T17:07:06Z

Modules/selectmodule.c

+    do {
+        if (((fd_set FAR *)(set))->fd_count < setsize)
+            ((fd_set FAR *)(set))->fd_array[((fd_set FAR *)(set))->fd_count++]=(fd);
+    } while(0);


What is the purpose of the surrounding code do { ... } while (0)?

vstinner · 2023-04-05T17:07:35Z

Modules/selectmodule.c

+    size_t i;
+    for (i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {


Suggested change

size_t i;

for (i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {

for (size_t i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {

vstinner · 2023-04-05T17:08:44Z

Modules/selectmodule.c

@@ -196,7 +211,7 @@ set2list(fd_set *set, pylist fd2obj[FD_SETSIZE + 1])
        return NULL;

    i = 0;


Can you declare i here?

vstinner · 2023-04-05T17:09:58Z

Modules/selectmodule.c

    struct timeval tv, *tvp;
    int imax, omax, emax, max;
    int n;
    _PyTime_t timeout, deadline = 0;

+    setsize = FD_SETSIZE;


I would prefer to initialize the variable where it's declared. Either declare it at this line, or move the declaration here. So the variable is never uninitialized.

vstinner · 2023-04-05T17:11:25Z

Modules/selectmodule.c

+        return NULL;
+    }
+
+    if ((size_t)rsize > setsize) setsize = (size_t)rsize;


You may write: setsize = Py_MAX(setsize, (size_t)rsize); (same for the 2 lines below). IMO it's more explicit (and how I would write it in Python).

vstinner · 2023-04-05T17:17:03Z

Modules/selectmodule.c

+    ofdset = (fd_set*) PyMem_NEW(SOCKET, setsize + 1);
+    efdset = (fd_set*) PyMem_NEW(SOCKET, setsize + 1);
+    if (ifdset->fd_array == NULL || ofdset->fd_array == NULL || efdset->fd_array == NULL) {
+        if (ifdset->fd_array) PyMem_DEL(ifdset->fd_array);


PyMem_DEL() is a alias to PyMem_Free(): use it directly.

The NULL test is useless and can be removed. PyMem_Free(NULL) is well defined, it does nothing: https://docs.python.org/dev/c-api/memory.html#c.PyMem_Free

vstinner · 2023-04-05T17:18:23Z

Modules/selectmodule.c

+    fd_set *ifdset, *ofdset, *efdset;
+#if !defined(MS_WINDOWS)
+    fd_set ifdset_stack, ofdset_stack, efdset_stack;
+#if !defined(SELECT_USES_HEAP)


It would make the code simpler if the SELECT_USES_HEAP macro would always be defined on Windows, no?

Raise FD_SETSIZE limit to 16384

b1ea111

the-knights-who-say-ni added the CLA signed label Jun 5, 2019

bedevere-bot added the awaiting review label Jun 5, 2019

andzn changed the title ~~bpo-28708: Raise FD_SETSIZE limit to 16384~~ bpo-28708: Raise FD_SETSIZE limit to 16384 on windows Jun 5, 2019

Dynamically allocate select fd_sets on Windows based on inputs list size

c2e6ef0

andzn changed the title ~~bpo-28708: Raise FD_SETSIZE limit to 16384 on windows~~ bpo-28708: Dynamically allocate select fd_sets on Windows based on inputs size Jun 10, 2019

andzn added 3 commits June 10, 2019 16:44

Move new FD_SET macro definition under defined(MS_WINDOWS)

c19f865

Added News file

185db02

Fix linux & doc build

555f0e1

jdemeyer requested changes Jun 12, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jun 12, 2019

jdemeyer requested changes Jun 12, 2019

View reviewed changes

asvetlov requested changes Jun 12, 2019

View reviewed changes

Modules/selectmodule.c Outdated Show resolved Hide resolved

Modules/selectmodule.c Outdated Show resolved Hide resolved

Modules/selectmodule.c Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting core review labels Jun 12, 2019

Add back SELECT_USES_HEAP

35885cb

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 14, 2019

vstinner mentioned this pull request Jun 14, 2019

[WIP] bpo-28708: Add setsize keywoard-only parameter to select.select() #13845

Closed

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

Merge branch 'master' into fix-issue-28708

e278594

dhirschfeld mentioned this pull request Jan 25, 2021

Choosing the Right ASGI Server for Deploying FastAPI fastapi/fastapi#2062

Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

Merge branch 'main' into fix-issue-28708

003b808

arhadthedev changed the title ~~bpo-28708: Dynamically allocate select fd_sets on Windows based on inputs size~~ gh-72894: Dynamically allocate select fd_sets on Windows based on inputs size Feb 17, 2023

arhadthedev added performance Performance or resource usage OS-windows extension-modules C modules in the Modules dir topic-IO labels Feb 17, 2023

vstinner reviewed Apr 5, 2023

View reviewed changes

python deleted a comment Apr 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

andzn commented Jun 5, 2019 •

edited by bedevere-bot

Loading

asvetlov commented Jun 5, 2019

jdemeyer left a comment

jdemeyer left a comment

bedevere-bot commented Jun 12, 2019

andzn commented Jun 14, 2019

bedevere-bot commented Jun 14, 2019

andzn commented Jun 14, 2019

NewUserHa commented Jun 15, 2019 •

edited

Loading

csabella commented Jan 25, 2020

andzn commented Jan 27, 2020

andzn commented Jul 28, 2020

arhadthedev commented Feb 17, 2023

andzn commented Feb 17, 2023

vstinner Apr 5, 2023

vstinner Apr 5, 2023

vstinner Apr 5, 2023

vstinner Apr 5, 2023

vstinner Apr 5, 2023

vstinner Apr 5, 2023

vstinner Apr 5, 2023

		size_t i;
		for (i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {

	size_t i;
	for (i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {
	for (size_t i = 0; i < setsize + 1 && fd2obj[i].sentinel >= 0; i++) {

		@@ -196,7 +211,7 @@ set2list(fd_set *set, pylist fd2obj[FD_SETSIZE + 1])
		return NULL;

		i = 0;

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

Are you sure you want to change the base?

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

Conversation

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

asvetlov commented Jun 5, 2019

jdemeyer left a comment

Choose a reason for hiding this comment

jdemeyer left a comment

Choose a reason for hiding this comment

bedevere-bot commented Jun 12, 2019

andzn commented Jun 14, 2019

bedevere-bot commented Jun 14, 2019

andzn commented Jun 14, 2019

NewUserHa commented Jun 15, 2019 • edited Loading

csabella commented Jan 25, 2020

andzn commented Jan 27, 2020

andzn commented Jul 28, 2020

arhadthedev commented Feb 17, 2023

andzn commented Feb 17, 2023

vstinner Apr 5, 2023

Choose a reason for hiding this comment

vstinner Apr 5, 2023

Choose a reason for hiding this comment

vstinner Apr 5, 2023

Choose a reason for hiding this comment

vstinner Apr 5, 2023

Choose a reason for hiding this comment

vstinner Apr 5, 2023

Choose a reason for hiding this comment

vstinner Apr 5, 2023

Choose a reason for hiding this comment

vstinner Apr 5, 2023

Choose a reason for hiding this comment

andzn commented Jun 5, 2019 •

edited by bedevere-bot

Loading

NewUserHa commented Jun 15, 2019 •

edited

Loading