Skip to content

Use mypyc to compile websocket reader - #13559

Draft
Dreamsorcerer wants to merge 19 commits into
masterfrom
mypyc
Draft

Dreamsorcerer wants to merge 19 commits into
masterfrom
mypyc

Conversation

@Dreamsorcerer

Copy link
Copy Markdown
Member

No description provided.

@codspeed-hq

codspeed-hq Bot commented Aug 27, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 21.8%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 2 regressed benchmarks
✅ 95 untouched benchmarks
⏩ 83 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
test_read_one_hundred_masked_large_binary_websocket_messages 60.2 ms 86.2 ms -30.24%
test_read_and_drain_one_hundred_websocket_text_messages 1.1 ms 1.3 ms -12.34%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing mypyc (876981a) with master (deab032)

Open in CodSpeed

Footnotes

  1. 83 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.

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

⚠️ JUnit XML file not found

The CLI was unable to find any JUnit XML files to upload.
For more help, visit our troubleshooting guide.

Comment thread aiohttp/_websocket/reader.py Outdated
# cannot be relied on anywhere. Cost: the reader and its buffers
# are reclaimed by full GC passes, not refcounting. Never break
# this cycle manually; remove when the mypyc issue is resolved.
self._gc_cycle = self

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is resulting in GBs of memory use, while removing it is resulting in numerous segfaults. Do not merge until this is fixed.

Comment thread aiohttp/_websocket/reader.py Outdated
@webknjaz

webknjaz commented Sep 1, 2026

Copy link
Copy Markdown
Member

Does mypyc also have a coveragepy plugin like Cython?

@Dreamsorcerer

Copy link
Copy Markdown
Member Author

Does mypyc also have a coveragepy plugin like Cython?

I don't think so, but it's literally the same code now, as it's just compiling the Python code. So we already have coverage from that.

@Dreamsorcerer

Copy link
Copy Markdown
Member Author

If we get to fully converting to mypyc, then I think the only part we'd need to worry about coverage is on the main HTTP parser, as that'd be separate code that uses llhttp.

@webknjaz

webknjaz commented Sep 1, 2026

Copy link
Copy Markdown
Member

I'm not so sure — once that code's in a C-extension, it'll start getting optimized on the compiler level and there will be fewer guarantees that it's getting executed. So we'll need to make sure to execute exactly the same tests against both implementations at all times (otherwise, the metrics would be unreliable).

@Dreamsorcerer

Copy link
Copy Markdown
Member Author

I'm not so sure — once that code's in a C-extension, it'll start getting optimized on the compiler level and there will be fewer guarantees that it's getting executed.

I'm not sure I follow. The compiler would optimise out parts of C-level code, it'd not give us any real change at the Python level as that'd just be instrumented calls which would always be called.

So we'll need to make sure to execute exactly the same tests against both implementations at all times (otherwise, the metrics would be unreliable).

We already do this. Again, the only discrepancies we need to worry about is the llhttp vs pure Python HTTP parser. But, before we could even consider that, mypyc needs to do native optimisations of ctypes calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants