Problem/Motivation

Some of my clients either have already over 3000 nodes and 3000 menu links or plan to have this many and more.

Client relying heavily on the menu system.

  • using menu_breadcrumbs contrib module for breadcrumbs generated by menu links
  • using pathauto pattern built by menu link parents [node:menu-link:parents:join-path]/[node:menu-link:title]
  • using two content types, child/parent, parent has enabled menu link, landing page can be a child of another landing page but basic page cannot have children, basic page child gets a disabled menu link
  • custom code disables menu links for newly created 'child' items

about 400 parents (enabled links), 2100 children menu links (disabled menu links)

without patch, 29 seconds from an empty cache to load the node edit page

with patch, 3 seconds from an empty cache to load the node edit page.

Steps to reproduce

See problem/motivation description.

Proposed resolution

see patch

Remaining tasks

Come up with an acceptable way to improve performance and scalability, perhaps inspire yourselves by the attached patch.

User interface changes

Vastly improved performance

API changes

See patch, fairly straight forward to understand. Tests do not exist for disabled menu item usage with the node form. Existing tests pass against the proposed patch.

Data model changes

None

Release notes snippet

none (yet)

CommentFileSizeAuthor
#4 node_add_edit_performance_fix.png60.96 KBjoseph.olstad
#3 D9_performance_and_scalability-3214263-3.patch868 bytesjoseph.olstad

Comments

joseph.olstad created an issue. See original summary.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Related issues: +#3110371: When adding a new menu link, restrict the available parents to the current menu

this 'related' issue provides a performance and usability improvement and works well with the proposed patch in this issue.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
StatusFileSize
new D9_performance_and_scalability-3214263-3.patch868 bytes

Functional patch to start the discussion.

This patch drops the walltime from 29 seconds to 3 seconds with 2400 translated menu links/nodes on the node/edit, node/add page from an empty cache.

For benchmarks, I'm comparing only a rebuilt cache just prior to openning the node form (node/edit OR node/add)

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
StatusFileSize
new node_add_edit_performance_fix.png60.96 KB

xhprof gui

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

26 seconds was with the patch mentioned in comment #2, otherwise without was 29 seconds

the 3,3 seconds wall was with both mentioned patches.

The one that made the biggest improvement is patch 3.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Active » Needs review
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Issue summary: View changes

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This will need a valid test case to show the issue. For the related issue #3110371: When adding a new menu link, restrict the available parents to the current menu just posted in slack to the #contribute channel to see about getting a product manager review.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

retriggering automated tests for 10.1.x, 11.x

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

For the test scenario I ran with XHProf it was about 400 landing page menu links (parents, enabled links), 2100 children (basic page/children that are not allowed to have children/ disabled menu links)

Performance prior to patching was the best with the administrator role
Performance prior to patching was the worst with lesser roles that did not have the administer menus /administer content permission. These roles incur vastly more hits to the db for queries relating to permissions to access the entity linked by the menu link and for queries to build the (useless to node form) disabled link entity it'self.

Performance benchmarks were based on a cold cache node edit/node add page load.

Performance after patching for this scenario was improved by about 900%

a full XHProf performance profile was done for this, in our use case with 2500 (or so, maybe it was more) menu links it is over 50,000 db queries on menu link permissions and loading disabled menu data that have no bearing on anything in the node edit form/node add form. The vast majority of those in our test scenario were for disabled menu links that are actually unwanted. The end result without the patch is just noise for the node form and very poor performance (especially on a cold cache).

It's especially poor performance for non administrator roles.

The administrator role gets better performance because there's no query to check menu link access

however lesser roles do not have permissions for everything so the whole permissions stack runs for those. The menu link permissions checks are expensive and unneeded in this case.

There's a legitimate justification for this patch to go into a core release , it passes core automated testing btw.

Check the test results on the core patch, all passing (still 100% passing).

If there's no core coverage for this scenario it's probably because disabled menu links are not essential to the node forms functionality. Basically noise and wasted db queries.

I repeat, 900% performance improvement for our example use case, and this number only rises as the site grows.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Issue summary: View changes
skaught’s picture

skaught
Primary language English
commented

needs work:
a. patch is using \Drupal::routeMatch(). please use injection.
b. for our example use case, Indeed, from Drupal Core there are many use cases where various different editor roles would need to have access all menu items.. This process does not actually aid in 'building the list better'.

in General, I might assume we are missing related issues already open/in-progress that we should look into as well.

skaught’s picture

skaught
Primary language English
commented
Related issues: +#2346973: Improve usability, accessibility, and scalability of long select lists

@Joseph
Am wondering about the difference in what was achieved with #3110371: When adding a new menu link, restrict the available parents to the current menu now that this is on Drupal 11! are these goals still inline?

if this is the concern now for Wxt5, can we back port this ourselves (until D11 is branched for?)?

marat.agl’s picture

marat.agl commented

Thanks for the patch @joseph.olstad
I also encountered this problem and solved the issue in this way, since for me the output of more than 10,000 menu items was not of great importance, I limited the depth to level 2 (setMaxDepth), thereby I achieved normal operation of editing and creating a page, and I edit the menu through the bigmenu module .

Sample:

 $route = \Drupal::routeMatch()->getRouteName();
 if ($route == 'entity.node.edit_form' || $route == 'node.add' ) {
     // my big menu (10000 links)
     if ($menu_name='menu-menu-category' ) {
         $parameters->onlyEnabledLinks();
         $parameters->setMaxDepth(2);
    }
    }

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

still using patch 3 with Drupal 11.3.5 , significant improvement to our node forms for bundles that have a menu.