django-cms / djangocms-versioning

General purpose versioning package for Django CMS 4 and above.
Other
34 stars 30 forks source link

fix: Unnecessary complexity in `current_content` query set #417

Closed fsbraun closed 4 months ago

fsbraun commented 4 months ago

Description

Due to a typo, the current_content() method of the admin manager of versioned objects had quadratic instead of linear complexity in the number of version objects in the database.

For most day-to-day situations, this is not relevant, since mostly single or only a few content objects are fetched.

For large query sets, such as when the menu tree is built, this has a tremendous impact.

Functionally, the change is irrelevant. Also, on Python level there is no impact. The performance gain comes solely from the database.

When testing with SQLite, 50.000 pages a db hit for the single query set reduced from 5 minutes to 1 second.

Explanation

Before the change in current_content() method of the queryset injected into versioned models:

qs = self.filter(versions__state__in=(constants.DRAFT, constants.PUBLISHED))\
    .values(*self._group_by_key)\
    .annotate(vers_pk=models.Max("versions__pk"))\
    .values("vers_pk")
return qs.filter(versions__pk__in=qs)

After - change is in the last line (other changes in the PR are cosmetics, really):

qs = self.filter(versions__state__in=(constants.DRAFT, constants.PUBLISHED))\
    .values(*self._group_by_key)\
    .annotate(vers_pk=models.Max("versions__pk"))\
    .values("vers_pk")
return self.filter(versions__pk__in=qs)

Before the change, the qs filters relevant versions, and then the returned qs does it again. This leads to two inner joints on versions (n^2 complexity).

After the change, the qs filters the relevant versions, and then filters the original qs (self) : One inner joint, linear complexity.

Checklist

fsbraun commented 4 months ago

Now also includes a vaguely related test for https://github.com/django-cms/django-cms/pull/7967.