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

Command icon 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

herved created an issue. See original summary.

herved’s picture

Issue summary: View changes
Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Actually 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?

herved’s picture

Issue summary: View changes
Status: Needs work » Needs review

The 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 cr between 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

<?php

/**
 * Populates the site with many content-managed menu links to demonstrate the
 * cost of MenuTreeStorage::rebuild() invalidating a cache tag for every menu on
 * the site. rebuild() runs on every `drush cr`.
 *
 * Each link is a normal MenuLinkContent entity in its own menu, so it has
 * discovered = 0 -- exactly like the menu links a site (or og_menu) creates.
 *
 *   drush scr populate_menus.php     # create the links
 *   time drush cr                    # measure, on 11.x and on the fix branch
 */

// Number of content menus to create. Bump it to make the difference larger.
$count = 10000;

$storage = \Drupal::entityTypeManager()->getStorage('menu_link_content');

// Remove any links from a previous run first, so this is repeatable.
$ids = $storage->getQuery()
  ->accessCheck(FALSE)
  ->condition('menu_name', 'bench_menu_%', 'LIKE')
  ->execute();
if ($ids) {
  $storage->delete($storage->loadMultiple($ids));
}

$t = microtime(TRUE);
for ($i = 0; $i < $count; $i++) {
  $storage->create([
    'title' => 'bench',
    'link' => ['uri' => 'route:<front>'],
    'menu_name' => "bench_menu_$i",
  ])->save();
}
printf("Created %d content menus in %.0fs. Now run: time drush cr\n", $count, microtime(TRUE) - $t);
berdir’s picture

Interesting, 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.

smustgrave’s picture

Can we drop the assertions messages too

herved’s picture

Sure, done, it's a bit redundant with the comment above anyway.

berdir’s picture

Status: Needs review » Needs work

Setting to needs work for the MR review.