Problem/Motivation
MenuTreeStorage::rebuild() only writes the discovered menu links (the menu-link plugin definitions, flagged discovered = 1), which come from code and live in a small, fixed set of menus. Links created from content (e.g. menu_link_content entities) are flagged discovered = 0 and are not written by a rebuild.
However, rebuild() picks the cache tags to invalidate via getMenuNames(), which returns every menu_name in the {menu_tree} table:
$affected_menus = $this->getMenuNames() + $before_menus;
$this->cacheTagsInvalidator->invalidateTags(Cache::buildTags('config:system.menu', $affected_menus, '.'));
So a rebuild invalidates a config:system.menu.* cache tag for every menu on the site, including menus it cannot have changed. rebuild() runs on every router/cache rebuild, so this happens on each cache clear.
On sites that create a menu per group/domain (e.g. og_menu, group_content_menu, domain menus), the number of menus grows with site content while the number of menus a rebuild actually writes stays fixed, so the redundant invalidations grow with it.
Steps to reproduce
See #5 for performance benchmark script.
Proposed resolution
Scope the lookup to menus that actually contain discovered links (the only ones rebuild() can affect).
Remaining tasks
?
User interface changes
None
Introduced terminology
None
API changes
None
Data model changes
None
Release notes snippet
None
Issue fork drupal-3605583
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
herved commentedComment #4
smustgrave commentedActually can drop the assert messages. Almost an unspoken rule we don’t include those, eventually there will be a code sniffing test.
As far as the change what’s the performance hit for the added query?
Comment #5
herved commentedThe real cost shows up on a full cache clear; the volume of menu cache-tag invalidations causes a lot of DB writes. The extra query the fix adds is marginal by comparison.
I updated the IS to clarify.
The script below creates 10K menu links, simulating a large site with many menus (e.g. og_menu, group_content_menu, domain menus). Once populated, compare
time drush crbetween the two branches. The fix avoids ~10K redundant cache-tag invalidations on every rebuild. Numbers on my machine:- without fix (main branch): 3.5s
- with fix (MR16126 branch): 1.8s
Comment #6
berdirInteresting, can confirm that discovered-only menus are only a relatively small subset of the total in one project with a lot of menus and group module, in my case 33 out of 236. I still see quite a few group-menus in that list. I feel like I don't entirely understand when menu links are discovered vs not, I don't really see a difference. comparing the links in some menus, all of them are menu_link_content, all are routed to a node, 3 out of 17 are discovered?
#3485030: Avoid saving menu links through node form when they do not change is somewhat related, but that was more about save() and not rebuild/saveRecursive(), still wondering if there's an option where we could be even more efficient and only invalidate the menus where we actually saved a change. will need to debug a bit through that code to better understand it.
Comment #7
smustgrave commentedCan we drop the assertions messages too
Comment #8
herved commentedSure, done, it's a bit redundant with the comment above anyway.
Comment #9
berdirSetting to needs work for the MR review.