Skip to content

perf: small reduction of DB queries #7887

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

Closed
wants to merge 1 commit into from

Conversation

jrief
Copy link
Contributor

@jrief jrief commented Apr 23, 2024

Description

This PR reduces the number of DB queries by a few.

Related resources

  1. chunk: fallback languages can be cached inside the Page object.
  2. chunk: Replacing the .filter(…) on the queryset against .all() actually reduces the number of queries. This is because that queryset in most cases is already consumed. Calling it with .filter(…) actually fetches the data a second time. Therefore filtering in Python is cheaper, especially if the number of languages never exceeds a significant number.

Also read https://docs.djangoproject.com/en/5.0/ref/models/querysets/#values

Checklist

  • I have opened this pull request against develop-4
  • 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.

@jrief jrief changed the title small reduction of DB queries [perf] small reduction of DB queries Apr 23, 2024
@marksweb marksweb changed the title [perf] small reduction of DB queries perf: small reduction of DB queries Apr 23, 2024
@marksweb marksweb force-pushed the fix-reduce-db-queries branch from e6890d7 to 42d2717 Compare April 23, 2024 15:34
@fsbraun
Copy link
Member

fsbraun commented Apr 24, 2024

I like the all() optimisation.

How does caching the fallback languages help performance. The fallback languages only depend on the settings, or am I missing something?

@vinitkumar
Copy link
Member

@jrief Are there any updates on this? Also, would be great to see the number of queries reduction shown by Django Debug Tool or something? In my opinion, it would be help remember the rationale behind the change in future.

@marksweb marksweb force-pushed the fix-reduce-db-queries branch from 0a94ec4 to 49c4dbc Compare July 7, 2024 09:03
fsbraun added a commit to fsbraun/django-cms that referenced this pull request Jul 29, 2024
fsbraun added a commit that referenced this pull request Aug 7, 2024
…7956)

* Refactor cms_menus.py: CPU time saving ~45% (measured with ~50.000 pages)

* Remove page content lookup from the loop

* Fix db accesses

* Optimize menu tags, especially `cut_levels

* Fix EmptyPageContent regression

* chore: update signature of `cut_after

* Refactor cms_menus

* Save one big query: Remove the need for page content prefetch (saving one big query) by building the nodes based on page content objects instead of page objects

* Adjust tests and add release note deprecation information

* Remove debugging code

* Update test to reflect lower query number

* Respect PageContentAdmin `get_queryset` in pagetree.

* Refactor cms_menus.py: CPU time saving ~45% (measured with ~50.000 pages)

* Remove page content lookup from the loop

* Fix db accesses

* Optimize menu tags, especially `cut_levels

* Fix EmptyPageContent regression

* chore: update signature of `cut_after

* Refactor cms_menus

* Save one big query: Remove the need for page content prefetch (saving one big query) by building the nodes based on page content objects instead of page objects

* Adjust tests and add release note deprecation information

* Remove debugging code

* Update test to reflect lower query number

* Respect PageContentAdmin `get_queryset` in pagetree.

* Add docsstrings

* Improve naming

* Add compatibility shim for `get_node_for_page` function.

* Fix deprecation warning

* Extend tests

* Add deprecation message to Level modifier docstring

* Ensure availability of `languages` property in `CMSMenu

* Optimize page content admin by prefetching the page object

* Update docstrings

* Query optimization from #7887

* Remove duplicate `"page__is_home"` from `.only()` expression
@fsbraun
Copy link
Member

fsbraun commented Aug 19, 2024

@jrief One change has been merged into develop-4 (

url.language: url for url in self.urls.all() if url.language in languages # TODO: overwrites multiple urls
). The other change (caching fallback languages) imho does not reduce db hits, since fallback languages are a matter of configuration only.

Can we close this?

@fsbraun fsbraun closed this Nov 4, 2024
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.

3 participants