Use functools.cached_property instead of django.utils.functional.cached_property.

[Thomas Grainger]

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

[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.

[Claude Paroz]

I don't think we should do anything until Python 3.8 becomes the minimal Python supported by Django.

[Mariusz Felisiak]

I agree with Claude, we can reconsider this ticket when Python 3.8 becomes the minimal Python supported by Django.

[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 ​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

[Adam Johnson]

LGTM, thanks for the benchmarking David

[David Smith]

An issue has been opened against Python's cached_property ​https://bugs.python.org/issue43468

Thanks to Antti Haapala for highlighting this.

[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

[Mateusz Leszko]

Replying to 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?

[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.

[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

[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.

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.

[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.

[Mike Edmunds]

The minimum version for Django 6.0 is Python 3.12. Someday can be now.

[David Smith]

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.

[Jacob Rief]

For reasons of code cleaness and to avoid confusion, we should rethink that decision.
Today, I ran the Django unit tests with Python's builtin cached_property decorator, and except test_cached_property_set_name_not_called expecting a slighlty different error message, it showed no issues.

Also consider Adam Johnson comment on the discussion forum:

Python’s implementation contains a few more guards around weird class edge cases, like no __dict__ or an unwriteable __dict__. It also provides a typing hook in __class_getitem__. Other than those things, it’s basically the same. It seems Python even copied the error message wording from Django.
I would say let’s migrate, which in practice means deprecating our version while moving all internal usage to Python’s version. Django won here and got one of its functions merged upstream (albeit with some drama around adding then removing locking), so let’s use it.

The builtin functools.cached_property also does not acquire a RLockanymore. So there is no overhead for thread safety as mentioned in comment 2 by Simon Charette in 2019.

I also retested the code snippet mentioned by Filip Sedlák in comment 13. This is the result of my tests:

class A:
    @cached_property
    def count(self):
        print('A.count()')
        return 15
        
class B(A):
    @property
    def count(self):
        print('B.count()')
        return super().count

b = B()
b.count  # calls B.count() then A.count() <-- expected
b.count  # calls B.count() but uses the cached value from A.count() <-- expected

so the mentioned misbeaviour seems to be fixed.

The post from Raymon Hettinger's thread on X (comment 12) has triggered its own discussion on ​https://discuss.python.org/t/finding-a-path-forward-for-functools-cached-property/23757/45 and as it seems the Python community went the same path as the Django implementation and removed the thread locking issue they had with version 3.8 and below.

So my proposal is to reopen this issue and start with an implementation deprecating Django's cached_property decorator.

[Jacob Rief]

reopen based on the forum discussion ​https://forum.djangoproject.com/t/why-does-django-implement-its-own-version-of-cached-property/46029/4

[Jacob Walls]

I don't see much pushback on the forum. It's confusing for newcomers (why use the vendored version if it's the same as the upstream one?) I think it makes sense to accept.

[Jacob Rief]

​https://github.com/django/django/pull/22026