Stop caching route lookups with query parameters or without results

Looking nice to me, but maybe moving the $cache_value into the if statement cause it's useless when we are not going to cache it?

      if ($routes->all() || $routes->getResources()) {
        $cache_value = [
          'path' => $path,
          'query' => $query_parameters,
          'routes' => $routes,
        ]; // Maybe move these lines into the if statement?
        $this->cache->set($cid, $cache_value, CacheBackendInterface::CACHE_PERMANENT, ['route_match']);
      }

Sending back to NW.

Removed needs reroll tag as #5 applies cleanly.

This feels like a bit of a sledgehammer fix since many of the cached "empty" routes could be belong to actual 404s (in which case you _want_ them to be cached for performance). The problem here isn't that the empty routes are being cached, but that they aren't being cached specifically enough.

The thing is that it's usually a module that's introducing the various context that makes a route accessible in one case and not another. In that case it's really up to the module to somehow inform the routing system about those contexts. Luckily core actually does have a way to do this with CacheableRouteProviderInterface::addExtraCacheKeyPart(). This was added in #3007669: Add publishing status to path aliases and allows you to add your own contexts to the route cache key.

An example of this is the Domain Access + Country Path module. Country path lets you toggle the active domain by path so:

- example.com/uk -> Domain #1
- example.com/us -> Domain #2

In this case when you see a link to example.com/us/whatever from example.com/uk on a cold cache, the empty route gets cached for all domains because the routing system doesn't know that there's a difference based if you're seeing the link on /us vs /uk. In this case Country path or even Domain access could inform the router about this difference. And in fact, there is actually a patch doing just that for Domain Access #3359253: Use core route provider with addExtraCacheKeyPart for route caching.

Really the hard part is usually figuring out specifically which module is introducing the contexts that cause this behaviour to happen. Ideally, the router would pull from the various render contexts that are floating around automatically, but I've only seen that eluded to in https://www.drupal.org/project/drupal/issues/2799543#comment-13344318

Rerolled the #5 patch

Re-roll patch #17 for 11.2.x

Re-rolled the patch for Drupal 11.3

I think it's reasonable to skip caching of 404 lookups, in general the hit rate on those is going to be low. The internal page cache and reverse proxies/CDNs can cache 404 pages (by path) which means they won't hit the route lookup a second time anyway.

I'm a bit confused by the issue.

I see the performance aspect. With security scanners and so on, there might be a huge amount of requests to 404 pages and caching them is a bit pointless, as they're also frequently invalidated and following requests are cached by internal page cache/CDN, so hit rate is low.

What I don't quite understand and would like to see clear steps to reproduce is the functional bugs. #16 mentions domain/country_path, that made sense, but the issue referenced there was fixed. And entity/upcasting related access checks is done later. This only caches the inbound processing and the raw route query. I don't see how that's possible.

In regards to performance:

I did some profiling (on umami, so including language prefixes, with blackfire). On a 404 page with warm caches but disabled page cache, RouteProvider::getRouteCollectionForRequest() is called twice and makes up 4% of the request (of 27ms, so about 1ms). That's with two calls, one for the main request and one for the 404-rendering sub-request. That 1ms is split into 50% for processInbound() (1 call), 25% 2 database cache lookups (one miss, one hit for the 404 page), the remaining 25% is reported as the function itself, but that must include the call to getRoutesByPath() and the query.

Testing a regular cache hit, on redis, the method doesn't even register in blackfire.

But, simulating a cache miss on an existing route with warm caches including dynamic page cache, so a get + lookup + cache set, essentially the worst-case but frequent scenario on active sites due to frequent cache wipes through alias updates:

database: total request 38ms, getRouteCollectionForRequest() is almost 20% of that (7.45ms). *but*. 82% of that time is the cache write, inbound processing is 11%, cache get and the actual lookup is around 3% each.
redis: total request is 28ms, getRouteCollectionForRequest() is only 4% of that, 60% of that is inbound processing, cache get 10%, write doesn't register (yay redis 2.x!)

