Add aiofastnet dependency to speedups extra (faster TLS and native kernel TLS support)#12744
Add aiofastnet dependency to speedups extra (faster TLS and native kernel TLS support)#12744tarasko wants to merge 43 commits into
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12744 +/- ##
==========================================
- Coverage 98.92% 98.91% -0.01%
==========================================
Files 131 132 +1
Lines 46881 46916 +35
Branches 2431 2434 +3
==========================================
+ Hits 46376 46408 +32
- Misses 379 381 +2
- Partials 126 127 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
for more information, see https://pre-commit.ci
|
FTR, we should be conservative about adding deps that have only existed for just under 3 months and aren't under our control. This poses a supply chain risk. Something to concider could be running an audit or offering hosting it under the org. |
Merging this PR will improve performance by 18.88%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_one_thousand_round_trip_websocket_binary_messages[small] |
16.3 ms | 13.2 ms | +24.06% |
| ⚡ | test_one_thousand_round_trip_websocket_text_messages |
16.8 ms | 13.6 ms | +23.24% |
| ⚡ | test_simple_web_file_response |
82 ms | 71.5 ms | +14.7% |
| ⚡ | test_simple_web_file_sendfile_fallback_response |
88.1 ms | 71.5 ms | +23.11% |
| ⚡ | test_ten_streamed_responses_iter_any |
21.7 ms | 19.8 ms | +9.98% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing tarasko:feature/aiofastnet_extra (133193d) with master (e8832ae)2
Footnotes
-
72 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
master(ec3f080) during the generation of this report, so e8832ae was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
…importing them locally
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
2 checks are failing:
Everything else pass |
I've already done an AI audit and shared any results with tarasko. tarasko also has some prior involvement with aiohttp in the past in relation to websockets optimisation, which is why I've encouraged this. If this library proves out to be beneficial then we'll open an invitation to host it under aio-libs, if tarasko is interested. |
|
I'm going to skip this for the 3.14 release and we can look at including it into 3.15/4. |
Yes, I'm interested. I think it would be great for aiofastnet credibility if you host it under aio-libs. Since aiofastnet replacing such a fundamental layer, it comes with a big security concern. I'm happy to discuss what can be done to mitigate any security risks. |
The audit results are in a private vulnerability report, if you've not already seen it: |
|
Codspeed reports around 20% performance gains on some benchmarks. I noticed these benchmarks run for plain TCP connections only. aiofastnest helps to optimize TCP case of course, but that is not where aiofastnet truly shines. I would expect much more significant gains for TLS connections, my projection is around 50-70%. Perhaps we could add TLS case to benchmarks as a separate PR, so there will be a good baseline. |
Thank you very much. I'm looking into it |
Yes, that'd be useful to add. |
There was a problem hiding this comment.
I'll do a full review later, but given the limited number of places it's used, maybe it's fine to just handle this in each file individually? Problem here is a complete loss of type safety and would likely need a lot more code to get that working sensibly.
There was a problem hiding this comment.
I can make typed wrappers for create_connection and start_tls in connector.py (and similar for other files)
Something like this:
connector.py
async def create_connection(loop: AbstractEventLoop, <actual typed arguments used by aiohttp>) -> tuple[Transport, Protocol]
if HAS_AIOFASTNET:
return await aiofastnet.create_connection(loop, <actual typed arguments used by aiohttp>)
else:
return await loop.create_connection(<actual typed arguments used by aiohttp>)This way it can still be easily mocked in tests. But the following will probably have to be repeated in multiple files in some way:
if NO_EXTENSIONS:
HAS_AIOFASTNET = False
else:
try:
import aiofastnet
HAS_AIOFASTNET = True
except ImportError:
HAS_AIOFASTNET = False
or I can keep net_helpers.py and make typed wrappers there. Seems a little cleaner for my taste, but no strong preference
There was a problem hiding this comment.
Yeah, that should be good.
This way it can still be easily mocked in tests. But the following will probably have to be repeated in multiple files in some way:
Yeah, we already do this for other libraries, so probably fine.
There was a problem hiding this comment.
if NO_EXTENSIONS:
Wait, do we want to disable it on NO_EXTENSIONS? That flag is just meant to disable the compiled Cython code in aiohttp.
There was a problem hiding this comment.
Ah, indeed. For some reason I thought that it is supposed to disable extensions and extras.
I'll stop using it.
How do you usually test aiohttp locally with and without a particular extra? Just by having 2 environments with and without a package?
There was a problem hiding this comment.
I guess. Currently the CI probably catches this simply by these libraries not being available on some platforms. We could probably do with a separate extras requirements file or something and not install it in the NO_EXTENSIONS test runs.
|
I messed something up with this branch and got swarmed with endless resolve conflicts. |
What do these changes do?
Discussion #12701
This change adds
aiofastnettospeedupsextras.aiofastnetprovides more efficient implementations foraiohttpwill use them ifaiofastnetcan be imported.On top of that,
aiofastnetprovides optional, transparent Kernel TLS support on Linux and native sendfile for TLS connections using SSL_sendfile. User can enable it via an option in SSLContext.Are there changes in behavior for the user?
This is an optimization change, it should not change visible behavior.
Is it a substantial burden for the maintainers to support this?
There is now an extra indirection layer in net_helpers.py, tests should prefer to mock its functions instead of mocking loop primitives like create_connection directly.
Every time when users will complain about something that sounds like a networking layer issue, it would be very important to know: is aiofastnet installed or not? So it would be good to add this question to the issue template.
aiofastnetitself is a medium size project, functionally-wise it has a limited scope, and I hope I'll be able to maintain it the next 5 years. It is relatively new, I have written a good amount of tests already and I'm currently adding code coverage reports. I want to get coverage close to 100%.Related issue number
Checklist
CONTRIBUTORS.txt