Cache incoming path processing and route matching

How much we can do also depends on #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use

+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -86,7 +95,23 @@ public function getContext() {
+      $cid = 'route_match:' . $path . ':' . $format;
+      if ($cached = $this->cacheBackend->get($cid)) {

This might be problematic because we have \Drupal\Core\Routing\ContentTypeHeaderMatcher Also i'm curious whether its really a good idea to inject the cache at this place. For upcasted entities for example we would store the entire entity in there. ... This would also make life harder for accept header based routing, or rather remove support for it for now.

Architecturally, I'd prefer to see the caching done in a wrapping object, rather than injected directly. It keeps the concerns separated better.

That said, swapping 2 queries for 2 queries doesn't seem like a win. For this to really have an impact, I'd suspect we need to bypass the path alias look up as well, or work that into a single query or something. At that point, though, we lose a lot of the modular flexibility of the system. I think the approach in #2471234: Create a ChainedRouteProvider which subclasses RouteProvider but could return early for most common routes is going to have more promise, but we'll of course need benchmarks to be sure.

#11: Use chained fast backend for now, that will give the same performance as having memcache.

--

More precise: Use chained fast backend for now _just for benchmarking_, that will give the same performance as having memcache.

This issue is somehow coupled to what we do in #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use

• In case we rely just on accept headers, we need to vary our cached entries by format
• In case we use extensions things are fine, the URL varies enough already
• In case we use query parameters things are fine, the URL varies enough already

: Class 'Drupal\Core\Path\InboundPathEvent' not found in d8/core/lib/Drupal/Core/Routing/RouteProvider.php on line 143

+++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
@@ -88,7 +87,7 @@ public function onKernelTerminate(PostResponseEvent $event) {
+    $events['incoming_path'][] = array('onIncomingPathConvertPath', 200);

+1 to have a dedicated event for that!

Let's see why it fails

I had a look at one of the test failures: DownloadTest, that one was reproducable on the local system.

We support "clean" URLs for accessing private files. As the routing system don't support arbitrary placeholders, we rewrite /system/files/foo/bar/baz to /system/files?file=foo/bar/baz using inbound path processing, see \Drupal\system\PathProcessor\PathProcessorFiles

This itself works fine, but in case path processing is not executed, we loose the added query parameter in the cache HIT.

meh, you forget the new file again ...

Here is a fix for the breadcrumb case + the missing file

Thanks for the breadcrumb fix - were you able to reproduce locally yourself?

Yes, used a subdir installation for that. Its so valuable that I have d8.dev and localhost/d8 pointing to the same installation.

@catch
Do you think we need some dedicated test coverage, or is the passive test coverage good enough?

1. +++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
   @@ -88,7 +87,7 @@ public function onKernelTerminate(PostResponseEvent $event) {
   +    $events['incoming_path'][] = array('onIncomingPathConvertPath', 200);
   

   If we're going to make a new event for this, it should be at least namespaced, preferably its own constant (to be consistent with elsewhere).

2. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -111,9 +134,22 @@ public function __construct(Connection $connection, StateInterface $state, Curre
   +      $event = new InboundPathEvent($request);
   +      $this->eventDispatcher->dispatch('incoming_path', $event);
   +      $path = $this->currentPath->getPath($request);
   +      // Incoming path processors may also set query parameters.
   +      $parameters = $request->query->all();
   +      $routes = $this->getRoutesByPath(rtrim($path, '/'));
   +      $this->cache->set($cid, [$path, $parameters, $routes], CacheBackendInterface::CACHE_PERMANENT, ['route_match']);
   +      return $routes;
   

   Talking with dawehner a bit in IRC, I'm not convinced an event is the right connection point here. Recall that

   1) Events are the most expensive extension point tool we have.

   2) There are other route providers already implemented, and we want to make it easy for people to experiment. That means as little implicit logic as possible.

   If we want to move alias lookup inside the provider, it should be an explicit action. Either a direct dependency, or a direct dependency with another interface (to indicate that the provider will handle aliasing, etc.), or something like that.

   That also begs the question of whether all inbound path processing should move inside the provider, and what the implications are for other pre-routing request listeners; they will not have the unaliased route available.

   Thoughts?

3. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -111,9 +134,22 @@ public function __construct(Connection $connection, StateInterface $state, Curre
   diff --git a/core/modules/system/src/EventSubscriber/ThemeSettingsCacheTag.php b/core/modules/system/src/EventSubscriber/ConfigCacheTag.php
   
   diff --git a/core/modules/system/src/EventSubscriber/ThemeSettingsCacheTag.php b/core/modules/system/src/EventSubscriber/ConfigCacheTag.php
   similarity index 73%
   
   similarity index 73%
   rename from core/modules/system/src/EventSubscriber/ThemeSettingsCacheTag.php
   
   rename from core/modules/system/src/EventSubscriber/ThemeSettingsCacheTag.php
   rename to core/modules/system/src/EventSubscriber/ConfigCacheTag.php
   
   rename to core/modules/system/src/EventSubscriber/ConfigCacheTag.php
   index eb68d75..1ece46d 100644
   
   index eb68d75..1ece46d 100644
   --- a/core/modules/system/src/EventSubscriber/ThemeSettingsCacheTag.php
   
   --- a/core/modules/system/src/EventSubscriber/ThemeSettingsCacheTag.php
   +++ b/core/modules/system/src/EventSubscriber/ConfigCacheTag.php
   
   +++ b/core/modules/system/src/EventSubscriber/ConfigCacheTag.php
   +++ b/core/modules/system/src/EventSubscriber/ConfigCacheTag.php
   @@ -2,7 +2,7 @@
   

   Is this just a Git quirk?

+++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
@@ -87,7 +71,7 @@ public function onKernelTerminate(PostResponseEvent $event) {
   static function getSubscribedEvents() {
-    $events['incoming_path'][] = array('onIncomingPathConvertPath', 200);
+    $events[KernelEvents::CONTROLLER][] = array('onKernelController', 200);
     $events[KernelEvents::TERMINATE][] = array('onKernelTerminate', 200);
     return $events;

Why CONTROLLER, rather than just a lower-priority REQUEST listener? We don't need the controller here; just a post-routing request.

Moving all of inbound path processing inside the routing system makes sense to me. Especially if at some point RouteProvider started getting its runtime input through the constructor (WAT?), fixing that seems like a good thing all on its own, caching aside.

Question: If we do move the logic inside RouteProvider, would it be possible to make the caching a wrapping class around the provider rather than integrated into it? That would be preferable as it is more loosely coupled, easier to test, and easier for other RouteProvider implementations to reuse.

Hm. OK, I've not seen the controller event used as a "post-routing event", but if Symfony's docs say that's kosher I'll run with it.

Keeping the cache in a wrapping object gives us the flexibility to try logic like that. (LRU, switching, etc.) At that point it just becomes a matter of twiddling the container and adding another interstitial object if we feel like it. There's a lot of flexibility there we can experiment with without API impact, assuming the wrapping cache isn't that complicated to implement. (It seems like it shouldn't be. Let's try.)

There you go :) The fix is stolen from Drupal\ckeditor\Tests\CKEditorTest::assertCKEditorLanguage().

Edit: btw, I could reproduce the fails when running the test in CLI.

When we talked about that on the WSCCI meeting crell favoured to put the caching into its own class wrapping the RouteProvider itself.

I'm personally not convinced that adding another layer makes things easier to understand

Changed some minor bits + added more specific test coverage ...

Gladly I'm not a maintainer of the routing system to decide whether we want to go with the wrapper approach or not, see #2480811-58: Cache incoming path processing and route matching

From #58:

There's a lot of flexibility there we can experiment with without API impact, assuming the wrapping cache isn't that complicated to implement. (It seems like it shouldn't be. Let's try.)

So I tried. Turns out, it is more complicated. Because the cache key depends on the path, and specifically the post-path-processing path, in order to form the cid in a wrapping cache class we would need to inject the path processor there as well, and run through it again. We'd also need to inject the current path service to the caching wrapper, too. At that point we don't seem to be gaining much anymore in terms of code cleanliness. (I might argue that means we need to think about it harder and refactor a bit more deeply, but given that we're in late beta I'll pass on that for now.)

However, while I was at it I realized that the cache structure is using meaningful positional keys. That totally doesn't fly from a DX perspective. So let's turn that into a named array at the very least so the code is more self-documenting.

It also looks like we expect path processors to be using paths without a leading /, although that's not documented. We should finally get around to standardizing those on a leading /, which would let us remove various trim and concat calls. Off topic for this issue, though, so I've filed a new one: #2508037: Clarify PathProcessor path format

+1 to use proper keys, it makes things at least a bit easier to grok.

Let me RTBC that, catch worked out most of it anyway

So I tried. Turns out, it is more complicated. Because the cache key depends on the path, and specifically the post-path-processing path, in order to form the cid in a wrapping cache class we would need to inject the path processor there as well, and run through it again. We'd also need to inject the current path service to the caching wrapper, too. At that point we don't seem to be gaining much anymore in terms of code cleanliness. (I might argue that means we need to think about it harder and refactor a bit more deeply, but given that we're in late beta I'll pass on that for now.)

Yeah and let's be fair, this is a true internal optimization, something you should actually not even care about, which is then fine if its not exposed as part of the interface / public classes.

It also looks like we expect path processors to be using paths without a leading /, although that's not documented. We should finally get around to standardizing those on a leading /, which would let us remove various trim and concat calls. Off topic for this issue, though, so I've filed a new one: #2508037: Clarify PathProcessor path format

Well right, you wanted to store paths with '/' in front, which I think is the way to go, but yeah we wanted to avoid changing too many APIs at once.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
   @@ -46,31 +38,24 @@ class PathSubscriber implements EventSubscriberInterface {
   +   *   THe alias manager.
   

   Nit: s/THe/The/

2. +++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
   @@ -88,7 +73,7 @@ public function onKernelTerminate(PostResponseEvent $event) {
   -    $events[KernelEvents::REQUEST][] = array('onKernelRequestConvertPath', 200);
   +    $events[KernelEvents::CONTROLLER][] = array('onKernelController', 200);
   

   This thoroughly confuses me:

   1. AccessAwareRouter is called during the REQUEST event.
   2. The IS says: Add caching to Drupal\Core\Routing\AccessAwareRouter::matchRequest()
   3. The CONTROLLER event is dispatched after the REQUEST event.
   4. Therefore, how does this work/help?

   Also, since AccessAwareRouter isn't touched at all, the IS seems outdated?

3. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -107,13 +129,31 @@ public function __construct(Connection $connection, StateInterface $state, Curre
   +    // Cache both the system path as well as route parameters and matching
   

   Do we still call this "system path"?

4. +++ b/core/modules/system/src/EventSubscriber/ConfigCacheTag.php
   @@ -47,12 +47,19 @@ public function __construct(ThemeHandlerInterface $theme_handler, CacheTagsInval
   +    // Changing the site settings may mean a change to the front page route.
   

   "may mean a change to the front page route" is ambiguous. What about "may cause a different route to become the front page route"?

5. +++ b/core/modules/system/src/Tests/Routing/RouteProviderTest.php
   @@ -50,13 +58,45 @@ class RouteProviderTest extends KernelTestBase {
   +  }
   +
   +
      protected function tearDown() {
   

   Nit: 2 newlines, should be 1.

6. Finally, shouldn't we do a round of profiling to know the actual gain here?

#73.4: much more understandable :) Thanks!

Convinced.

Wow that is quite an improvement. Was this the frontpage with cached data?

Concur on RTBC.

Nice patch only a couple of minor nits.

1. Discussed with @catch in IRC - we think this needs a change record since "now if you want to implement a route provider you have to take care of incoming path processing. And if you want to rely on current path you have to run after routing."

2. +++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
   @@ -46,31 +38,24 @@ class PathSubscriber implements EventSubscriberInterface {
   -   * @param Symfony\Component\HttpKernel\Event\GetResponseEvent $event
   -   *   The Event to process.
   ...
   +  public function onKernelController(FilterControllerEvent $event) {
   

   Removed the @param docs - but we still have a parameter.

3. +++ b/core/modules/system/src/Tests/Routing/RouteProviderTest.php
   @@ -7,9 +7,16 @@
   +use Drupal\url_alter_test\PathProcessor;
   

   Not used

Wrote the CR

Addressed the nitpicks.

The CR from @dawehner is here: https://www.drupal.org/node/2509216. What seems to be missing is the second part.. how that affects event listeners exactly. I have some module that might be affected by this.. e.g. globalredirect and redirect, wondering how they will be affected (Especially globalredirect.. that's doing things like comparing the path with the alias and redirects to the alias). So some more information on that would be very helpful.

Can also confirm what @catch said in #78. The wall time difference in that profiling isn't real. I'm also seeing fewer saved function calls, not sure why, likely different setup (standard with 2 languages and negotiation but I'd expect actually more savings there, not less)

In my profile run, we're saving ~1.3ms by not calling PathProcessorManager::processInbound() anymore, that includes language negotiation and 2 config loads. getRouteCollectionForRequest() is exactly as fast as before, even though the calls are completely different and as @catch said, it's a cache query that can be moved of to memcache/redis way more easily.

So given #83 and #84, I think this can move back to RTBC.

@berdir
Is https://www.drupal.org/node/2509216/revisions/view/8584240/8586258 what you wanted?

In my profile run, we're saving ~1.3ms by not calling PathProcessorManager::processInbound() anymore
I agree those numbers have been quite high, but you afaik also have a much lower baseline.

Committed 7145644 and pushed to 8.0.x. Thanks!

From the CR

you need to come after KernelEvents::REQUEST with priority 32.

I've been concerned for a while about the clustering of events with priorities around this number. I think currently there events where it would impossible to add an event in-between. I think it will be wroth opening an issue to assess all of core's events and their priorities.

#89: I agree, I am also concerned on the priorities being scattered throughout so many files.

With D7 module weight was at least pretty easy to understand, with events we probably need a handbook page that has all the priorities and order listed ...

---

Also: Yay!

+1 to that concern. At the very least, we should create more room between the weights. SmartCache in fact runs into this problem: it needs to sit at a place where there is no gap whatsoever!

Published the CR.

+1 on doing an audit of the kernel event listeners. I have opened an issue here: #2510738: Audit (and reorganize?) Kernel event listener priorities

@Crell++ for opening that follow-up!