Opened 6 months ago

Closed 6 months ago

Last modified 2 months ago

#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 (9)

comment:1 by Simon Charette, 6 months ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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_name though.

comment:2 by Adam Johnson, 6 months ago

Patch needs improvement: unset

comment:3 by Simon Charette, 6 months ago

Patch is LGTM

comment:4 by Natalia Bidart, 6 months ago

Needs tests: set

I agree the branch looks good but tests are missing, setting flags accordingly.

comment:5 by Adam Johnson, 6 months ago

Needs tests: unset

comment:6 by Natalia Bidart, 6 months ago

Triage Stage: AcceptedReady for checkin

Will merge once CI is green.

comment:7 by nessita <124304+nessita@…>, 6 months ago

Resolution: fixed
Status: assignedclosed

In b9838c65:

Fixed #35405 -- Converted get_cache_name into a cached property in FieldCacheMixin.

FieldCacheMixin is used by related fields to track their cached values.
This work migrates get_cache_name() to be a cached property to optimize
performance by reducing unnecessary function calls when working with
related fields, given that its value remains constant.

Co-authored-by: Simon Charette <charette.s@…>
Co-authored-by: Sarah Boyce <42296566+sarahboyce@…>
Co-authored-by: Natalia <124304+nessita@…>

comment:8 by nessita <124304+nessita@…>, 2 months ago

In 39abd56:

Refs #35405 -- Adjusted deprecation warning stacklevel in FieldCacheMixin.get_cache_name().

comment:9 by Natalia <124304+nessita@…>, 2 months ago

In dd58edc:

[5.1.x] Refs #35405 -- Adjusted deprecation warning stacklevel in FieldCacheMixin.get_cache_name().

Backport of 39abd56a7fb1e2f735040df0fdfc08f57d91a49b from main.

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