Performance Issue in processMenuLinkTree in Large Menus

Same issue with 8.x-2.19.

Same for me. The administration menu inherited of menu_item_extras functionalities.

Huge TTFB : 40 seconds to load a page !!!!!!

Updating from 8.x-2.19 to 3.x just fixed the Admin Toolbar hover issue, not the performance issue.

I'm having the same problem.

I am having the same issue after after adding a menu field that reference to a taxonomy that have a lots of terms.

+1 for same issue

Subscribing, processMenuLinkTree is taking up a lot of time in my slow processes in NewRelic

For some real world data, here is a screenshot from NewRelic with a before/after when i replaced Menu Item Extras with TB Megamenu.

Pre-replacement we were seeing average 5s page loads. After disabling menu item extras and replacing with TB Megamenu, we are seeing sub 1s page loads.

I can just confirm as well. The function is the most heavest currently on the site and takes 1/3 of total load time.

I'm more of a fronted / JS developer, but it seems that `getMenuLinkItemEntity` function uses pretty high level `entityTypeManager` to load every menu item one at a time. https://git.drupalcode.org/project/menu_item_extras/-/blob/3.0.x/src/Ser...

Maybe relevant: https://drupal.stackexchange.com/questions/283604/why-does-loadmultiple-...

Update #1: Currently testing a possible improvement for the caching @ https://git.drupalcode.org/issue/menu_item_extras-3370223/-/commits/3370...
My idea is to add more caching of the menu item entities. Currently there doesn't seem to be any caching and pretty much on every page load dozens (depending on your menu size) entities are being fetched and loaded. With this approach, I added a caching so only the initial page load will be a slow one. Afterwards the menu itself is being retrieved from cache which should be significantly faster. Hopefully will do a deployment this week in order to gather some newRelic data as performance changes.

Update #2: The patch serves out needs. Noticeably improves the performance in our case processMenuLinkTree now takes about 5% of total execution time compared to ~35% before. Further things to improve - probably one cache could be used for all languages instead of caching per language. https://git.drupalcode.org/project/menu_item_extras/-/merge_requests/37

I've attempted to performance-test the merge request here. I'm having a hard time coming up with reliable results showing it improving things. I've used Siege to test a set of pages, attempting differing amounts of concurrency, as both an authenticated user and not and cannot reliably get it to show a performance difference.

The best I've been able to achieve is using a profiler comparing loading 2 pages, both of which have menus on them:

Pre-patch:
- Page 1: 17.8s
- Page 2: 8.02s

Post-patch:
- Page 1: 17.8s
- Page 2: 6.6s

Understandably the savings is limited to the second page (though, interestingly, I might have thought the use of loadMultiple might have helped the first page load but while the number of queries is reduced the time is not reduced in my testing). As-expected, on the load of the 2nd page I can see virtually all of the savings comes in the processMenuLinkTree method, going from 2.07 s → 290 ms.

However, it's somewhat strange to me that I can't seem to find meaningful improvement when using Siege across a wider array of pages. I'm hitting each page once (concurrency of 1) to maximally benefit from caching (though I've experimented with other values), always running it immediately after a cache clear and it's just not consistently showing me a benefit despite repeated tests. I can seemingly only find the benefit in my limited profiling context. Once I turn off the profiler and even just use siege to hit the 2 pages that I was profiling I can no longer see a meaningful or consistent performance improvement with the patch in place which makes me wonder if there's something about the profiler overhead that's particularly responsive to the optimizations in the patch compared to how it is in the "real world" with the profiler disabled.

I've tested it with and without the patch using moderated/high synthetic non-logged traffic (20 concurrent) hitting the same hundred nodes and views during one hour each and it doesn't make any difference: it remains at almost half of a second using a menu of 30 elements and 7 fields.

Other graphs of execution time or memory have the same shape.

