-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Skip middleware code when there are no user middlewares installed #2629
Conversation
Fixes performance issue introduced by #2577, boosting from 8K req/sec to almost 9K req/sec. If there are no middlewares installed by the user the attribute `request._match_info.current_app` already looks at to the right app, this is by design.
Test numbers
I've used this benchmark [1] [1] https://github.com/pfreixes/aiohttp_regression_performance_test |
Codecov Report
@@ Coverage Diff @@
## master #2629 +/- ##
==========================================
+ Coverage 97.92% 97.92% +<.01%
==========================================
Files 38 38
Lines 7261 7265 +4
Branches 1260 1261 +1
==========================================
+ Hits 7110 7114 +4
Misses 47 47
Partials 104 104
Continue to review full report at Codecov.
|
aiohttp/web_app.py
Outdated
|
||
for subapp in self._subapps: | ||
subapp.freeze() | ||
self._run_middlewares |= len(subapp._middlewares) > 0 |
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.
The line is cryptic a little.
Please replace with self._run_middlewares = self._run_middlewares or subapp._run_middlewares
The change respects middlewares from deep nested subapps: app -> subapp1 -> subapp2.
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.
Fair enough, done. Added also a bit of internal documentation about this flag and the root cause of its usage.
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.
Please merge when green
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs. |
What do these changes do?
Fixes performance issue introduced by #2577, boosting from 8K req/sec
to almost 9K req/sec.
If there are no middlewares installed by the user the attribute
request._match_info.current_app
already looks at to the right app,this is by design.
Are there changes in behavior for the user?
No