Cache the query in RouteProvider::preloadRoutes()

• Let's cache the loading of individual routes
• Cache also the state query.

Note: The patch currently has https://www.drupal.org/files/issues/2480811-73.patch included.

haha, good try daniel

.

Rerolled that.

Just some minor nits. That said, I'd like to see benchmark numbers on this to make sure the added dependencies actually buy us something measurable.

1. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -83,6 +84,11 @@ class RouteProvider implements PreloadableRouteProviderInterface, PagedRouteProv
   +   * Cache ID prefix used to load routes.
   +   */
   +  const ROUTE_LOAD_CID = 'route_provider.route_load:';
   

   If it's a cid prefix, not a complete cid, the name should reflect that.

2. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -339,6 +355,7 @@ public function getAllRoutes() {
   +    Cache::invalidateTags(['routes']);
   

   Please inject the service. No static calls in objects.

For sure,

There we go.

Nothing special, just sweeping up some nits

1) service definition now mentions cache_tags.invalidator
2) Added standard optional notation for final parameter
3) refactored constrictor to use arguments, in the order in which they appear - just for cosmetic reasons.

(1) makes ActionUnistallTest pass.

Opps my patch crashed in behind #9 sorry for the confusion.

Didn't do profiling, but look a bit at the amount of routes that we're preloading.

On a standard installation, just 30. On our production site, 272 routes... Including every public view and page_manager route. You quickly have quite a few of those, obviously. Wondering if we should somehow limit that by default or try to filter our a few more things.

{router} is the table with the most time spent according to new relic for us. We're not yet on the latest version, so we don't benefit from the route caching that @catch worked on but this query is very likely more expensive. I'll try to do some profiling locally.

just fixing the failing test.

On a standard installation, just 30. On our production site, 272 routes... Including every public view and page_manager route. You quickly have quite a few of those, obviously. Wondering if we should somehow limit that by default or try to filter our a few more things.

Mh, that indeed seems to be too much. I'm curious whether we should add a flag on routes 'preload': TRUE and add this to node/{node} taxonomy/{taxonomy_term}
and user/{user} and maybe each view with a page display and some more?

@Berdir
I agree that we preload too many routes, but does that mean we should change it as part of this issue? IMHO this issue is about getting rid of the SQL queries and replace them with caches. How many entries we have doesn't matter for that case.

I did some profiling but as expected (as I use database caching) this does not make things slower.

=== 8.0.x..8.0.x compared (55ae1487df572..55ae151f9861d):

ct  : 27,935|27,935|0|0.0%
wt  : 63,237|63,327|90|0.1%
mu  : 14,352,952|14,352,992|40|0.0%
pmu : 14,419,544|14,419,480|-64|-0.0%

=== 8.0.x..2508445 compared (55ae1487df572..55ae152e511af):

ct  : 27,935|27,959|24|0.1%
wt  : 63,237|63,240|3|0.0%
mu  : 14,352,952|14,358,096|5,144|0.0%
pmu : 14,419,544|14,424,096|4,552|0.0%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2508445           |     27,959 |     29,696 |     27,976 |     27,959 |     27,959 |
| 8_0_x             |     27,935 |     29,622 |     27,952 |     27,935 |     27,935 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2508445           |     63,240 |     79,360 |     66,558 |     65,676 |     71,328 |
| 8_0_x             |     63,327 |     89,646 |     67,108 |     66,287 |     72,147 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2508445           | 14,358,096 | 14,737,912 | 14,391,373 | 14,358,096 | 14,709,544 |
| 8_0_x             | 14,352,992 | 14,762,456 | 14,392,346 | 14,352,992 | 14,704,600 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2508445           | 14,424,096 | 14,868,272 | 14,458,064 | 14,424,096 | 14,774,560 |
| 8_0_x             | 14,419,480 | 14,894,200 | 14,459,594 | 14,419,480 | 14,769,664 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

Absolutely, I was trying to say that it is very worth to cache because there's so much. And we should additionally, in a separate issue, try to improve what we are caching.

For the benchmark, you could simulate having my amount of routes by creating 200-300 views clones?

Well yeah sure, let's do that, even technically the actual improvement is somewhere else.

<?php

/**
 * @file
 * Create all the views.
 */

$view = \Drupal\views\Entity\View::load('frontpage');
for ($i = 0; $i < 412; $i++) {
  $clone = $view->createDuplicate();

  $clone->set('id', 'frontpage_' . $i);
  $clone->save();
}

?>

<code>
=== 8.0.x..2528178 compared (55ae2241896c2..55ae229f4167b):

ct  : 31,231|31,231|0|0.0%
wt  : 111,003|110,348|-655|-0.6%
mu  : 19,104,864|19,104,536|-328|-0.0%
pmu : 19,250,872|19,250,736|-136|-0.0%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2528178           |     31,231 |     32,967 |     31,248 |     31,231 |     31,231 |
| 8_0_x             |     31,231 |     32,918 |     31,248 |     31,231 |     31,231 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2528178           |    110,348 |    149,659 |    117,750 |    116,962 |    124,511 |
| 8_0_x             |    107,262 |    140,066 |    117,903 |    117,109 |    126,136 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2528178           | 19,104,536 | 19,439,760 | 19,123,339 | 19,104,536 | 19,411,544 |
| 8_0_x             | 19,104,544 | 19,471,648 | 19,126,770 | 19,104,568 | 19,411,888 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2528178           | 19,250,664 | 19,639,656 | 19,270,284 | 19,250,736 | 19,557,928 |
| 8_0_x             | 19,250,256 | 19,671,232 | 19,273,206 | 19,250,256 | 19,557,056 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

dawehner and I discussed this patch at length in today's WSCCI meeting. Short version: RTBC. This is probably the limit of how far we should try to optimize the SQL-based routing system, though. Further efforts should focus on alternative backends than an SQL table for something that's not all that SQL-ish to begin with. :-)

The only thing we might should look at is whether we still want to preload that many routes by default.

Added a new issue for that: #2543016: Move route preloading into the UrlGenerator: don't simply preload on every HTML request

1. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -76,6 +78,13 @@ class RouteProvider implements PreloadableRouteProviderInterface, PagedRouteProv
   +   * The cache tag invalidator.
   ...
   +   * @var \Drupal\Core\Cache\CacheTagsInvalidatorInterface
   ...
   +  protected $cacheTagInvalidator;
   

   Nit: the interface has "tags", plural, the docs and property here have it singular.

2. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -83,6 +92,11 @@ class RouteProvider implements PreloadableRouteProviderInterface, PagedRouteProv
   +  const ROUTE_LOAD_CID_PREFIX = 'route_provider.route_load:';
   

   Supernit: this is the first time I see a prefix that has the colon included.

3. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -339,6 +366,7 @@ public function getAllRoutes() {
   +    $this->cacheTagInvalidator->invalidateTags(['routes']);
   

   Do we have other places that should use this new 'routes' cache tag?

See #2154461: Consider persistent caching and/or CacheCollector for state system for a more generic potential approach.