-
-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump FD_SETSIZE on Windows #87
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
@@ -11,6 +11,7 @@ source: | |||
- osx64-dist.patch # [osx and x86_64] | |||
- win-find_exe.patch # [win] | |||
- win-library_bin.patch # [win] | |||
- fd_setsize.patch # [win] | |||
|
|||
build: | |||
number: 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to bump the build number to get that change in.
*/ | ||
#if defined(MS_WINDOWS) && !defined(FD_SETSIZE) | ||
-#define FD_SETSIZE 512 | ||
+#define FD_SETSIZE 2048 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write a quick sentence to why this change is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why decide on number in particular? Why not twice or half as many?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like any arbitrary limit, it just feels a priori sufficient :-) Note there's a slight runtime cost as the fd array is runtime allocated; I think the cost is practically negligible but still.
By default:
>>> a, b= socket.socketpair()
>>> %timeit select.select([a], [a], [a])
The slowest run took 12.48 times longer than the fastest. This could mean that a
n intermediate result is being cached.
100000 loops, best of 3: 2.89 µs per loop
With this PR:
>>> %timeit select.select([a], [a], [a])
The slowest run took 24.39 times longer than the fastest. This could mean that a
n intermediate result is being cached.
100000 loops, best of 3: 3.52 µs per loop
@ocefpaf, thanks for the review comments. I think I've addressed them. |
I've rerendered in order to fix the CI bullds. |
I'm not apposed to this change but I do think some discussion is warranted. The patches that are currently applied are either cosmetic or are needed to allow python to operate properly in a conda environment. This patch is different, it changes the behavior of python in a manner that is not necessary for python to work in a conda environment (at least as far as I can tell). Additionally, this could create cases where a users script would work properly with the I think making "improvement" to packages can be a slippery slope, who gets to decide what defaults should be changed or what trade-offs should be made. Finally, has a similar change been suggested upstream, and if so, how was it received? |
@jjhelmus, the upstream issue reference is in the patch. |
Also issue raised for |
Thanks for pointing out the upstream issue @pitrou, missed that link when I first looked at this. I'm in favor of this change as this should keep this compatible with future versions of Python in the I think merging this change should wait until conda-forge moves to conda build 2.x to avoid creating a package missing the |
Adapted from work by pitrou in pull request conda-forge#87
Closing issue. Feel free to reopen if there is interest in applying this change to Python 2.7. |
No description provided.