[Django] #30949: use functools.cached_property over django.utils.functional.cached_property where available (py3.8+)
Django
where available (py3.8+)
-------------------------------------------+------------------------
Reporter: Thomas Grainger | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/30949>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
where available (py3.8+)
Reporter: Thomas Grainger | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Adam (Chainz) Johnson:
Old description:
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:1>
Django
where available (py3.8+)
Reporter: Thomas Grainger | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
FWIW `functools.cached_property` adds a `RLock` 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:2>
Django
where available (py3.8+)
Reporter: Thomas Grainger | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
I don't think we should do anything until Python 3.8 becomes the minimal
Python supported by Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:3>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: nobody
Cleanup/optimization |
Component: Utilities | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* component: Uncategorized => Utilities
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Someday/Maybe
Comment:
I agree with Claude, we can reconsider this ticket when Python 3.8 becomes
the minimal Python supported by Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:4>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Type: | Smith
Cleanup/optimization | Status: assigned
Component: Utilities | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Smith):
* owner: nobody => David Smith
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:5>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: Utilities | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David Smith):
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
[https://gist.github.com/smithdc1/873688e563c66a0dd253602aab6fc3e2 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
[https://github.com/python/cpython/blob/63298930fb531ba2bb4f23bc3b915dbf1e17e9e1/Lib/functools.py#L964
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:6>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: Utilities | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam Johnson):
* cc: Adam Johnson (added)
Comment:
LGTM, thanks for the benchmarking David
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:7>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David Smith):
An issue has been opened against Python's `cached_property`
https://bugs.python.org/issue43468
Thanks to Antti Haapala for highlighting this.
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:8>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Rainer Koirikivi):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:9>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mateusz Leszko):
Replying to [comment:2 Simon Charette]:
> FWIW `functools.cached_property` adds a `RLock` 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:10>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam Johnson):
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:11>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Thomas Grainger):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:12>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Filip Sedlák):
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.
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:13>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Akshet Pandey):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:14>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mike Edmunds):
* stage: Someday/Maybe => Unreviewed
Comment:
The minimum version for Django 6.0 is Python 3.12. Someday can be now.
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:15>
Django
django.utils.functional.cached_property.
-------------------------------------+-------------------------------------
Reporter: Thomas Grainger | Owner: David
Type: | Smith
Component: Utilities | Version: dev
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Smith):
* status: assigned => closed
Comment:
The cpython implementation is not identical to what django has. Django's
version is battle tested and has stood the test of time. I also don't
think we would gain that much from lower maintenance of using a cpython
version.
Given that I don't think it's worth changing. Especially since there is a
price to pay for the change with (many?) projects being impacted and
needing to change their imports.
--
Ticket URL: <https://code.djangoproject.com/ticket/30949#comment:16>