Problem/Motivation

I got a ticket to investigate our drush cc all because now it is so slow to require a maintenance window. So I did a profile run and realized there's just too much functionality hanging off _menu_navigation_links_rebuild despite it does nothing in 99.99% of cases.

Proposed resolution

Similarly to #512962: Optimize menu_router_build() / _menu_router_save() but disjoint from it, do not rebuild the menu links which belong to unchanged router paths.

This simple approach needs to be refined as menu link properties can be defined in hook_menu. In this case, reuse the existing item query _menu_navigation_links_rebuild to check these properties didn't change. In one more optimization step, exempt known core paths from this query.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

chx created an issue. See original summary.

chx’s picture

chx’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2688893_2.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
2.26 KB

fetchAll vs fetchAllAssoc('path')

Status: Needs review » Needs work

The last submitted patch, 5: 2688893_2.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
2.42 KB

Turns out $item under rebuild have a number of underscored properties from router rebuild and navigation link reuses it. So same query but only path and weight (filler, could be anything) as fields and array_intersect_key($menu, $query->execute()->fetchAllKeyed()); to cut $menu to size.

Status: Needs review » Needs work

The last submitted patch, 7: 2688893_7.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
2.71 KB

Perhaps the schema sql call during test setup upsets it. I removed it and ran a test and it worked.

Status: Needs review » Needs work

The last submitted patch, 9: 2688893_9.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
9.1 KB

The used table type doesn't support BLOB/TEXT CREATE TEMPORARY TABLE {db_temporary_0} Engine=MEMORY SELECT this ancient .. still haunting us. Blargh!

Status: Needs review » Needs work

The last submitted patch, 11: 2688893_11.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
9.15 KB

Status: Needs review » Needs work

The last submitted patch, 13: 2688893_13.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
10.2 KB
1.26 KB
chx’s picture

FileSize
2.99 KB
12.44 KB

If a menu item changes menu name and has children then we have not moved those in #15 but now we do.

However, in #15 merely setting menu_name made the system keep the item. This means setting but not changing a menu root would enforce keeping the entire tree for link rebuilding and that's true for the admin tree so it'd make the patch almost ineffective.

Instead, we factor out the query from _menu_navigation_links_rebuild and run it to check whether the link-y things set has changed. That's much better! Compared to HEAD, we do not run more queries. Compared to #15 we do -- one for each menu item which has link-y things set. So I took the five paths where core sets menu_name in hook_menu and hardwired their link-y defaults and check against them. If you move the admin tree to another menu, no worries, we fall back to the query.

This is now correct and speedy.

chx’s picture

Issue summary: View changes
valthebald’s picture

Not only this method is faster, but it also consumes much less memory (average 10MB less with 3500 menu items)

Fabianx’s picture

Marking for review by me.

torgosPizza’s picture

Just wanted to chime in and mention that this patch helped us a LOT. I also have tested #753064: _menu_link_translate() might avoid calling _menu_load_objects(), #2688893: Optimize menu link rebuild and #1118028: PDOException in _menu_router_save() , but this patch gave the most positive result.

ksenzee’s picture

FileSize
12.45 KB
12.46 KB

#2762241: Missing default.profile from the file system added system_update_7081(). Here's a reroll, plus an interim patch for those of us who had this patch in production before system_update_7081() was added to core. (We'll all have some database surgery to do if and when this patch makes it in, but we already knew that. I hope.)