Pre-load non-admin routes

There's been lots of discussion of this in person/irc, but this might be the only issue.

I think we definitely need to look at pre-loading/caching the most commonly accessed routes, retitling and bumping priority.

Breadcrumbs create some inbound route lookup that we've not had so much before.

More profiling done on just how bad the route generator is at the moment: #2102777: Allow theme_links to use routes as well as href.

Tagging for reroll.

+++ b/core/lib/Drupal/Core/Routing/AdminRoutesPreloader.php
@@ -0,0 +1,133 @@
+      if (strpos($route->getPath(), '/admin') !== 0 && $route->hasDefault('_content')) {

Shouldn't that be === FALSE? Otherwise "/admin/structure/views/view/admin_view_custom" would match.

1. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +  /**
   +   * Contains the admin routes.
   +   *
   +   * @var array
   +   */
   +  protected $adminRoutes;
   +
   +  /**
   +   * Contains the admin routes while rebuilding the routes.
   +   *
   +   * @var array
   +   */
   +  protected $adminRoutesOnRebuild = array();
   

   Don't you mean non-admin?

2. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +        $this->adminRoutesOnRebuild[] = $name;
   

   Er, it's non-admin routes that we're pre-loading. Right?

Drupal.org fail again. Which of those should we review? :-)

Code great to me. I'd love a benchmark so we know how much this is helping (or hurting). After all, we are doing a lot of route loading. Perhaps we need to update menu link access checking before thats possible? Should we postpone for that?

1. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +    if ($this->negotiation->getContentType($event->getRequest()) == 'html') {
   

   Could we restrict more? What about partial HTML requests like a single block? Can we identify those versus full page requests?

+++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
@@ -0,0 +1,135 @@
+    $this->nonAdminRoutesOnRebuild = array_unique($this->nonAdminRoutesOnRebuild);

Why does this need to be saved to the object as well as the state system? A route rebuild is almost always followed immediately by a redirect, which would throw that away anyway. It seems unnecessary.

I'd prefer to punt on partial requests until we know for sure how to identify them. This entire issue is non-API-changing, just performance optimization, so we can refine it at our leisure as we decide what qualifies as "good enough" performance.

Speaking of, what's the best way to benchmark this? It's something that would affect the whole request so my knee-jerk thought is ab, but Mark yells at me every time I try to benchmark anything without xhprof. :-)

xhprof for the whole request works great.

Patch looks great. I only have minor points.

1. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +   * Load all the admin routes right before the actual page is rendered.
   

   Load -> Loads
   perhaps "the" can be dropped.

2. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +   *   The Event to process.
   

   Event -> event

3. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +    // Just preload on normal html pages, as they will contain the menu links.
   

   html -> HTML

   More importantly: What does this have to do with menu links? It seems that would be important information to document more explicitly.

4. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +   * Contains the non-admin routes.
   

   Here and elsewhere: I'm wondering whether there isn't a better word for "non-admin", i.e. a positive word. I could come up with anything, but non-admin is certainly suboptimal.

5. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +      // Just preload non admin and _content routes, as they are the ones
   

   non admin -> non-admin

   It seems strange to say "_content routes". It's sort of obvious what you mean, but _content is not an actual word :-)

6. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +      // that appear on menu links and co.
   

   See above regarding "menu links and co."

7. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +    $this->nonAdminRoutesOnRebuild = array_unique($this->nonAdminRoutesOnRebuild);
   +    $this->state->set('routing.non_admin_routes', $this->nonAdminRoutesOnRebuild);
   

   Because of the array_unique() I don't think anything would actually go wrong currently, but I think we should listen to the REBUILD_FINISHED event and reset $this->nonAdminRoutesOnRebuild. This is useful if route rebuilding happens multiple times per request.

8. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +    $events[RoutingEvents::ALTER] = array('onAlterRoutes', -1024);
   

   Any reason for the -1024? If so, a comment would be great.

9. +++ b/core/lib/Drupal/Core/Routing/NonAdminRoutesPreloader.php
   @@ -0,0 +1,135 @@
   +    // Come before the HtmlViewSubscriber.
   

   Maybe: This needs to run before HtmlViewSubscriber.

   Also: *Why* that is the case would also be important to document. That's not saying much but to me, at the very least, that's not obvious.

10. +++ b/core/tests/Drupal/Tests/Core/Routing/NonAdminRoutesPreloaderTest.php
    @@ -0,0 +1,165 @@
    +   * Tests onRequest on a non html request.
    

    html -> HTML

Good idea, but I really don't like depending on the path for this (or anything in D8) - can we flag non-admin routes when they are defined?

We shouldn't ship with this fragile path matching implementation - though it might be ok to go in if we have a critical follow-up issue to define something better.

