Problem/Motivation

Route preloading helps to avoid DB queries, but it comes with the cost to unserialize more than actually needed.

Proposed resolution

Still load the preloaded routes, but just unserialize on the fly.

Remaining tasks

User interface changes

API changes

API addition - new interface + method

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it's refactoring functions for performance
Issue priority Major because it improves performance for non-cached pages. Not critical because it's a modest improvement and not a release blocker since the changes are non-breaking.
Prioritized changes The main goal of this issue is performance
CommentFileSizeAuthor
#106 crappy-interdiff.txt751 bytesaspilicious
#104 2381505-104.patch13.63 KBaspilicious
#99 2381505-98.patch13.57 KBdawehner
#99 interdiff.txt960 bytesdawehner
#94 increment.txt1.37 KBpwolanin
#94 2381505-94.patch13.58 KBpwolanin
#75 increment.txt2.42 KBpwolanin
#75 2381505-75.patch13.17 KBpwolanin
#68 increment.txt1.15 KBpwolanin
#68 2381505-68.patch13.06 KBpwolanin
#65 interdiff.txt778 bytesdawehner
#65 2381505-65.patch13.07 KBdawehner
#63 interdiff.txt2.18 KBdawehner
#63 2381505-63.patch13.07 KBdawehner
#60 increment-57-60.txt703 bytespwolanin
#60 2381505-60.patch13.03 KBpwolanin
#57 increment-53-57.txt4.25 KBpwolanin
#57 2381505-57.patch13.06 KBpwolanin
#53 interdiff.txt2.17 KBdawehner
#53 2381505-53.patch11.55 KBdawehner
#51 interdiff.txt1.08 KBdawehner
#51 2381505-51.patch11.63 KBdawehner
#47 2381505-47.patch12.02 KBrteijeiro
#37 increment-34-37.patch1.86 KBpwolanin
#37 2381505-37.patch12.03 KBpwolanin
#36 increment-31-34.txt5.2 KBpwolanin
#34 increment-31-34.txt8.6 KBpwolanin
#34 2381505-34.patch11.95 KBpwolanin
#31 increment-26-31.txt5.47 KBpwolanin
#31 2381505-31.patch10.57 KBpwolanin
#26 2381505-26.patch10.69 KBdawehner
#24 interdiff.txt6.36 KBdawehner
#24 2381505-24.patch10.68 KBdawehner
#22 interdiff.txt617 bytesdawehner
#22 2381505-22.patch6.4 KBdawehner
#20 2381505-20.patch6.4 KBdawehner
#20 interdiff.txt583 bytesdawehner
#18 2381505-18.patch6.4 KBdawehner
#18 interdiff.txt676 bytesdawehner
#16 interdiff.txt1.47 KBdawehner
#16 2381505-16.patch5.74 KBdawehner
#8 interdiff.txt6.27 KBdawehner
#8 2381505-8.patch6.13 KBdawehner
#3 2381505-3.patch8.79 KBdawehner
#3 interdiff.txt836 bytesdawehner
#1 2381505-1.patch8.68 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
8.68 KB

Let's upload the existing patch.

Status: Needs review » Needs work

The last submitted patch, 1: 2381505-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Performance
FileSize
836 bytes
8.79 KB

Let's fix it.

dawehner’s picture

Issue tags: +needs profiling

Good profiling criterias would be a similar page like #2380417: Bootstrap flame graph

Damien Tournoud’s picture

It's worse than you think: given the way Drupal\Core\Routing\CompiledRoute is currently written, each route is deserialized twice: once in Symfony\Component\Routing\CompiledRoute::unserialize, once in Drupal\Core\Routing\CompiledRoute::unserialize.

And on serialization, it is serialized in the parent, deserialized and re-serialized...

Damien Tournoud’s picture

Also, I don't get why the preloader is storing the list of non-admin route in the state? Why not just adding a database column on the {router} table a populate it in MatcherDumper::dump()?

Damien Tournoud’s picture

