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

Improve sync.py typing and utilities performance #390

Merged
merged 4 commits into from
May 18, 2023

Conversation

bellini666
Copy link
Contributor

  • Make sure sync_to_async/async_to_sync keeps the original function typing signature

  • Define iscoroutinefunction/markcoroutinefunction at import time instead of an if/else in runtime.

The current implementation:

In [7]: timeit.timeit(lambda: iscoroutinefunction(some_sync_func), number=10000)
Out[7]: 0.012024379917420447

Versus this PR

In [11]: timeit.timeit(lambda: iscoroutinefunction(some_sync_func), number=10000)
Out[11]: 0.004663120023906231

This is a performance improvement of almost 300%.

* Make sure `sync_to_async`/`async_to_sync` keeps the original function
  typing signature

* Define iscoroutinefunction/markcoroutinefunction at import time instead
  of an if/else in runtime. This improves the performance of those
  function calls by almost 300%
Copy link
Member

@andrewgodwin andrewgodwin left a comment

Choose a reason for hiding this comment

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

Code changes look reasonable, but you will need to please the mypy in the CI to get this in!

@bellini666
Copy link
Contributor Author

@andrewgodwin thanks for the review. Just solved all the issues to please mypy :)

@andrewgodwin
Copy link
Member

Unfortunately not for 3.9 and below, it looks like - the problem with ParamSpec is it's relatively new, and you'll need a way of keeping things backward-compatible somehow.

I don't care much about 3.7 as it's about to be EOL, but 3.8 and 3.9 do still matter.

(Apologies I can't tell our CI to run for you on every commit!)

@bellini666
Copy link
Contributor Author

Unfortunately not for 3.9 and below, it looks like - the problem with ParamSpec is it's relatively new, and you'll need a way of keeping things backward-compatible somehow.

I don't care much about 3.7 as it's about to be EOL, but 3.8 and 3.9 do still matter.

Oh, sorry about that. The code at sync.py was already conditionally importing from typing only for 3.10+, but I forgot to do the same when fixing typing on current_thread_executer.py

I only have python 3.11 in my local dev environment, so I'll only know for older versions once the CI gets to run

(Apologies I can't tell our CI to run for you on every commit!)

Np :)

Hopefully everything should be fine now

@andrewgodwin
Copy link
Member

Still something awry it looks like. You should be able to run the full set of python versions tests locally with tox if you'd like.

@bellini666
Copy link
Contributor Author

Still something awry it looks like. You should be able to run the full set of python versions tests locally with tox if you'd like.

@andrewgodwin I was being lazy to install all required python versions =P. Just did that, ran tox and was able to really fix everything now.

Sorry about that!

@andrewgodwin andrewgodwin merged commit 72f45c5 into django:main May 18, 2023
6 checks passed
@andrewgodwin
Copy link
Member

Yup, all looks good now! Thanks, I appreciate you taking the time to go through all those revisions!

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.

None yet

2 participants