Cache MenuActiveTrail::getActiveIds() for *all* menus per route match: 1 cache get instead of N DB queries, saves 1 ms/response

Quick fix of the issue summary to not contain the same picture twice

Noticed today that apart from actual router matching, this is the only non-cache, non-state database query we execute in the minimal profile on every request.

So it represents potentially a 50% reduction in queries to the actual database when using different cache/state backends.

I'm curious, how often do we things practically store per URL already?

+++ b/core/lib/Drupal/Core/Menu/MenuActiveTrail.php
@@ -38,16 +41,66 @@ class MenuActiveTrail implements MenuActiveTrailInterface {
+    parent::__construct(NULL, $cache, $lock);

Should we document that CID will be calculated in getCid() ?

1. +++ b/core/lib/Drupal/Core/Menu/MenuActiveTrail.php
   @@ -38,16 +41,66 @@ class MenuActiveTrail implements MenuActiveTrailInterface {
   +      ksort($route_parameters);
   +      return 'active-trail:route:' . $this->routeMatch->getRouteName() . ':route_parameters:' . serialize($route_parameters);
   +    }
   

   I like that a lot that this is all still inside MenuActiveTrail, so in case someone decided to build a trail, for example based upon history, it would be possible to do so.

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -84,53 +60,31 @@ class MenuLinkTree implements MenuLinkTreeInterface {
      public function getCurrentRouteMenuTreeParameters($menu_name) {
   -    $route_parameters = $this->routeMatch->getRawParameters()->all();
   -    ksort($route_parameters);
   -    $cid = 'current-route-parameters:' . $menu_name . ':route:' . $this->routeMatch->getRouteName() . ':route_parameters:' . serialize($route_parameters);
   -
   -    if (!isset($this->cachedCurrentRouteParameters[$cid])) {
   -      $cache = $this->cache->get($cid);
   -      if ($cache && $cache->data) {
   -        $parameters = $cache->data;
   -      }
   -      else {
   -        $active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);
   -
   -        $parameters = new MenuTreeParameters();
   -        $parameters->setActiveTrail($active_trail)
   -          // We want links in the active trail to be expanded.
   -          ->addExpandedParents($active_trail)
   -          // We marked the links in the active trail to be expanded, but we also
   -          // want their descendants that have the "expanded" flag enabled to be
   -          // expanded.
   -          ->addExpandedParents($this->treeStorage->getExpanded($menu_name, $active_trail));
   -
   -        $this->cache->set($cid, $parameters, CacheBackendInterface::CACHE_PERMANENT, array('config:system.menu.' . $menu_name));
   -      }
   -      $this->cachedCurrentRouteParameters[$menu_name] = $parameters;
   -    }
   -
   -    return $this->cachedCurrentRouteParameters[$menu_name];
   +    $active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);
   

   Great change!

3. +++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
   index 2ea72ff..16f4b8c 100644
   --- a/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
   
   --- a/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
   +++ b/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
   
   +++ b/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
   @@ -62,8 +62,10 @@ protected function setUp() {
   +    $cache = $this->getMock('\Drupal\Core\Cache\CacheBackendInterface');
   +    $lock = $this->getMock('\Drupal\Core\Lock\LockBackendInterface');
    
   -    $this->menuActiveTrail = new MenuActiveTrail($this->menuLinkManager, $this->currentRouteMatch);
   +    $this->menuActiveTrail = new MenuActiveTrail($this->menuLinkManager, $this->currentRouteMatch, $cache, $lock);
      }
   

   I'm curious whether we should adapt the unit test to test the new caching code, it would be possible, this is for sure.

I'm curious, how often do we things practically store per URL already?

Path alias pre-load cache is also per-URL.

Well, I'd kinda like to save us from breaking the following piece of code for example:

+  protected function getCid() {
+    if (!isset($this->cid)) {
+      $route_parameters = $this->routeMatch->getRawParameters()->all();
+      ksort($route_parameters);
+      return 'active-trail:route:' . $this->routeMatch->getRouteName() . ':route_parameters:' . serialize($route_parameters);
+    }

Nothing documents why the ksort() is needed, yes I know, but maybe someone else doesn't know in the future. Having test coverage for that helps against that.

Well, then lets do it. I think the patch itself is sane

RTBC, looks great now with the added test coverage.

Train of thought

I pondered this change for a moment, but because blocks are plugins it is pretty unlikely that someone will display a menu manually without going through the block (or some other #pre_render cache).

[ Also the code to display a menu manually is really complicated and placing a block is really simple. ]

This looks great. I also saw these queries when profiling with the minimal profile/front page/logged out - they're the only uncached database queries apart from path alias and routing lookups. So this will both reduce the number of queries and it also cuts out overhead from SelectQuery.

Committed/pushed to 8.0.x, thanks!