-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
use AI_NUMERICHOST | AI_NUMERICSERV to skip getaddrinfo thread in asyncio #90980
Comments
now that the getaddrinfo lock has been removed on all platforms the numeric only host resolve in asyncio could be moved back into BaseEventLoop.getaddrinfo |
Could you provide more context for the proposed change? |
hello, it's actually a bit of a round about context, but it was brought up on a tornado issue where I was attempting to port the asyncio optimization to tornado: tornadoweb/tornado#3113 (comment) I think it would be better to use this AI_NUMERICHOST | AI_NUMERICSERV optimization from trio everywhere instead |
To summarize the justification, this patch does two things: it moves an optimization from create_connection to getaddrinfo, which makes it apply to more callers (including Tornado), and it makes the code simpler and less redundant (net reduction of 47 non-test lines in the patch). As far as we can tell, the reason it wasn't done this way in the first place is that at the time getaddrinfo held a global lock on some platforms, but this is no longer true. If there's still some locking in or around getaddrinfo on some platforms (or some libc implementations), this patch would be a bad idea. Is there a good way to test for that? I suppose we could set up a deliberately-slow DNS server and try to call getaddrinfo with AI_NUMERICHOST while another thread is blocked talking to that server, but that seems like a lot of test infrastructure to build out. |
On MacOS in 2015, getaddrinfo was found to be much slower than inet_pton. Unless that's changed, this patch would be a performance regression on that platform. Data and benchmark script in https://groups.google.com/g/python-tulip/c/-SFI8kkQEj4/m/m1-oCMSABgAJ |
running the benchmark script https://gist.github.com/graingert/5097c6be997ab2a201b48b858524e163 on my Linux machine I get before:
after:
|
on macos: before https://github.com/graingert/cpython/runs/6528334512?check_suite_focus=true#step:7:1
after https://github.com/graingert/cpython/runs/6528182360?check_suite_focus=true#step:7:11
|
benchmarking https://github.com/graingert/cpython/runs/6528895181?check_suite_focus=true
|
Next action: Review the existing PR and determine next steps. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: