Skip to content
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

Pass the request ctx rather than use the globals in the app #5229

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pgjones
Copy link
Member

@pgjones pgjones commented Aug 20, 2023

The globals have a performance penalty which can be justified for the convinience in user code. In the app however the ctx can easily be passed through the method calls thereby reducing the performance penalty.

This may affect extensions if they have subclassed the app and overridden these methods.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@pgjones
Copy link
Member Author

pgjones commented Aug 20, 2023

I've done something similar for Quart after finding it had a significant impact on micro benchmarks.

Edit: The quart study was done a few years ago before the switch to ContextVars which seem significantly quicker. I still think this is a useful change, but seems less performance motivated now.

This was added as per 0ec7f71 by
mistake.
The globals have a performance penalty which can be justified for the
convinience in user code. In the app however the ctx can easily be
passed through the method calls thereby reducing the performance
penalty.

This may affect extensions if they have subclassed the app and
overridden these methods.
@pgjones pgjones marked this pull request as ready for review August 27, 2023 09:20
@davidism
Copy link
Member

My initial reaction is that while this makes sense, I'm uncomfortable with making the change. I'll have to think about this for a little bit.

@davidism
Copy link
Member

Could you give more information about the benchmarks? How micro are we talking; how fast does the complexity of the app overshadow any speed gains?

Is the speed loss in the use of the proxy, or is the gain in the use of the local variable? Perhaps using _cv_req.get() directly and avoiding the proxy could have a similar improvement?

@pgjones
Copy link
Member Author

pgjones commented Aug 28, 2023

This is an interesting one, with Quart the equivalent to this makes around a 1.5 times improvement, whereas for Flask it doesn't seem to have any affect (maybe a minor positive one). I don't think I can therefore remove it from Quart, but I'd like the APIs to match.

(I tried _cv_request in Quart and it made little difference.)

Also I don't think this makes it more complex - without knowledge of Flask's globals it would be expected to be passed via the call chain.

@pgjones
Copy link
Member Author

pgjones commented Sep 16, 2023

This will be deferred to allow further discussion.

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