Skip to content

fix: Refactor menus app: significant time saving (queries and cpu) #7956

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 46 commits into from
Aug 7, 2024
Merged

fix: Refactor menus app: significant time saving (queries and cpu) #7956

merged 46 commits into from
Aug 7, 2024

Conversation

fsbraun
Copy link
Member

@fsbraun fsbraun commented Jul 11, 2024 •
edited
Loading

Description

Menu creation takes significant time for installations with a large number of pages. This PR reduces time needed for creating the menu and rendering the menu. Measurements were done on a test installation with ~55,000 pages (SQLite db).

Improvement 1: Generating the navigation node list

Creation of menu nodes is cached. So, this is a one-time gain after, say, adding a page.

The efficiency gain comes from identifying the relevant page content object only once per page object when translating the page list into a menu node list.

Improvement 2: Rendering the menu

The second angle of attack are the menu template tags: They depend on the cut_levels implementation, which was very inefficient. Especially, the extra_inactive parameter indicating how many inactive children should be present in the navigation had a computational time of O(n) with n being the number of pages/menu entries. The PR makes reduces this O(1).

Improvement 3: Versioning-induced db hits

An N+1 issue is introduced when generating the menu nodes through djangocms-versioning. Each menu node is checked against the home page to check if it needs to be cut. For this, the home page needs to be not in the navigation. Since, "not in navigation" is a page content property, this check triggers a database hit to find the relevant page content version. This PR only checks once if the home page is in the menu.

Additional improvement potential

  • No partial update of cached node lists.

@jrief Can you help to test this in a realistic environment?

Related resources

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 the channel #pr-reviews on our Discord Server to find a “pr review buddy” who is going to review my pull request.

@fsbraun fsbraun requested review from jrief and marksweb July 11, 2024 20:30
@fsbraun fsbraun changed the title fix: Refactor cms_menus.py: CPU time saving ~45% (measured with ~50.000 pages) fix: Refactor menus app: significant CPU time saving Jul 13, 2024
@fsbraun fsbraun marked this pull request as draft July 13, 2024 09:12
@fsbraun fsbraun marked this pull request as ready for review July 15, 2024 14:56
fsbraun added 2 commits July 17, 2024 14:23
@fsbraun
Merge branch 'develop-4' into feat/menu-perf
900583b
@fsbraun
Save one big query: Remove the need for page content prefetch (saving…
6ebc40f 
… one big query) by building the nodes based on page content objects instead of page objects
@fsbraun fsbraun changed the title fix: Refactor menus app: significant CPU time saving fix: Refactor menus app: significant time saving (queries and cpu) Jul 17, 2024
marksweb
marksweb reviewed Jul 22, 2024
View reviewed changes
@fsbraun fsbraun mentioned this pull request Jul 22, 2024
Closed
4 tasks
@fsbraun fsbraun mentioned this pull request Jul 22, 2024
Merged
4 tasks
@fsbraun
Copy link
Member Author

fsbraun commented Jul 24, 2024 •
edited
Loading

I added benchmarks from https://github.com/fsbraun/django-cms-benchmark:

57,500 pages Build node tree
Time in ms		 Build node tree
Queries		 Render nodes
Time in ms
Before 28,061 8,982 1,154
After 1,968 9 809
 Improvement  14x 1,000x 30%

Before

Total pages: 57498
------------------
.Build nodes.
-------------
/usr/local/lib/python3.11/site-packages/django/db/backends/base/base.py:176: UserWarning: Limit for query logging exceeded, only the last 9000 queries will be returned.
  warnings.warn(
Total nodes:          57498
Total queries:        8982
Total process time:   28061ms
Process time (cache):   633ms

.Show menu (nodes are cached).
------------------------------
Total process time:  1154ms
Calls cut_levels:    57498
Queries:             1
Generated menu size: 8kB

.Show page preview including menu.
----------------------------------
Total time:         31038ms
Calls cut_levels:   47911
Queries:            9000

.
----------------------------------------------------------------------
Ran 4 tests in 95.448s

After

Total pages: 57498
------------------
.Build nodes.
-------------
Total nodes:          57498
Total queries:        9
Total process time:    1968ms
Process time (cache):   552ms

.Show menu (nodes are cached).
------------------------------
Total process time:  809ms
Calls cut_levels:    6
Queries:             1
Generated menu size: 8kB

.Show page preview including menu.
----------------------------------
Total time:         4454ms
Calls cut_levels:   6
Queries:            21

.
----------------------------------------------------------------------
Ran 4 tests in 14.094s

@fsbraun fsbraun merged commit 59d50f2 into django-cms:develop-4 Aug 7, 2024
32 checks passed
@fsbraun fsbraun deleted the feat/menu-perf branch August 7, 2024 22:20
@jrief
Copy link
Contributor

jrief commented Aug 8, 2024

@fsbraun

@jrief Can you help to test this in a realistic environment?

on our system, this merge request now is running in production and a huge relief for our customers, especially those which are not superusers.

@fsbraun fsbraun mentioned this pull request Oct 31, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marksweb marksweb marksweb approved these changes

@jrief jrief Awaiting requested review from jrief

Assignees
No one assigned
Labels
component: menus component: performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fsbraun @jrief @marksweb