Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Unnecessary complexity in current_content query set #417

Merged
merged 5 commits into from
Jul 30, 2024

Conversation

fsbraun
Copy link
Member

@fsbraun fsbraun commented Jul 14, 2024

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

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

@fsbraun fsbraun requested review from jrief and marksweb July 14, 2024 23:37
@fsbraun fsbraun changed the title fix: Unnecessary complexity in current_content manager fix: Unnecessary complexity in current_content query set Jul 15, 2024
@fsbraun fsbraun force-pushed the fix/current-content-fix branch from 4102b46 to 53af09b Compare July 22, 2024 08:37
@fsbraun
Copy link
Member Author

fsbraun commented Jul 26, 2024

Now also includes a vaguely related test for django-cms/django-cms#7967.

Copy link
Contributor

@Aiky30 Aiky30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks valid to me

@fsbraun fsbraun merged commit 9b8abd5 into master Jul 30, 2024
109 checks passed
@fsbraun fsbraun deleted the fix/current-content-fix branch July 30, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants