#35405 closed Cleanup/optimization (fixed)
Use @cached_property for FieldCacheMixin cache key
| Reported by: | Adam Johnson | Owned by: | Adam Johnson |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Ready for checkin | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
FieldCacheMixin is used by related fields to track their cached values. Its existing design means calling get_cache_name() for each operation, even though that value doesn’t change. Changing get_cache_name() into a cached property can thus save a bunch of small but useless function calls working with related fields.
I profiled this change by using cProfile on a selection of tests using related fields:
$ python -m cProfile -s cumtime -o profile runtests.py --parallel 1 foreign_object *relat* ... Found 399 test(s). ...
Before, there were 12,520 function calls:
$ python -m pstats profile <<< 'sort cumtime
stats 10000' | rg get_cache_name
11193 0.001 0.000 0.001 0.000 /Users/chainz/Documents/Projects/django/django/db/models/fields/related.py:512(get_cache_name)
712 0.000 0.000 0.000 0.000 /Users/chainz/Documents/Projects/django/django/db/models/fields/reverse_related.py:251(get_cache_name)
615 0.000 0.000 0.000 0.000 /Users/chainz/Documents/Projects/django/django/contrib/contenttypes/fields.py:143(get_cache_name)
After, there are just 227 calls (should be one per related field):
$ python -m pstats profile <<< 'sort cumtime
stats 10000' | rg cache_name
172 0.000 0.000 0.000 0.000 /Users/chainz/Documents/Projects/django/django/db/models/fields/related.py:512(cache_name)
34 0.000 0.000 0.000 0.000 /Users/chainz/Documents/Projects/django/django/db/models/fields/reverse_related.py:251(cache_name)
21 0.000 0.000 0.000 0.000 /Users/chainz/Documents/Projects/django/django/contrib/contenttypes/fields.py:143(cache_name)
The time saving is minimal here. It may be notable when working with a lot of model instances.
Change History (10)
comment:1 by , 18 months ago
| Patch needs improvement: | set |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 18 months ago
| Patch needs improvement: | unset |
|---|
comment:4 by , 18 months ago
| Needs tests: | set |
|---|
I agree the branch looks good but tests are missing, setting flags accordingly.
comment:5 by , 18 months ago
| Needs tests: | unset |
|---|
comment:6 by , 18 months ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Will merge once CI is green.
It's a change that isn't too invasive so I think it's worth doing. I think we should add a deprecation shim for
get_cache_namethough.