Opened 5 years ago
Last modified 15 months ago
#30949 assigned Cleanup/optimization
Use functools.cached_property instead of django.utils.functional.cached_property.
Reported by: | Thomas Grainger | Owned by: | David Smith |
---|---|---|---|
Component: | Utilities | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Adam Johnson | Triage Stage: | Someday/Maybe |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
functools gains cached_property in Python 3.8: https://bugs.python.org/issue21145
django.utils.functional.cached_property
should import it and deprecate its own implementation
Change History (14)
comment:1 by , 5 years ago
Description: | modified (diff) |
---|
follow-up: 10 comment:2 by , 5 years ago
comment:3 by , 5 years ago
I don't think we should do anything until Python 3.8 becomes the minimal Python supported by Django.
comment:4 by , 5 years ago
Component: | Uncategorized → Utilities |
---|---|
Summary: | use functools.cached_property over django.utils.functional.cached_property where available (py3.8+) → Use functools.cached_property instead of django.utils.functional.cached_property. |
Triage Stage: | Unreviewed → Someday/Maybe |
Type: | Uncategorized → Cleanup/optimization |
I agree with Claude, we can reconsider this ticket when Python 3.8 becomes the minimal Python supported by Django.
comment:5 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 4 years ago
We're nearing the point where Python 3.8 will become the minimum supported version. I've therefore started to look at this ticket as I think "someday" has now arrived, and hopefully we can make a decision one way or the other with this.
The first thing I've done is to look at the performance of the two functions and have put together a script here. Running it with isolated CPUs is giving repeatable results that show very little (if anything) between the two functions. This benchmark was run with Python 3.9.
There were previous comments about the impact of the RLock
mechanism, this is only used the first time when the value is set. I suspect this is why I'm seeing little performance difference between the two. (Hopefully, I've understood the context of these comments correctly)
If no one has any objections at this stage, I'll prepare a patch in due course.
python bench_cached_property.py ..................... Django Cache: Mean +- std dev: 1.07 us +- 0.05 us ..................... Python Cache: Mean +- std dev: 1.08 us +- 0.05 us
comment:8 by , 4 years ago
An issue has been opened against Python's cached_property
https://bugs.python.org/issue43468
Thanks to Antti Haapala for highlighting this.
comment:9 by , 4 years ago
I created a quick-and-dirty version of the benchmark with IO and multithreading to highlight the performance issues: https://gist.github.com/koirikivi/c58d30fce18ac1f0d65f06bfa4f93743
comment:10 by , 3 years ago
Replying to Simon Charette:
FWIW
functools.cached_property
adds aRLock
acquisition overhead for thread safety which is not something needed by most internal Django's usage and third party ones from my personal experience.
If we were to proceed with this patch we should measure the impact of this thread safety mechanism first and also document it.
Hi, I am trying to solve this issue, but is't it true that: if functools.cached_property
uses RLock
, then we don't need to worry about it, cause its python3 implementation and we can trust it?
comment:11 by , 3 years ago
then we don't need to worry about it, cause its python3 implementation and we can trust it?
It would be a performance regression. Especially see the above linked issue which implies massive degradation with multiple threads/processes.
comment:12 by , 3 years ago
I think development of a fixed cached_property has stalled? It looks like it might even be removed from CPython.
https://mobile.twitter.com/raymondh/status/1431016905276633088
I think this ticket should be wontfix, and there should be a docs in Django about why you MUST not use @functools.cached_property and always use the django one
comment:13 by , 2 years ago
One thing to note is that the @functools.cached_property
supports a use case that Django's doesn't. If you define a subclass with plain @property
, the caching doesn't happen because the @property
is a "data descriptor" so it gets precedence before the cached value. It's still not clear to me why super().count
also avoids the cached value in self.__dict__
but it clearly does.
In [8]: class A: ...: @django.utils.functional.cached_property ...: def count(self): ...: print('Computing... A.count') ...: return 15 ...: ...: ...: class B(A): ...: @property ...: def count(self): ...: print('B.count') ...: return super().count ...: ...: In [9]: b = B() In [10]: b.count B.count Computing... A.count <===== expected Out[10]: 15 In [11]: b.count B.count Computing... A.count <===== should not happen Out[11]: 15
The difference with functools
is that functools.cached_property
still explicitly looks into instance.__dict__
if it's called.
It's a corner case but I expected super().count
to be cached. It hit us when overriding Paginator.count
. The lack of caching led to four count queries. Small impact, easy workaround, but surprising behaviour.
comment:14 by , 15 months ago
The locking behavior for functool.cached_property was fixed in 3.12. See: https://github.com/python/cpython/issues/87634#issuecomment-1467140709
Someday can now be when minimum version is python 3.12 for django.
FWIW
functools.cached_property
adds aRLock
acquisition overhead for thread safety which is not something needed by most internal Django's usage and third party ones from my personal experience.If we were to proceed with this patch we should measure the impact of this thread safety mechanism first and also document it.