+  /**
+   * {@inheritdoc}
+   */
+  public function get($name) {
+    if (isset($this->routes[$name])) {
+      if (is_string($this->routes[$name])) {
+        $this->routes[$name] = unserialize($this->routes[$name]);
+      }
+      return $this->routes[$name];
+    }
+
+    return NULL;
+  }

As much as possible, you should call the provider here. Or else you risk deserializing the same route multiple time.

(And it's really sad that you have to override all the other methods, but that's Symfony\Component\Routing\RouteCollection's fault.)

dawehner’s picture

FileSize
6.13 KB
6.27 KB

Thank you for your great comments tonight!

It's worse than you think: given the way Drupal\Core\Routing\CompiledRoute is currently written, each route is deserialized twice: once in Symfony\Component\Routing\CompiledRoute::unserialize, once in Drupal\Core\Routing\CompiledRoute::unserialize.

Okay, that is a clear bug, let's fix it.

And on serialization, it is serialized in the parent, deserialized and re-serialized...

Right, we can fix that as well, but its makes the code a bit more ugly, if you ask me.

Also, I don't get why the preloader is storing the list of non-admin route in the state? Why not just adding a database column on the {router} table a populate it in MatcherDumper::dump()?

I don't care, using state seemed to be the most flexible solution in case contrib has better ideas.

(And it's really sad that you have to override all the other methods, but that's Symfony\Component\Routing\RouteCollection's fault.)

At least for that usecase I realized we don't need to subclass the RouteCollection BS, let's just implement that on the RouteProvider level and be done.

Status: Needs review » Needs work

The last submitted patch, 8: 2381505-8.patch, failed testing.

catch’s picture

I don't care, using state seemed to be the most flexible solution in case contrib has better ideas.

Yes this was the original reason and it still seems sensible to me, if someone comes up with a better contrib optimization they should be able to completely remove this one from running at all.

Damien Tournoud’s picture

@catch: but really they should do that by swapping out the route preloading service, no?

Having the default MatcherDumper add this information to the database doesn't really hurt.

Damien Tournoud’s picture

The current implementation will not scale, and from the look of it, it is already pretty inefficient even with core alone.

catch’s picture

@Damien I'm also thinking of things like altering all HTML admin routes away completely then using a PHP dumper for everything else. So no pre-loading at all then. In that case the route pre-loading could be replaced with a null implementation and the dumper would be different anyway, so even for this keeping it in the db seems OK indeed.

Another thing that would help with simple routes that don't need to generate links, or links that do but where there's a 100% render cache hit, would be to only pre-load the routes when generating a link for the first time. No point pre-loading something that'll never get accessed. Probably separate issue for that.

moshe weitzman’s picture

Using a db column instead of state would not help with our goal of a speedup via "unserialize on the fly". Lets consider that out of scope for this issue.

Hopefully someone can debug the test fail on the most recent patch.

moshe weitzman’s picture

Using a db column instead of state would not help with our goal of a speedup via "unserialize on the fly". Lets consider that out of scope for this issue.

Hopefully someone can debug the test fail on the most recent patch.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.74 KB
1.47 KB

Alright, so this time it looks a bit better.

Status: Needs review » Needs work

The last submitted patch, 16: 2381505-16.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
676 bytes
6.4 KB

Maybe that is all, what is needed.

moshe weitzman’s picture

Are we really unserializing on the fly here? I see that loadNonAdminRoutes() passes TRUE as second argument to getRoutesByNames().

I can do a flame graph with and without this change if you think that would be helpful.

dawehner’s picture

FileSize
583 bytes
6.4 KB

Ehem, you are absolute right, my bad.

Status: Needs review » Needs work

The last submitted patch, 20: 2381505-20.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.4 KB
617 bytes

Maybe the filewidget test was just a random failure.

Status: Needs review » Needs work

The last submitted patch, 22: 2381505-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.68 KB
6.36 KB

Maybe, just maybe, this helps.

Status: Needs review » Needs work

The last submitted patch, 24: 2381505-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.69 KB

First just a reroll.

Status: Needs review » Needs work

The last submitted patch, 26: 2381505-26.patch, failed testing.

pwolanin’s picture

Patch still applies cleanly, but I'm not seeing the fail locally.

Status: Needs work » Needs review

pwolanin queued 26: 2381505-26.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 26: 2381505-26.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
10.57 KB
5.47 KB

I'm not sure it makes sense to modify the interface - why not just add a method that does the desired pre-loading?

It also looks like there is a bug in the case you needed to rebuild routes.

Really not able to reproduce the test bot fail locally, even after discussion with dawehner.

I get fails when putting index.php into the Drupal URL, but I get the same fails on 8.0.x.

Status: Needs review » Needs work

The last submitted patch, 31: 2381505-31.patch, failed testing.

pwolanin’s picture

I spun up a testbot myself - looks like a PHP segfault

 [Sun Mar 15 21:33:54 2015] [notice] child pid 28983 exit signal Segmentation fault (11)

Very weird, but let's try changing the code a little.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
11.95 KB
8.6 KB

Wow - this is just horrible.

Even with my own testbot, debugging this was just trial and error.

If I disable APC, instead of a segfault I get a SQL fatal due to duplicate uuid generation. WTF

It seems all is solved by putting back the call to parent::serialize(); in CompiledRoute::serialize(). So clearly some weird PHP bug in that version of 5.4 that expects you to call serialize up the full chain of inheritance.

pwolanin’s picture

And being PHP - it seems in other cases calling parent::serialize() causes the segfault: https://bugs.php.net/bug.php?id=65591

pwolanin’s picture

FileSize
5.2 KB

Oops the last interdiff was wrong - here's the correct one.

pwolanin’s picture

FileSize
12.03 KB
1.86 KB

Updated comment

Status: Needs review » Needs work

The last submitted patch, 37: increment-34-37.patch, failed testing.

Berdir’s picture

FYI: Field API had segfaults with the Serializable interface for quite some time and switched to use __sleep()/__wakeup(): #2074253: Fatal error when using Serializable interface on PHP 5.4

pwolanin’s picture

Status: Needs work » Needs review

Since the upstream symfony class implements the serialize method, and I don't think __sleep() actually allows you to decide which data to serialize, I' not sure moving to __sleep() is a viable option

dawehner’s picture

Great work @pwolanin!!

I think the problem mentioned in #2074253: Fatal error when using Serializable interface on PHP 5.4 is not a problem here, we don't have objects referencing each other etc.
The Route object is some kind of simple object, just containing data.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
@@ -156,19 +146,9 @@ public function getRequirements() {
-    $data = [];
-    // All symfony provided variables.
-    // @todo A lot of them are actually not used by Drupal, at least for now,
-    //   so think about optimize them out.
-    $data['vars'] = $this->getVariables();
-    $data['path_prefix'] = $this->getStaticPrefix();
-    $data['path_regex'] = $this->getRegex();
-    $data['path_tokens'] = $this->getTokens();
-    $data['path_vars'] = $this->getPathVariables();
-    $data['host_regex'] = $this->getHostRegex();
-    $data['host_tokens'] = $this->getHostTokens();
-    $data['host_vars'] = $this->getHostVariables();
-
+    // At least some versions of PHP 5.4 + APC segfault if this call to the
+    // parent method is missing, so we cannot optimize by inlining that code.
+    $data = unserialize(parent::serialize());

We don't callserialize() often anyway, so there is actually no point in optimizing that call to parent.

pwolanin’s picture

Yes, but in any case - clearly serialize is dangerous territory

dawehner’s picture

@pwolanin
Do we need something else on this issue?

pwolanin’s picture

I think it's RTBC. The only bit where I'm not 100% sure about the tradeoff is:

+        // Leave the variable set but free up the memory.
+        $this->serialized_routes[$name] = '';

But as long as the reset() method is used everything works as intended.

dawehner queued 37: 2381505-37.patch for re-testing.

The last submitted patch, 37: 2381505-37.patch, failed testing.

rteijeiro’s picture

FileSize
12.02 KB

Re-rolled!

Status: Needs review » Needs work

The last submitted patch, 47: 2381505-47.patch, failed testing.

Status: Needs work » Needs review

rteijeiro queued 47: 2381505-47.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 47: 2381505-47.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.63 KB
1.08 KB

Just fixing some stuff.

Status: Needs review » Needs work

The last submitted patch, 51: 2381505-51.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.55 KB
2.17 KB

There we go.

Crell’s picture

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
@@ -177,6 +146,10 @@ public function getRequirements() {
+    // At least some versions of PHP 5.4 (e.g. used by the testbot) segfault
+    // if this call to the parent method is missing. However, in other cases,
+    // calling the parent method is known to cause a segfault.
+    // https://bugs.php.net/bug.php?id=65591
     $data = unserialize(parent::serialize());

So wait, so this will segfault in some versions, either way? It's just a question of which version we segfault on? That... sucks.

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -132,34 +139,34 @@ public function getRouteByName($name) {
+  public function preLoadRoutes($names) {

This method would need to be added to an interface. Remember, alternate implementations of RouteProvider already exist.

I'm open to this if it's a significant performance boost, but I am wary of moving preload awareness into the provider. The current preloader is a completely transparent zero-API add-on. This moves it partially into the Provider, which feels like mixing concerns. Could we perhaps better abstract the operations involved, eg, so that the Provider doesn't need to know that cache preloading is the task at hand? Something like fetchRoute() (gets serialized version), decodeRoute() (turns serialized version into loaded version) and loadRoute() (gets serialized version and decodes it)? (I'm just making up names but hopefully you get the idea.)

dawehner’s picture

Yeah its certainly not a bad idea to give these things a better name.

pwolanin’s picture

@Crell - it seems calling parent::serialize() is the less risky of the 2 choices here. The bug example had to set up a complex nesting to get that to cause the segfault.

pwolanin’s picture

FileSize
13.06 KB
4.25 KB

In either case we are assuming that the route provider has an internal cache. Is that a useful assumption?

We could do something like this, though I'm not sure it makes senes to extend the interface another level.

We could also just check that it's a Drupal\Core\Routing\RoutProviderInterface

dawehner’s picture

+++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
@@ -74,7 +74,13 @@ public function onRequest(KernelEvent $event) {
+      if (is_subclass_of($this->routeProvider, '\Drupal\Core\Routing\PreloadRouteProviderInterface')) {

Why do you not just use $this->routeProvider instanceof PreloadRouteProviderInterface

pwolanin’s picture

oh, I guess that works too - I just recall using is_subclass_of() for some of the plugin code.

pwolanin’s picture

FileSize
13.03 KB
703 bytes
dawehner’s picture

oh, I guess that works too - I just recall using is_subclass_of() for some of the plugin code.

Yeah, you need that if you don't have an actual instance of those objects.

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
    @@ -177,6 +146,10 @@ public function getRequirements() {
       public function serialize() {
    +    // At least some versions of PHP 5.4 (e.g. used by the testbot) segfault
    +    // if this call to the parent method is missing. However, in other cases,
    +    // calling the parent method is known to cause a segfault.
    +    // https://bugs.php.net/bug.php?id=65591
         $data = unserialize(parent::serialize());
    

    This comment still confuses me. It reads as "this will sometimes segfault, sometimes not", which is not confidence-inducing. WHY did we decide that doing this was the better option? (Don't tell me; update the patch.)

  2. +++ b/core/lib/Drupal/Core/Routing/PreloadRouteProviderInterface.php
    @@ -0,0 +1,27 @@
    +interface PreloadRouteProviderInterface extends RouteProviderInterface {
    

    PreloadableRouteProviderInterface?

  3. +++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
    @@ -25,7 +25,7 @@ class RoutePreloader implements EventSubscriberInterface {
    -   * @var \Drupal\Core\Routing\RouteProviderInterface
    +   * @var \Drupal\Core\Routing\RouteProviderInterface|\Drupal\Core\Routing\PreloadRouteProviderInterface
    

    Unnecessary. It's always RouteProviderInterface. "Or a subclass thereof" is implicit in class/interface type checks.

  4. +++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
    @@ -74,7 +74,13 @@ public function onRequest(KernelEvent $event) {
       protected function loadNonAdminRoutes() {
         if ($routes = $this->state->get('routing.non_admin_routes', array())) {
    -      $this->routeProvider->getRoutesByNames($routes);
    +      if ($this->routeProvider instanceof PreloadRouteProviderInterface) {
    +        $this->routeProvider->preLoadRoutes($routes);
    +      }
    +      else {
    +        // Fall back to simply loading the routes to warm any caches.
    +        $this->routeProvider->getRoutesByNames($routes);
    +      }
         }
    

    This I like.

Much better this time around!

dawehner’s picture

FileSize
13.07 KB
2.18 KB

This comment still confuses me. It reads as "this will sometimes segfault, sometimes not", which is not confidence-inducing. WHY did we decide that doing this was the better option? (Don't tell me; update the patch.)

Well, that is just reality, PHP is interesting, see http://3v4l.org/QFDDc especially. We don't change behaviour here, but its good that we document it, for now.
Note: In order to solve it in a sane way, we would need upstream to use protected variables, but they don't, which well, some of us strongly disagree:

https://github.com/symfony/symfony/pull/14282

Unnecessary. It's always RouteProviderInterface. "Or a subclass thereof" is implicit in class/interface type checks.

Well, I think adding the two possible candidates indicates clearly that some other parts of the code has different behaviour depending on the more specific interface.

Status: Needs review » Needs work

The last submitted patch, 63: 2381505-63.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.07 KB
778 bytes

Let's fix it.

Crell’s picture

Status: Needs review » Needs work

The comment is still unacceptably vague. Looking at the code, I don't know why we're doing something that is called out as dangerous.

dawehner’s picture

The comment is still unacceptably vague. Looking at the code, I don't know why we're doing something that is called out as dangerous.

... because its PHP, well, what about just leaving out the entire documentation, given that its all what we are doing there.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
13.06 KB
1.15 KB

Ok, how about just a little note so people don't try to optimize it out? Also fixes doxygen.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -131,34 +138,34 @@ public function getRouteByName($name) {
+        // Leave the variable set but free up the memory.
+        $this->serializedRoutes[$name] = '';

Is there a reason to leave the variable set?

--

Except this question, the patch looks RTBC for me.

Can we get some profiling numbers, please?

pwolanin’s picture

@Fabianx - yes, there is an isset() check to avoid loading it again.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -131,34 +138,34 @@ public function getRouteByName($name) {
+      if (!isset($this->routes[$name]) && isset($this->serializedRoutes[$name])) {
+        $this->routes[$name] = unserialize($this->serializedRoutes[$name]);
+        // Leave the variable set but free up the memory.
+        $this->serializedRoutes[$name] = '';
       }

I see that isset() and assume it is this one, but won't always $this->routes[$name] be set then and we can never reach that condition?

klausi’s picture

Status: Needs review » Needs work

@Fabianx: the check is happening in preloadRoutes() with the array_diff(), so we need to keep it set. Maybe the comment should point that out.

  1. +++ b/core/lib/Drupal/Core/Routing/PreloadableRouteProviderInterface.php
    @@ -0,0 +1,27 @@
    +   * @param array $names
    +   *   The list of names to load.
    

    If this is just a list of strings it should use "string[]" instead of "array" as type.

  2. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
    @@ -53,6 +53,13 @@ class RouteProvider implements RouteProviderInterface, PagedRouteProviderInterfa
    +   * A cache of already-loaded serialized routes, keyed by route name.
    +   *
    +   * @var array
    

    Same here, should be something like "string[]" or whatever the data type of the array values is.

Fabianx’s picture

Thanks, klausi. That makes more sense now :).

pwolanin’s picture

ok, I'll fix up the comments in a bit

pwolanin’s picture

Status: Needs work » Needs review
FileSize
13.17 KB
2.42 KB

Actually, I think the code changed from the earlier version such that we don't need to keep it set.

Status: Needs review » Needs work

The last submitted patch, 75: 2381505-75.patch, failed testing.

pwolanin’s picture

Looks like it might be a testbot issue: "File /tmp/some-temp-file.jpg could not be copied to temporary://some-temp-file.jpg."

pwolanin queued 75: 2381505-75.patch for re-testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -145,8 +145,7 @@ public function preLoadRoutes($names) {
-    $routes_to_load = array_diff($names, array_keys($this->routes));
-    $routes_to_load = array_diff($routes_to_load, array_keys($this->serializedRoutes));
+    $routes_to_load = array_diff($names, array_keys($this->routes), array_keys($this->serializedRoutes));
     if ($routes_to_load) {

Oh nice, I wasn't aware that this is possible.

Fabianx’s picture

RTBC + 1

pwolanin’s picture

Issue tags: +D8 Accelerate Dev Days
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

alexpott’s picture

Also do we have any numbers on the perf benefits?

pwolanin’s picture

Priority: Normal » Major
Issue summary: View changes
pwolanin’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
Fabianx’s picture

Status: Needs work » Needs review

Still needs profiling, what would be a good test case?

Crell’s picture

Find/make a non-admin page that has no links on it, or extremely few. That would be the upper-bound; we'd still pre-fetch all non-admin routes, but now only deserialize one or two. Benchmark the before/after with this patch.

If it looks to make a difference, then this is a performance improvement done in an API-safe way, IMO. (That is my stance as routing maintainer.)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Here are the steps I've done:

  • Install drupal
  • Disable page cache
  • Profile using xhprof_kit:
    ### FINAL REPORT
    
    === 8.0.x..2381505 compared (552e48497efa2..552e574854901):
    
    ct  : 45,503|44,971|-532|-1.2%
    wt  : 98,447|95,959|-2,488|-2.5%
    mu  : 21,271,192|20,680,520|-590,672|-2.8%
    pmu : 21,425,416|20,834,760|-590,656|-2.8%
    
catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
@@ -74,7 +74,13 @@ public function onRequest(KernelEvent $event) {
   protected function loadNonAdminRoutes() {

This bit makes we wonder a bit.

We added this for pre-loading. So why not call it preloadNonAdminRoutes (or just update the docs) and skip the additional interface and code path?

There's no return from there, so it shouldn't make any practical difference.

Crell’s picture

Status: Needs work » Reviewed & tested by the community

catch: As currently written in this patch, if the RouteProvider supports preloading we preload (but don't deserialize) non-admin routes. If it does not, then we still pre-load (but spend the time deserializing) non-admin routes. That's still faster than doing nothing but not as fast as delaying the deserialization, too. That gives us options.

The main reason for the separate interface is noted in #54: We don't want to load too much awareness of preloading into all route providers. That's the wrong place for that knowledge to reside. Making it a separate, opt-in interface follows the Interface Segregation Principle (I in SOLID), and keeps RouteProviders conceptually independent of preloading. (Strictly speaking, the fact the preloaded routes are kept in serialized form is an internal implementation of the default RouteProvider, as it should be; alternate implementations may have better options they can leverage and we should let them.)

Fabianx’s picture

RTBC + 1, I agree with #90 the Interface gives us flexibility

catch’s picture

Status: Reviewed & tested by the community » Needs work

No this really isn't right:

if the RouteProvider supports preloading we preload (but don't deserialize) non-admin routes. If it does not, then we still pre-load (but spend the time deserializing) non-admin routes. That's

(emphasis added).

The interface isn't PreLoadAndUnserialize. If we don't pre-load we shouldn't pre-load at all, if we do pre-load we should pre-load. Whether we unserialize or not is not the distinction here - that's 100% an internal optimization and depends on the storage.

Crell’s picture

The alternative then wouldn't be to merge the interfaces, but to simply remove the else clause entirely. Ie, if a RouteProvider doesn't have the preload interface we don't do anything *at all*. I would accept that if catch would.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
13.58 KB
1.37 KB

Ok, simplified.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - if tests pass

alexpott’s picture

Assigned: Unassigned » catch

Assigning to catch since he's been involved with this one.

catch’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
@@ -63,18 +63,13 @@ public function __construct(RouteProviderInterface $route_provider, StateInterfa
+      if ($routes && $this->routeProvider instanceof PreloadableRouteProviderInterface) {
if ($event->getRequest()->getRequestFormat() == 'html' && $this->routeProvider instanceof PreloadableRouteProviderInterface)

Would save unnecessarily loading the state entry and simplify things a bit more.

Wim Leers’s picture

Issue tags: +Needs reroll
dawehner’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
960 bytes
13.57 KB

Ha, @crell mentioned the same point.

webchick’s picture

Assigned: Unassigned » catch

Back to catch

Fabianx’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
@@ -65,8 +65,7 @@ public function __construct(RouteProviderInterface $route_provider, StateInterfa
     if ($event->getRequest()->getRequestFormat() == 'html') {
-      $routes = $this->state->get('routing.non_admin_routes', []);
-      if ($routes && $this->routeProvider instanceof PreloadableRouteProviderInterface) {
+      if ($this->routeProvider instanceof PreloadableRouteProviderInterface && ($routes = $this->state->get('routing.non_admin_routes', []))) {

As this is called on every request and hence is in the critical path I take the liberty to nit on this:

Should be:

if ($this->routeProvider instanceof PreloadableRouteProviderInterface && $event->getRequest()->...)

so that we don't do anything if the interface is wrong.

catch’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
@@ -63,18 +63,12 @@ public function __construct(RouteProviderInterface $route_provider, StateInterfa
+      if ($this->routeProvider instanceof PreloadableRouteProviderInterface && ($routes = $this->state->get('routing.non_admin_routes', []))) {

Yes that also saves having to worry about precedence here if we move the check above.

aspilicious’s picture

I'm going to reroll this.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
13.63 KB
Wim Leers’s picture

#104: could you please provide an interdiff? Thanks!

aspilicious’s picture

FileSize
751 bytes

No I forgot, so I did a crappy file interdiff :(

   public function onRequest(KernelEvent $event) {
-    // Just preload on normal HTML pages, as they will display menu links.
-    if ($event->getRequest()->getRequestFormat() == 'html') {
-      $this->loadNonAdminRoutes();
-    }
-  }
-
-  /**
-   * Load all the non-admin routes at once.
-   */
-  protected function loadNonAdminRoutes() {
-    if ($routes = $this->state->get('routing.non_admin_routes', array())) {
-      $this->routeProvider->getRoutesByNames($routes);
+    // Only preload on normal HTML pages, as they will display menu links.
+    if ($this->routeProvider instanceof PreloadableRouteProviderInterface && $event->getRequest()->getRequestFormat() == 'html') {
+      if ($routes = $this->state->get('routing.non_admin_routes', [])) {
+        // Preload all the non-admin routes at once.
+        $this->routeProvider->preLoadRoutes($routes);
+      }
Crell’s picture

If we really really want the best performance, we can write a compiler pass that detects the interface of the route provider service and only registers the pre-loader listener if it has the right interface. That may be excessive, though. :-)

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

I'm assigning back to catch, so he can decide

Fabianx’s picture

RTBC + 1

catch’s picture

The instanceof check is extremely cheap if we're actually implementing it, and the nitpicking here is as much style (i.e. not having assignments in the second part of an if statement) as optimization. So yes compiler pass seems excessive. There's something to be said for not loading the interface if no runtime code uses it, but since we think it'll be implemented 99.9% of the time that's not worth worrying about.

Committed/pushed to 8.0.x, thanks!

  • catch committed a9f9c03 on 8.0.x
    Issue #2381505 by dawehner, pwolanin, aspilicious, rteijeiro:...
aspilicious’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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