I tried profiling it without any caching, but blackfire had some issues, so couldn't compare there, but with xhprof/xhgui, the method is about 1%/1ms. It does trigger two dynamic queries, they're very fast now as they're in mariadb query cache but on real sites with many different variations, they're probably more expensive.

Still, I remain unconvinced that this cache is really beneficial, with the bad hit ratio on active sites.

> What I don't quite understand and would like to see clear steps to reproduce is the functional bugs. #16 mentions domain/country_path, that made sense, but the issue referenced there was fixed. And entity/upcasting related access checks is done later. This only caches the inbound processing and the raw route query. I don't see how that's possible.

So there is one test fail that seems to indicate that the problem isn't routing but alias lookups. That can apparently get cached with not enough context (workspace), that doesn't really seem 404 related, so there might be more issues lurking there?

workspaces does set a routing cache key in \Drupal\workspaces\EventSubscriber\WorkspaceRequestSubscriber::onKernelRequest() and I don't really understand the test yet to understand if this is a bug being exposed or just a faulty test.

Just noting that this topic was also discussed over in #3580545: Make empty route lookup cacheable. I initially implemented proper cache context support for the 404 lookup there, but that is now not the direction the issue is going in. The code now just sits in an issue fork branch for posterity, and in case anyone wants to pick it up here.

That said and despite my disagreements with @Berdir in that issue, I'm not convinced the 404 caching makes too much sense either. On high performance sites ideally you would use some cache backend with a proper cache eviction policy, but with the default database backend it just seems like asking for cache pollution...

Re: #25 Re-ran the pipeline and it is all green. The failing tests seem to have been caused by a test runner failure. Seems they are unrelated.

Running the test-only code:

https://git.drupalcode.org/issue/drupal-3123401/-/jobs/9380425

LGTM.

I did an experimental merge request that removes caching completely to see where that lands us. See #23/#25 on thoughts and numbers.

One thing I realized when updating one performance tests is that at least in that test, we didn't actually any hits on the main request on that cache. But there were several removed cache gets and writes, which is actually secondary lookups, based on manual debugging, mostly due to Url::fromInternalPath() on link fields. Those attempt to resolve the URL to a route an match that.

that's one reason why the cache ID and logic around that is so complex.

I also updated the OpenTelemetryAuthenticatedPerformanceTest test, that's a bit a more honest scenario. As visible there, removing this cache results in two extra queries on a dynamic page cache hit, but only if we have a hit on the exact query string. Adding any kind of tracking URL for example would break that again and we're back to the same except we avoid the extra cache lookups and writes.

Can this one get a summary update please

I rebased, updated some more performance tests but not all.

I tried my best writing a new issue summary. It's long, it's a complicated issue with many possible alternatives.

The functional tests, also mentioned in the issue summary are quite interesting, PathWorkspacesTest and PathLanguageTest fail in ways that I can currently only assume are because they only worked due to cache hits that should not have been cache hits, maybe the tests are set up incorrectly or this is hiding bugs.

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Changing to a performance task.

Discussed a little bit with @catch, he mentioned a preference for only stopping to cache lookups with query strings and those with no results.

Adjusted for that, also made it easy for a subclass to disable completely and deprecated the helper method for that.

Updated the explicit caching tests and performance tests.

The standard login tests now result in not using the cache, this is because standard is now using the login form on the access denied page and that has special logic around the destination and includes it. this wouldn't happen for the first request in umami, but the redirect will include the cookie check.

We could try doing some more fancy, like ignore certain query parameters like destination that likely won't affect routing, but that will make it more complicated. or limit to only those. But that would make it more complicated. I also kind of expected routes with _format, such as ajax requests to pop up somewhere, but I haven't seen those yet, I guess we don't have explicit tests for those.

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Just trying to resolve conflicts; haven't played with the performance tests so might take a bit; will revert the merge commit if I get stuck

As discussed on slack, I rebased and updated the asserts, force pushed over your merge. Keeping at needs work as I want to apply the changes I discussed with @catch in slack.

Rebased again, that extra performance test is gone again.

Undeprecated the query method and created #3621551: Further route caching improvements

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.