@DavisA - Thank you for the patch! (https://git.drupalcode.org/project/menu_item_extras/-/merge_requests/37.... if folks want the direct link).

It helped my team solve timeout issues with a site with ~1200 menu depth/links. Without the patch, uncached sites were constantly timing out. With the patch, while it is slow to load, timeouts no longer occur.

Confirming that the changes in the MR #40 I posted reduce response time from ~25 seconds to ~1500 ms for a large enterprise drupal 10 site with 1200 menu links.

Before:

After:

This also resolves that cache stampedes I was observing in redis previously as hgetall was 4.5 seconds and is now ~370 ms

Thanks for spotting this issue.

I can confirm that applying this patch to a website with a significant number of menu entries reduces the number of SQL queries by 33%.

Update: We found an edge case with this patch: it can cause issues when the same menu is loaded twice on the page, for instance, via two different blocks with different configurations. In this scenario, the first menu is cached with its block configuration, and then the second menu uses the same cached configuration, which can lead to issues if the two blocks have different configurations.

It seems to have stopped working with Drupal 10.4. When I downgrade the core to 10.3, it works again.

LogicException: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in Drupal\Core\Database\Connection->__sleep() (line 1920 of /var/www/web/core/lib/Drupal/Core/Database/Connection.php).

Backtrace
#0 [internal function]: Drupal\Core\Database\Connection->__sleep()
#1 /var/www/web/core/lib/Drupal/Component/Serialization/PhpSerialize.php(14): serialize(Array)
#2 /var/www/web/core/lib/Drupal/Core/Cache/DatabaseBackend.php(284): Drupal\Component\Serialization\PhpSerialize::encode(Array)
#3 /var/www/web/core/lib/Drupal/Core/Cache/DatabaseBackend.php(227): Drupal\Core\Cache\DatabaseBackend->doSetMultiple(Array)
#4 /var/www/web/core/lib/Drupal/Core/Cache/DatabaseBackend.php(215): Drupal\Core\Cache\DatabaseBackend->setMultiple(Array)
#5 /var/www/web/modules/contrib/menu_item_extras/src/Service/MenuLinkTreeHandler.php(139): Drupal\Core\Cache\DatabaseBackend->set('menu_link_conte...', Array, -1, Array)
#6 /var/www/web/modules/contrib/menu_item_extras/src/Service/MenuLinkTreeHandler.php(213): Drupal\menu_item_extras\Service\MenuLinkTreeHandler->getMenuLinkItemContent(Object(Drupal\menu_item_extras\Entity\MenuItemExtrasMenuLinkContent), 0, false)
#7 /var/www/web/modules/contrib/menu_item_extras/menu_item_extras.module(255): Drupal\menu_item_extras\Service\MenuLinkTreeHandler->processMenuLinkTree(Array, 'main')
...

probably related: https://www.drupal.org/project/drupal/issues/3400483

OK, sorry for the false alarm - it looks like the actual issue was caused by the upgrade of the drupal/menu_multilingual module (from 1.0.0-beta3 to 1.0.0). Also, the patch https://www.drupal.org/files/issues/2024-05-21/3366412-12.patch was added, so the problem likely originates from there.

After removing the patch and downgrading drupal/menu_multilingual to the previous version, the patch from here started working again

While the performance improvements gained by the changes in MR #40 are very very helpful, I can confirm the issue described in #18.
I tried extending the cache tags to include the menu level and menu depth, but the performance gains started to evaporate.

Does anyone have any idea how to push this forward, addressing the issue mentioned by guillaumeg in comment 18?

I have also encountered this performance issue.

I agree that MR 40 addresses this but I can confirm the problem mentioned by @guillaumeg and @dshields. It can be replicated by adding two blocks with the same menu, configuring these with the same starting level but different depths. The depth of the first block will be replicated by the second.

I also agree with the comments mentioned by @daspeter in the review of MR 40.

I have created a new merge request based on MR 40 to address two things:

1. The single-menu/different-configuration issue
2. Most review comments are either fixed or made obsolete

I can confirm that MR 50 works wonders for our (admittedely huge) menus.

This caused a regression in 3.1.2, see #3615084: [regression] Fatal error: 3.1.2 caches built render arrays.
The built render array is written to the cache backend, which can fatal in many scenarios, and the cache ID ignores cache contexts, a revert is proposed there.

Moving back to Needs Work, the assumptions described by @herved on the issue #3615084: [regression] Fatal error: 3.1.2 caches built render arrays need to be reviewed and addressed.

Extracted from the regression issue, sharing the admin's private information with anonymous users could be a security risk.

Apart from the points reported on #3615084: [regression] Fatal error: 3.1.2 caches built render arrays, we found another one:

• Introduced static cache $view_mode_cache in getMenuLinkContentViewMode().

The problem is the static state on a service keyed only by entity ID. If view_mode is translatable, the first translation requested pins the value for all others in the request, and the static never clears in long-running processes (Drush, queue workers). Also, if an entity is saved, it will not be refreshed. The saving is minimal, so the maybe could be removed. (Needs to be measured)

Also, maybe it should use drupal_static.
Changes introduced:

public function getMenuLinkContentViewMode(MenuLinkContentInterface $entity) {
  static $view_mode_cache = [];

  if (isset($view_mode_cache[$entity->id()])) {
    return $view_mode_cache[$entity->id()];
  }
  ...
  $view_mode_cache[$entity->id()] = $view_mode;
  return $view_mode;
}

It tries to cache the "view_mode" get, but that field should already be in memory:.
Code after the static cache:

if (!$entity->get('view_mode')->isEmpty()) {
  $value = $entity->get('view_mode')->first()->getValue();
  ...
}

As temporal solution is set the previous version on Composer

Context translation problem introduced on commit:
https://git.drupalcode.org/project/menu_item_extras/-/commit/1f7376e1e2a...

Before the changes, MenuLinkTreeHandler used the method "getMenuLinkItemEntity"

That gets the translation from the context:

  public function getMenuLinkItemEntity(MenuLinkInterface $link) {
...
    if ($menu_item) {
      $menu_item = $this->entityRepository->getTranslationFromContext($menu_item);
    }
    return $menu_item;
  }

But that code was removed, and now, the translation comes from the current language:
MenuLinkTreeHandler::processMenuLinkTree

          // Map entities by their IDs
          foreach ($entities as $entity) {
              // Pre-translate entities to current language if available
              if ($entity->hasTranslation($current_language)) {
                  $entity = $entity->getTranslation($current_language);
              }
              $entity_map[$entity->id()] = $entity;
          }

So, depending on how the site is configured the entity could diverge.
\Drupal\Core\Entity\EntityRepository::getTranslationFromContext. It uses "TYPE_CONTENT"

  public function getTranslationFromContext(EntityInterface $entity, $langcode = NULL, $context = []) {
    $translation = $entity;

    if ($entity instanceof TranslatableDataInterface && count($entity->getTranslationLanguages()) > 1) {
      if (empty($langcode)) {
        $langcode = $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
        $entity->addCacheContexts(['languages:' . LanguageInterface::TYPE_CONTENT]);
      }

$this->languageManager->getCurrentLanguage defaults to TYPE_INTERFACE

  public function getCurrentLanguage($type = LanguageInterface::TYPE_INTERFACE) {
    return $this->getDefaultLanguage();
  }

Restoring the issue to its previous status since the regression is already being addressed in the issue https://www.drupal.org/project/menu_item_extras/issues/3615084
Depending on where the maintainers decide to address the problem, the status of this issue may be changed again.

This issue caused a Critical regression that is also a security issue. If people who worked on this can help get the issue fixed over there, that'd be great.

#3615084: [regression] Fatal error: 3.1.2 caches built render arrays