Opened 4 years ago

Last modified 8 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 Adam Johnson)

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 Adam Johnson, 4 years ago

Description: modified (diff)

comment:2 by Simon Charette, 4 years ago

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.

comment:3 by Claude Paroz, 4 years ago

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

comment:4 by Mariusz Felisiak, 4 years ago

Component: UncategorizedUtilities
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: UnreviewedSomeday/Maybe
Type: UncategorizedCleanup/optimization

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

comment:5 by David Smith, 3 years ago

Owner: changed from nobody to David Smith
Status: newassigned

comment:6 by David Smith, 3 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:7 by Adam Johnson, 3 years ago

Cc: Adam Johnson added

LGTM, thanks for the benchmarking David

comment:8 by David Smith, 3 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 Rainer Koirikivi, 3 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

in reply to:  2 comment:10 by Mateusz Leszko, 3 years ago

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?

comment:11 by Adam Johnson, 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 Thomas Grainger, 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 Filip Sedlák, 22 months 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 Akshet Pandey, 8 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.

Note: See TracTickets for help on using tickets.
Back to Top