Opened 4 years ago

Closed 4 years ago

#24590 closed Cleanup/optimization (fixed)

Cache return value of `swappable_setting`

Reported by: Marten Kenbeek Owned by: Markus Holtermann
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Field.swappable_setting is called very often during model rendering. It loops over all models until it finds a match or otherwise returns None. Most models aren't swappable, so in most cases it never breaks out of the loop.

This method should be cached to speed up rendering in migrations. The Field instances are constantly copied, so that's not a good place, but it always uses django.apps.apps. Moving this looping over models to Apps.get_swappable_setting and using an lru_cache speeds up rendering quite a bit.

Both AUTH_USER_MODEL and the models contained in the main apps instance should never change, except for testing scenario's. The cache should never have to be cleared in non-testing scenario's, which results in another very significant speed-up, for a total of about 35% faster rendering in my benchmarks.

Change History (13)

comment:2 Changed 4 years ago by Markus Holtermann

Triage Stage: UnreviewedAccepted

comment:3 Changed 4 years ago by Tim Graham

Patch needs improvement: set

comment:4 Changed 4 years ago by Markus Holtermann

Owner: changed from Marten Kenbeek to Markus Holtermann
Status: newassigned

comment:5 Changed 4 years ago by Markus Holtermann

Needs documentation: set
Patch needs improvement: unset

comment:6 Changed 4 years ago by Markus Holtermann

Needs documentation: unset

comment:7 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:8 Changed 4 years ago by Markus Holtermann <info@…>

Resolution: fixed
Status: assignedclosed

In e1427cc6:

Fixed #24590 -- Cached calls to swappable_setting.

Moved the lookup in Field.swappable_setting to Apps, and added
an lru_cache to cache the results.

Refs #24743

Thanks Marten Kenbeek for the initial work on the patch. Thanks Aymeric
Augustin and Tim Graham for the review.

comment:9 Changed 4 years ago by Tim Graham

Has patch: unset
Triage Stage: Ready for checkinAccepted

Looks like the new tests aren't isolated as they fail in reverse: $ ./tests/runtests.py --reverse migrations

comment:10 Changed 4 years ago by Tim Graham

Resolution: fixed
Status: closednew

comment:11 Changed 4 years ago by Markus Holtermann

This change isolates the tests in migrations that rely on a replaced swappable model: https://github.com/django/django/pull/5199

comment:12 Changed 4 years ago by Markus Holtermann <info@…>

In 8e631a3:

Refs #24590 -- Ensured isolation between autodetector tests

Fixed a regression introduced in e1427cc609fa6ab247501b101cfb3c0092aba55b when running tests in reverse order.

comment:13 Changed 4 years ago by Markus Holtermann

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top