For example - we could add to the pre-loaded list all routes accessed by users with a certain role.

To test this, I used a node page viewed as anonymous user, after giving anonymous enough perms to produce a few more links.

As you can see, the number of queries made by the router is cut in half:

7 is still kind of a lot, but this is certainly an improvement. I'd be ok with this going in and then we can continue to tune it as we discover more.

Also, we should rename this class so that it doesn't have the implementation details in the name. Just "RoutePreloader" would be fine.

Let's try this image again:

Looks like a good improvement. Do you know which routes the 7 remaining queries are for?

Cross posting #1316692: Convert hook_admin_paths() into declarative properties on routes

+  protected function getNonAdminRoutes() {
+    if (!isset($this->nonAdminRoutes)) {
+      $this->nonAdminRoutes = $this->state->get('routing.non_admin_routes', array());
+    }
+    return $this->nonAdminRoutes;
+  }
...
+      $this->loadNonAdminRoutes();
...
+  protected function loadNonAdminRoutes() {
+    if ($routes = $this->getNonAdminRoutes()) {
+      $this->routeProvider->getRoutesByNames($routes);
+    }
+  }

Do we need this conditional multi-layer loading for any particular reason or could we merge the whole logic into getNonAdminRoutes?

Also, as @msonnabaum already mentioned, I'd additionally leave the "non-admin" detail out of the method names — it's perfectly possible that we want to preload further routes, so just getRoutes() would be sufficient?

Turns out the remaining 7 were routes like user.logout that don't have _content, so we were excluding them. I don't really see why we'd want to, so I removed that check and now it's down to 2 queries from 14.

This patch also renames the class, although I didnt rename the methods because they are private. I had the same thought about getNonAdminRoutes, so I removed that and the property that caches those, since onRequest should only get called once anyways.

That won't help user.logout, which is _controller (it returns a redirect).

Think this patch is what you meant.

That said, I really really dislike this:

--- a/core/modules/user/user.routing.yml
+++ b/core/modules/user/user.routing.yml
@@ -10,6 +10,8 @@ user.logout:
   path: '/user/logout'
   defaults:
     _controller: '\Drupal\user\Controller\UserController::logout'
+  options:
+    _preload: TRUE
   requirements:
     _user_is_logged_in: 'TRUE'

This will result in additional queries because contrib modules will forget to add this option, and they will only be found by people who are profiling, which is very few. A typical module author should never have to worry about route preloading.

Also, I'm not crazy about arguing that we need to whitelist without any data about memory usage to show that it's worthwhile at all.

Just checked two 7.x sites - both are relatively complex, but don't have a ridiculous number of contrib modules enabled.

120 modules | 120+ non admin/% routes | 600+ admin routes.

150 modules | 180+ non admin/% routes | 700+ admin/% routes.

The most modules I've ever seen installed on a site was over 300, let's say 300 non-admin routes for that one, don't have that db handy.

Not sure what that gives us, but I' think we need to be reasonably OK up to 150-200 total non-admin routes, past that you have other problems to worry about.

If this was 7.x, we'd very easily be able to see which routes had menu links pointing to them, vs. MENU_CALLBACK which is nearly always going to be inbound rather than outbound.

I don't see a way to get that information without introducing a dependency on the menu link system or using CacheCollector to build the list though.

I don't quite get what #49 is showing. Don't you just want to measure memory usage between the number of routes passed to getRoutesByNames?

Talked to catch and dawehner in IRC about this and we came to the conclusion that we should just go with the simplest version in #42, then we can open a followup to optimize memory usage by switching it to something like the CacheCollector pattern.

Attached is the same as #42.

We have green, and a consensus. RTBC

52: route_rebuild-2058845-52.patch queued for re-testing.

#11 is wrong

php -r "var_dump(strpos('/admin/structure/views/view/admin_view_custom', '/admin'));"
int(0)

if (strpos($route->getPath(), '/admin') === FALSE) {

Should be

$path = $route->getPath();
if ($path != '/admin' && strpos($path, '/admin/') !== 0) {

Afaics

Also noticed

use Drupal\Core\KeyValueStore\KeyValueStoreInterface;

Unused use

Thanks!

@dawehner I suggested

$path = $route->getPath();
if ($path != '/admin' && strpos($path, '/admin/') !== 0) {

For a reason... what if you have a site that is aimed some type or types of administrators and you have views on paths like /administrator/typeA and /administrator/typeB or whatever.

59: route_rebuild-2058845-59.patch queued for re-testing.

I agree, this is an entirely optional optimization with no API impact. We or contrib and rip it out and rewrite it whenever we feel like it without breaking anything; it certainly doesn't need an API of its own. Let's land it.

Committed 3cd6aa8 and pushed to 8.x. Thanks!