Reduce WebSocket frame parser complexity #8682
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8682 +/- ##
==========================================
- Coverage 97.96% 97.96% -0.01%
==========================================
Files 107 107
Lines 33860 33858 -2
Branches 3970 3970
==========================================
- Hits 33170 33168 -2
Misses 513 513
Partials 177 177
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
running the profile shows the code is only 2-3% faster. Thats within the margin of error. The code is a lot easier to read now though. I think we will have to cythonize like #2837 suggests to get more out of it |
|
|
I'd really like to try throwing such code through mypyc first and seeing how that performs. |
|
Have you had good luck with mypyc in the past? Its been a while since I tried it and wasn't able to ever use it in production because of stability issues. |
Think it's only been considered stable for a couple of years, and was missing AsyncIterator support until around then. mypy itself seems to be stable, and I've used it for a few scripts, and in https://github.com/ruundii/bthidhub without further issues. |
|
Since the performance improvement is within the margin of error, I recharacterized this to be a code complexity reduction. |
|
I'd probably skip the changelog myself, seems like no user-visible changes. |
In short, even a couple of years ago, I didn't experience any stability issues personally, but didn't try it out on larger projects. The only issues were related to specific features, which were predictable. Things like AsyncIterator being missing resulted in some files not being able to compile, and there was atleast one case where the compiled code wouldn't work for some reason, along the lines of an inspection of a function needed to return that it was a function, but the compiled version is not considered a function by Python's inspect module, so was incompatible with the library code I was using. |
|
Thanks for the additional color. I'll also ask for some recent feedback from Home Assistant devs to see if anyone has used it in a larger project. |
|
Marc pointed out that black is using There are probably some gotchas we might hit based on that post, but its still probably easier than re-writing it in cython, although cython might be a bit faster, and aiohttp already uses cython. I'm on the fence on which direction to go with this. |
|
Yeah, I'd prefer to try mypyc first, as it's easier to maintain a Python file, rather than Cython + Python. |
|
Also, I think the main gotchas are to do with interacting with the object, e.g. mocking, inspecting etc. The code for parsers should be fairly private, so we shouldn't expect much user code to be interacting with it. |
|
It looks like it doesn't like the enums |
|
Well, that's a disappointing start. If you can isolate a single part that causes the error, then I'd file a bug at https://github.com/mypyc/mypyc/issues Maybe for immediate testing to get an idea of performance, we can just work around it (e.g. replace enum with ints). |
|
It looks like there is already an issue for the problem with IntEnum mypyc/mypyc#1022 |
|
Found one more back in 2020 mypyc/mypyc#721 It doesn't look like it will get solved any time soon. |
|
|
|
Yeah, mypyc/cython is a tangent, not related to this PR. |
Backport to 3.10: 💚 backport PR created✅ Backport PR branch: Backported as #8727 🤖 @patchback |
(cherry picked from commit 490fca6)
Backport to 3.11: 💚 backport PR created✅ Backport PR branch: Backported as #8728 🤖 @patchback |
(cherry picked from commit 490fca6)
…lexity (#8727) Co-authored-by: J. Nick Koston <nick@koston.org>
…lexity (#8728) Co-authored-by: J. Nick Koston <nick@koston.org>
This one was just closed, so might be worth another try when the next mypy release is out.. |
|
I'm relatively happy where we landed with this one as we don't have to maintain a separate pyx file for the WebSocketReader as the pxd turned out to be enough. Would be nice to give mypyc another shot for the next project that comes up that could benefit from it. |
What do these changes do?
Small improvement to reduce the WebSocket frame parser complexity
Are there changes in behavior for the user?
no
Is it a substantial burden for the maintainers to support this?
code is a bit easier to follow/maintain.