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

Refs #34118 -- Avoided repeat coroutine checks in MiddlewareMixin. #17402

Merged
merged 1 commit into from Oct 23, 2023

Conversation

adamchainz
Copy link
Sponsor Member

@adamchainz adamchainz commented Oct 22, 2023

32d70b2 changed MiddlewareMixin to call iscoroutinefunction on every request. Whilst fast, it’s not free, with many function calls and unwrapping in its tree: https://github.com/python/cpython/blob/88bac5d5044e577825db1f9367af908dc9a3ad82/Lib/inspect.py#L420 . Multiplying this by many middlewares it will add up.

This PR changes to cache the result in an instance variable, as it did before.

Quick benchmark shows iscoroutinefunction can take ~0.5us on a function with one decorator:

Python 3.12.0 (main, Oct  5 2023, 12:02:11) [Clang 15.0.0 (clang-1500.0.40.1)]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.16.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import inspect

In [2]: def f(): pass

In [3]: %timeit inspect.iscoroutinefunction(f)
358 ns ± 1.62 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [4]: from functools import cache

In [5]: @cache
   ...: def g(): pass

In [6]: %timeit inspect.iscoroutinefunction(g)
544 ns ± 1.22 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Yes, this seems totally reasonable to me.

(The asgiref.sync utilities include a check that they're correctly passed a coroutine marked class, so an error here would show in the test output, I trust.)

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@adamchainz Thanks 👍

@felixxm felixxm merged commit e2922b0 into django:main Oct 23, 2023
32 checks passed
@adamchainz adamchainz deleted the avoid_multiple_async_check branch October 23, 2023 12:14
@adamchainz
Copy link
Sponsor Member Author

Thanks! Look after the microseconds and the milliseconds will look after themselves 😉

@adamchainz
Copy link
Sponsor Member Author

@felixxm backport so 5.0 doesn't have the performance regression?

@felixxm
Copy link
Member

felixxm commented Oct 26, 2023

@felixxm backport so 5.0 doesn't have the performance regression?

I categorized this as a cleanup. If we want to treat this as a bug it should be a release blocker for Django 4.2, so a separate ticket and a release note in 4.2.7.txt would be needed.

@adamchainz
Copy link
Sponsor Member Author

Oh sorry, I didn't realize that commit was in 4.2, I thought it was only on the 5.0 branch.

Eh, I can't be bothered filing a ticket if no one else has spotted this.

@carltongibson
Copy link
Member

(I don't think it's release blocker status...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants