Updated: Comment #N

Problem/Motivation

Emitting menu links will soon require a full route object to check for access.
This requires single loading of route objects all over the place, which is potentially a DB query at the moment.

Proposed resolution

This patch efficiently preloads non-admin routes in order to speed up menu link display.

Remaining tasks

User interface changes

API changes

Original report by @dawehner

Files: 
CommentFileSizeAuthor
#59 interdiff.txt1.97 KBdawehner
#59 route_rebuild-2058845-59.patch11.43 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,701 pass(es).
[ View ]
#56 interdiff.txt2.34 KBdawehner
#56 route_rebuild-2058845-56.patch11.16 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 64,417 pass(es).
[ View ]
#44 route_rebuild-2058845.patch11.37 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 61,995 pass(es), 92 fail(s), and 89 exception(s).
[ View ]
#44 interdiff.txt812 bytesdawehner
#42 interdiff.txt19.98 KBmsonnabaum
#42 route_rebuild-2058845-42.patch10.32 KBmsonnabaum
PASSED: [[SimpleTest]]: [MySQL] 63,426 pass(es).
[ View ]
#38 route_preload_xhprof.png124.82 KBmsonnabaum
#37 Screenshot 2014-02-04 23.11.57.png124.82 KBmsonnabaum
#35 route_rebuild-2058845-35.patch10.97 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 63,707 pass(es).
[ View ]
#35 interdiff.txt1.93 KBdawehner
#33 2058845-route_rebuild-32.patch10.83 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 63,258 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#29 interdiff.txt2.83 KBdawehner
#29 2058845-preload_routes-29.patch10.37 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#16 routes-2058845.patch10.08 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 61,743 pass(es), 1,056 fail(s), and 20,932 exception(s).
[ View ]
#16 routes-2058845.patch10.08 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,997 pass(es).
[ View ]
#12 preload-2058845.patch4.5 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,058 pass(es).
[ View ]
#12 interdiff.txt2.17 KBdawehner
#10 preload-2058845.patch4.35 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,047 pass(es).
[ View ]
#4 breadcrumb-2058845-4.patch4.66 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#4 interdiff.txt780 bytesdawehner
#2 breadcrumb-2058845-1.patch3.9 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Comments

catch’s picture

Title:Potential issue: Preload all routes of a menu tree» Look at pre-loading common routes
Component:simpletest.module» routing system
Priority:Normal» Major
Issue tags:+Performance

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.

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new3.9 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

This loads the routes on breadcrumbs.

Status:Needs review» Needs work

The last submitted patch, breadcrumb-2058845-1.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new780 bytes
new4.66 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Missing services change.

Status:Needs review» Needs work

The last submitted patch, breadcrumb-2058845-4.patch, failed testing.

catch’s picture

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.

dawehner’s picture

Status:Needs work» Needs review
Issue tags:-Performance, -MenuSystemRevamp, -WSCCI

#4: breadcrumb-2058845-4.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, breadcrumb-2058845-4.patch, failed testing.

Cottser’s picture

Issue summary:View changes
Issue tags:+Needs reroll

Tagging for reroll.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new4.35 KB
PASSED: [[SimpleTest]]: [MySQL] 59,047 pass(es).
[ View ]

This tries to implement the /admin logic in a preloader.

tim.plunkett’s picture

+++ 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.

dawehner’s picture

StatusFileSize
new2.17 KB
new4.5 KB
PASSED: [[SimpleTest]]: [MySQL] 59,058 pass(es).
[ View ]

Yeah right.

Crell’s picture

  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?

Cottser’s picture

Issue tags:-Needs reroll
dawehner’s picture

Issue tags:+Performance, +WSCCI, +phpunit

There we go, this time also with a proper test.

dawehner’s picture

StatusFileSize
new10.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,997 pass(es).
[ View ]
new10.08 KB
FAILED: [[SimpleTest]]: [MySQL] 61,743 pass(es), 1,056 fail(s), and 20,932 exception(s).
[ View ]

There we go, this time also with a proper test.

Crell’s picture

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

dawehner’s picture

Let me quote my UI diff tool: The files are identical.

moshe weitzman’s picture

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?

moshe weitzman’s picture

Title:Look at pre-loading common routes» Pre-load non-admin routes
Issue summary:View changes
Issue tags:+Needs benchmarks
dawehner’s picture

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

I would love to, though I am not aware of a way yet, as we don't have for example single block requests yet.

Crell’s picture

+++ 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. :-)

catch’s picture

xhprof for the whole request works great.

dawehner’s picture

16: routes-2058845.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 16: routes-2058845.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

16: routes-2058845.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 16: routes-2058845.patch, failed testing.

tstoeckler’s picture

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

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new10.37 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new2.83 KB

Thank you for your review, great points!

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

The patch adds some basic description of the idea onto the class documentation. Does that help?

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.

Great idea. It is always great to see that new features can be used in more places than the itinitial usecase.

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.

I totally agree that we should always document why a specific priority is chosen. Here we can just drop the line of documentation as previous versions of the patch just moved it from VIEW to REQUEST as menu links could also be rendered on the actual controller, so using the VIEW event is too late.

Status:Needs review» Needs work

The last submitted patch, 29: 2058845-preload_routes-29.patch, failed testing.

The last submitted patch, 29: 2058845-preload_routes-29.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

Sadly I totally misunderstood your suggestion with the used event.

dawehner’s picture

StatusFileSize
new10.83 KB
FAILED: [[SimpleTest]]: [MySQL] 63,258 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

missing attachment.

Status:Needs review» Needs work

The last submitted patch, 33: 2058845-route_rebuild-32.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.93 KB
new10.97 KB
PASSED: [[SimpleTest]]: [MySQL] 63,707 pass(es).
[ View ]

Fixed also the unit test

pwolanin’s picture

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.

msonnabaum’s picture

Issue tags:-Needs benchmarks
StatusFileSize
new124.82 KB

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.

msonnabaum’s picture

StatusFileSize
new124.82 KB

Let's try this image again:

catch’s picture

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

tim.plunkett’s picture

sun’s picture

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

msonnabaum’s picture

StatusFileSize
new10.32 KB
PASSED: [[SimpleTest]]: [MySQL] 63,426 pass(es).
[ View ]
new19.98 KB

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.

dawehner’s picture

My reason for checking for _content was to not load routes like the REST ones, generic AJAX controllers etc. I think checking for _content and or _form, _entity_list, _entity_form or _entity_view should cover all we have in core.

dawehner’s picture

StatusFileSize
new812 bytes
new11.37 KB
FAILED: [[SimpleTest]]: [MySQL] 61,995 pass(es), 92 fail(s), and 89 exception(s).
[ View ]

I totally like the new naming, let's go with it.

On the other hand I am not convinced of the blacklisting approach instead of the whitelisting we have at the moment. This adds back _content
but expands it with _form, _entity_form, _entity_view and last allow to define routes to be preloaded.

tim.plunkett’s picture

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

Status:Needs review» Needs work

The last submitted patch, 44: route_rebuild-2058845.patch, failed testing.

msonnabaum’s picture

Status:Needs work» Needs review
StatusFileSize
new918 bytes
new11.37 KB
FAILED: [[SimpleTest]]: [MySQL] 60,695 pass(es), 205 fail(s), and 115 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 47: route_rebuild-2058845-47.patch, failed testing.

dawehner’s picture

Here is a really simple controller:

<?php
    $before
= memory_get_peak_usage(TRUE);
   
$route_provider = \Drupal::service('router.route_provider');
   
$lazy_collection = $route_provider->getAllRoutes();
   
$routes = array();
   
$route_names = array();
   
$admin_names = array();
    foreach (
$lazy_collection as $name => $route) {
     
$routes[] = $route;
     
$route_names[] = $name;
      if (
strpos($route->getPath(), '/admin') === 0) {
       
$admin_names[] = $name;
      }
    }
   
$after = memory_get_peak_usage(TRUE);
   
debug($before);
   
debug($after - $before);
   
debug(count($route_names));
   
debug(count($admin_names));
?>

This was executed on the standard profile, so if you install contrib modules the amount
of routes would easily get at least times 2 or 3.

The result was

6029312
1310720
346
261

so this feature would add 4 MB of ram to every request, in actual real world scenarios over 10 MB.

catch’s picture

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.

msonnabaum’s picture

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?

msonnabaum’s picture

Status:Needs work» Needs review
StatusFileSize
new10.32 KB
PASSED: [[SimpleTest]]: [MySQL] 64,358 pass(es).
[ View ]

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.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

We have green, and a consensus. RTBC

moshe weitzman’s picture

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

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

#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

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new11.16 KB
PASSED: [[SimpleTest]]: [MySQL] 64,417 pass(es).
[ View ]
new2.34 KB

Good catch!

sun’s picture

Status:Needs review» Reviewed & tested by the community

Thanks!

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

@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.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new11.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,701 pass(es).
[ View ]
new1.97 KB

Okay.

beejeebus’s picture

i love this patch. preloading++

having said that, i wonder if we should make RoutePreloader just mechanism, and allow the policy to live in event subscribers to a 'route.preload.names' event or similar.

core would ship a listener that implemented the non-admin route preloading thing, and contrib can go nuts. like config object preloading, i think we should take care to avoid one-size-fits-all, and make sure core doesn't get in the way. leaving as needs review because i'm late to the party, and i can live with this coming in a follow up.

if we do make this a follow up, then we should rename RoutePreloader to NonAdminRoutePreloader or something, to make it clear we've put the policy as well as the mechanism in this class.

dawehner’s picture

having said that, i wonder if we should make RoutePreloader just mechanism, and allow the policy to live in event subscribers to a 'route.preload.names' event or similar.

While I totally appreciate the idea I struggle really how we want to implement it. You certainly need somehow access to the available routes during this event, so should we pass the full route collection onto this custom event.
Alternative all the event subscribers could use both a routing event and the preloading event and use both to set the names properly.

Wim Leers’s picture

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

beejeebus’s picture

Status:Needs review» Reviewed & tested by the community

my comments weren't meant to block this going in as is. we can address my point in a follow up.

dawehner’s picture

Issue summary:View changes

Great, thank you!

Given how simple the actual code is and given how few people will actually tweak this service I really think it would be fine for them to replace the full class as it is.

Crell’s picture

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.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.