Skip to content
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

Performance improvement on the TCP connector with async DNS resolver #1260

Closed
clemlesne opened this issue Jan 15, 2025 · 10 comments
Closed

Performance improvement on the TCP connector with async DNS resolver #1260

clemlesne opened this issue Jan 15, 2025 · 10 comments
Labels
enhancement New feature or request

Comments

@clemlesne
Copy link

clemlesne commented Jan 15, 2025

Opportunity

AIOHTTP TCP connector performance could be improved by using AIODNS. AIOHTTP maintainers explicitly recommends it. It technically uses c-ares as low-level resolver implementation.

Implementation

It should be located near that line:

return aiohttp.TCPConnector(
limit=self._max_pool_connections,
ssl=ssl_context or False,
**self._connector_args,
)

Straightforward to implement at the first view.

Impacts

Windows compatibility

Context:

  • asyncio loop requires being specifically configured on Windows with a non-default configuration
  • I personally encountered frequent issues on Windows, especially weird behaviors with AV and network stack
  • Btw, it works like a charm on macOS and Linux

Mitigation:

  • Disable it entirely on Windows?
  • Enable it as a feature flag

Performance

We should monitor performance as of automatic performance testing, to ensure this kind of change does not have unwanted effects.

Related

The current implementation limits parallel connections to 10, accordingly to the default setting in botocore:

https://github.com/boto/botocore/blob/26a4f341d5496e12696692904d1dc83b939b71b5/botocore/httpsession.py#L81

This is relatively low and certainly impacts performance. AIOHTTP maintainers recommend 100, again, it should be interesting to benchmark that.

@clemlesne clemlesne changed the title Performance improvement on the TCP connector with async DNS resolved Performance improvement on the TCP connector with async DNS resolver Jan 15, 2025
@dacevedo12
Copy link

is aiodns something that needs to be explicitly configured by aiobotocore or it enables by default as long as it's installed?

btw, should we be concerned about aio-libs/aiodns#122

@jakob-keller
Copy link
Collaborator

I am not familiar with aiodns, but it appears to be tied to SelectorEventLoop. That would rule out alternative event loop implementations, such as uvloop. Therefore, any support for aiodns must be implemented either as strictly opt-in or be conditional on the event loop used at runtime. Is this even worth the effort?

@thehesiod
Copy link
Collaborator

AFK but according to docs you just need to install aiohttp with speedups and it should get used automatically https://docs.aiohttp.org/en/stable/

@clemlesne
Copy link
Author

AFK but according to docs you just need to install aiohttp with speedups and it should get used automatically https://docs.aiohttp.org/en/stable/

It is not, see aio-libs/aiohttp#2228.

Something like this needs to be done:

session = ClientSession(
    connector=TCPConnector(resolver=AsyncResolver()),
)

@clemlesne
Copy link
Author

I am not familiar with aiodns, but it appears to be tied to SelectorEventLoop. That would rule out alternative event loop implementations, such as uvloop. Therefore, any support for aiodns must be implemented either as strictly opt-in or be conditional on the event loop used at runtime. Is this even worth the effort?

This could be an opt-in in the create_client func (e.g enable_async_dns: bool = False), with a strong recommendation written in the documentation. Like that, we don't implement a breaking change for those using other event loops implementations.

@thehesiod
Copy link
Collaborator

the way to handle this is adding an arg to AioClientConfig

@thehesiod
Copy link
Collaborator

open to PRs! otherwise we'll get to this soon, good idea. btw I remember trying to use c-ares via aiodns several years ago while it was still new however it had some rather unfortunate issues so had to roll back. I'm sure it's more stable by now.

@thehesiod thehesiod added the enhancement New feature or request label Jan 15, 2025
@thehesiod
Copy link
Collaborator

so I looked at this more, we don't need to do anything.

  1. aiodns support is dynamically checked in aiohttp here: cca57fc883c86826b126d3/aiohttp/resolver.py#L13
  2. which is then plugged into DefaultResolver here: https://github.com/aio-libs/aiohttp/blob/216e082807d02fafaacca57fc883c86826b126d3/aiohttp/resolver.py#L146
  3. which is then used by the TCPConnector here: https://github.com/aio-libs/aiohttp/blob/701ad1e4ffff8c2b95d3c7a870e6ef892680aa9d/aiohttp/connector.py#L860-L861

given we don't override the resolver assuming your system supports aiodns it will get used. Will close this task unless you can point out an error in the above logic

@thehesiod
Copy link
Collaborator

also note you can easily override the max parallel connections via AioConfig/Config. We follow the botocore defaults for now as I don't see a good reason to change it. It's kinda configured for hobby use, and up to user to "productionize" settings :)

@thehesiod
Copy link
Collaborator

oops, forgot to close :)

@jakob-keller jakob-keller closed this as not planned Won't fix, can't repro, duplicate, stale Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants