-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Refactor FlowControlDataQueue
to improve performances
#9659
Conversation
CodSpeed Performance ReportMerging #9659 will improve performances by 12.01%Comparing Summary
Benchmarks breakdown
|
why is that super call so expensive..... |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #9659 +/- ##
=======================================
Coverage 98.64% 98.65%
=======================================
Files 116 116
Lines 35475 35482 +7
Branches 4208 4211 +3
=======================================
+ Hits 34996 35003 +7
Misses 321 321
Partials 158 158
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The finally block is not needed because `self._limit` is never decreased if read raises
8d76764
to
e14531b
Compare
FlowControlDataQueue.read
to improve performances
FlowControlDataQueue.read
to improve performancesFlowControlDataQueue
to improve performances
Backport to 3.11: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 1bb146a on top of patchback/backports/3.11/1bb146ad36b1660548d23166be2e7b37579a1001/pr-9659 Backporting merged PR #9659 into master
🤖 @patchback |
(cherry picked from commit 1bb146a)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As DataQueue doesn't have _size anymore, the Generic should not require Sized anymore. Also, probably want to have WSMessage queues move to that, as they don't need the _size calculations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_SizedT
is in this case WSMessage
.... I think could be changed to _T
FlowControlDataQueue
is only used by the WebSocket? Does the WebSocket not need to pause the protocol when the limit is hit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah. That logic can simply be removed then. I thought I remembered it being used in the parser or something with bytes, where it tracked the size of the data stream..
The
DataQueue
base class trackssize
but it never uses it. Move all thesize
tracking into theFlowControlDataQueue