Add cache wrapper to the UrlGenerator

I've made a bit of a start on this, will try to get a patch up at the weekend.

Can we also add a Drupal::generator() or something like that method here while we're at it.

And, let's not do something and something.cached as service names, because people end up using the wrong service. Maybe something and something.uncached , maybe the uncached one can even be a private service?

Ugh, sorry - I have been very distracted lately. Here are the beginnings of a patch - it totally breaks the installer and I haven't yet figured out what exactly is going wrong. Hardly seems worth setting to needs review, though I wouldn't mind getting some eyes on it to see if I'm at least going in the right direction with this as I'm not totally sure I've understood what's required.

Let's remove that and implement the single method which is needed (getPathFromRoute).

Found one bug while writing some tests: You also have to add the parameters to the key.

This fixes a couple of the failures.

Let's at least remove the old test code from urlgeneratorTest.

I have a feeling that at least some of the remaining test failures are caused by to strict caching. For example we don't take the language into account, but just putting it into the cacheKey did not helped.

Let's move it out #2057215: PathBasedGeneratorInterface should extend the base ones, so the proper generate method can also be used

Unassigning myself from some issues as I have very little time for core these days as I prepare to move 5000kms across the country...

#10: drupal-1965074-10.patch queued for re-testing.

Also needs work since PathBasedGeneratorInterface has been replaced.

Rerolled and added tests for generateFromRoute.

I can't fix the language based failures now.

So, I see a couple bugs in there ('system_path' -> '_system_path', and the wrong prefix for some entries) . Added a pass at language handling and simplified the code a bit.

Also, why don't we consider moving the in-memory caching and the clear() method to the Drupal UrlGenerator and interface? Though at that point inheritance might make more sense than decorating.

#21: cache-url_generator-1965074-21.patch queued for re-testing.

+++ b/core/tests/Drupal/Tests/Core/Routing/CachedUrlGeneratorTest.php
@@ -63,11 +71,11 @@ protected function setUp() {
   public function testGenerate() {
...
-    $this->assertEquals('test-route-1', $this->cachedUrlGenerator->generate('test_route'));
-    $this->assertEquals('test-route-1', $this->cachedUrlGenerator->generate('test_route'));
+    $this->assertEquals('test-route-1', $this->cachedUrlGenerator->generateFromRoute('test_route'));
+    $this->assertEquals('test-route-1', $this->cachedUrlGenerator->generateFromRoute('test_route'));

@@ -77,15 +85,15 @@ public function testGenerate() {
-    $this->assertEquals('test-route-1/value1', $this->cachedUrlGenerator->generate('test_route', array('key' => 'value1')));
-    $this->assertEquals('test-route-1/value1', $this->cachedUrlGenerator->generate('test_route', array('key' => 'value1')));
-    $this->assertEquals('test-route-1/value2', $this->cachedUrlGenerator->generate('test_route', array('key' => 'value2')));
-    $this->assertEquals('test-route-1/value2', $this->cachedUrlGenerator->generate('test_route', array('key' => 'value2')));
+    $this->assertEquals('test-route-1/value1', $this->cachedUrlGenerator->generateFromRoute('test_route', array('key' => 'value1')));
+    $this->assertEquals('test-route-1/value1', $this->cachedUrlGenerator->generateFromRoute('test_route', array('key' => 'value1')));
+    $this->assertEquals('test-route-1/value2', $this->cachedUrlGenerator->generateFromRoute('test_route', array('key' => 'value2')));
+    $this->assertEquals('test-route-1/value2', $this->cachedUrlGenerator->generateFromRoute('test_route', array('key' => 'value2')));
   }

We are testing the generate method, so we should call the generate method.

dawehner - in our implementation that basically delegates to generateFromRoute, and I inlined that delegation in this class now. That mean you could cache once calls to generate() or generateFromRoute(). Actually to do that right, we should actually not set $options unless $absolute is TRUE.

puts the test mostly back, and add comments and a small change to generate(), fixes use of Languge

fix language stuff mostly

fix doxygen.

should fix the last test

Let's try to bring down the patch size and introduce a CachedUrlGeneratorInterface similar to CachedModuleHandler and CachedPathAliasManager etc.

Missing file for CachedUrlGeneratorInterface ?

can't find dawehner, so adding the missing interface (not hard to guess).

I just reviewed the interdiff. The interface contains understandable basic documentation.

+++ b/core/lib/Drupal/Core/Routing/CachedUrlGeneratorInterface.php
@@ -0,0 +1,22 @@
+ * Provides additioanl methods for generators that cache the URLs.

additioanl should be additional

thanks.

updating title

#37: url_generator-1965074-37.patch queued for re-testing.

#37: url_generator-1965074-37.patch queued for re-testing.

reroll.

I think this needs work in 1 of 2 directions.

A quick fix would be to cache just in memory, so we avoid repeating the work if the same link is generated more than once per page (not sure this is even worth doing).

Otherwise, we need to maybe put the cache set into a destructor so we can accumulate all the generated routes for a page?

From Berdir here, we can implement Drupal\Core\DestructableInterface - core will cal the destruct() method on these when shutting down the container so that's the opportunity to persist the data.

We could potentially move somehow the storage logic of AliasManagerCacheDecorator into here.

What happens here when we have dynamic query string parameters etc... like tokens?

Updated issue summary.

This issue is no longer blocked! This is just a reroll.

Last patch is from September 20, 2013 which in HEAD years this makes it an aged pensioner...

So no promises except it installs, UrlGenerator test pass locally .. lets see what testbot has to say

1) My reroll in #47 mangled the service definition of url_generator in core.services.yml.

2) Relaxed __construct of MenuLinkFormController to permit any object conforming to UrlGeneratorInterface
so we can feed it a caching version.

3) minor tidy now using the appropriate getter in CachedUrlGenerator.php

-      $this->cacheKey .= '::' . $this->languageManager->getLanguage(Language::TYPE_URL)->id;
+      $this->cacheKey .= '::' . $this->languageManager->getLanguage(Language::TYPE_URL)->getId();

butter fingers

just looking at the commit log 7 commits between submission of patch and testing of patch

good o'le sprint time at NYC camp... will try again shortly

Browser install worked locally for me at #51, works for me now after commit sprint .. retesting

51: url_generator-1965074-50.patch queued for re-testing.

More careful inspection of logs shows I need to backout "minor tidy" #50.3.

Trivial reroll + fixed a single issue.

Constructor of CachedUrlGenerator now takes LanguageManagerInterface

-  public function __construct(UrlGeneratorInterface $url_generator, CacheBackendInterface $cache, LanguageManager $language_manager) {
+  public function __construct(UrlGeneratorInterface $url_generator, CacheBackendInterface $cache, LanguageManagerInterface $language_manager) {

Recent patches reintroduced assertions that HEAD removed ...

LanguageListTest should pass now this has been resolved.

I am still trying to track down those pesky exceptions ... they all seem to have a common form.

+++ b/core/core.services.yml
@@ -307,12 +314,15 @@ services:
+  url_generator:
+    class: Drupal\Core\Routing\CachedUrlGenerator
+    arguments: ['@url_generator.uncached', '@cache.url_generator', '@language_manager']

I think it would make sense to call this "url_generator.cached" and then have "url_generator" simply be an alias for the former. That makes it easier to swap from cached to uncached.

#64 .. This is how it looks

  url_generator.uncached:
    class: Drupal\Core\Routing\UrlGenerator
    arguments: ['@router.route_provider', '@path_processor_manager', '@route_processor_manager', '@config.factory', '@settings']
    calls:
      - [setRequest, ['@?request']]
      - [setContext, ['@?router.request_context']]
  url_generator.cached:
    class: Drupal\Core\Routing\CachedUrlGenerator
    arguments: ['@url_generator.uncached', '@cache.url_generator', '@language_manager']
  url_generator:
    alias: url_generator.cached

looking at the other uses of alias in HEAD, and looking for conformity. The obvious set of names would be

url_generator
url_generator.cached,
url_generator.active

but I am happy to go with tstoeckler's suggestion as that way we hide a implementation details away from other
devs.

Still hunting those 36 exceptions ... but in the mean time I have found 2 nitpicks.

That blew up because I did not subject #66 to the testbot..... and there was a problem with my aliasing.

I retried to get the alias to work but failed

using http://symfony.com/doc/current/components/dependency_injection/advanced.... as a reference I tried the alternate form of

url_generator:
  alias: '@url_generator'

but they introduced a different class of problem..
Unnervingly I tried swapping the order of url_generator and url_generator.uncached service definition in core.services.yml which if I understand the multiple pass dependency resolution nature of setting up services should make no difference.. and yet the error messages changed. [ and yes I was clearing the cache :-) ]

I tried for extra protection making the url_generator.uncached. private, so no application code can make use of it but no success..

So in the end while muttering under my breath about alpha level software I concluded that all this was out of the scope of this issues and backout the alias changes in
#65

PS making 'url_generator.uncached' private seems sensible.. so that remained.

Removed some exceptions.

Patch no longer applied. Reroll.

Patch no longer applies - reroll.

Nothing but merging and auto-merging.

Reroll.

Looking at git history ... there was a revert between submission and testing .. which causes a conflict ....

so the reroll lasted about 30mins
This is the second revert today .. will move this forward tomorrow when there is more stability

Reroll of #74 just to be sure.

Just keeping notes for myself, so I don't forget, things to correct when reroll is better ...

trival fix for CachedUrlGenerator.php

  /**
   * {@inheritdoc}
   */
  public function generate($name, $parameters = array(), $absolute = FALSE) {
    $options = array();
    // We essentially inline the implentation from the Drupal UrlGenerator
    // and avoid setting $options so that we increase the liklihood of caching.
    if ($absolute) {
      $options['absolute'] = $absolute;

implementation and likelihood need a spelling correction

( convert to psr-4 has just landed. )

Hmm traded the 3 identical exceptions exceptions in #74

The container was serialized.	User error	Container.php	34	Drupal\Core\DependencyInjection\Container->__sleep()

For 96 exceptions today in #79.. So still the one issue to understand. :-)

Reroll.

Straight reroll.

This fixes those errors.

But the fact that we get that far is scary, because it means $options is getting passed entities which have plugins bags, so even with this, we are serializating a lot of stuff, which makes building the cache key slow.

Also pretty sure this isn't working at all right now, DestructableInterface needs the corresponding needs_destruction service tag to do anything which means we never write the cache from what I can see. Unit tests are nice but this needs an integration test to make sure that we actually write something into the cache after a request. Also, cache stampedes could be an issue..

So, I'm wondering how much of this patch is overkill?

It seems a lot of the slowness is in basically generating the internal (system) path from the route name and route parameters.

Can we just cache that piece of it and let the rest of the generator do its work?

Basically in generateFromRoute(), caching the result of these 3 method calls:

    $route = $this->getRoute($name);
    $this->processRoute($route, $parameters);
    $path = $this->getInternalPathFromRoute($route, $parameters);

Also pretty sure this isn't working at all right now, DestructableInterface needs the corresponding needs_destruction service tag to do anything which means we never write the cache from what I can see.

Correct. Even if you add the tag, it still doesn't work, because writeCache() only writes if there's a $this->cacheKey, which only gets set in setRequest(). If you try to make setRequest() called by adding a

calls:
      - [setRequest, ['@?request']]

to the 'url_generator' service definition, it still doesn't work, because that gets called prior to _system_path getting populated.

+++ b/core/lib/Drupal/Core/Routing/CachedUrlGenerator.php
@@ -0,0 +1,227 @@
+  public function getPathFromRoute($name, $parameters = array()) {
+    return $this->urlGenerator->getPathFromRoute($name, $parameters);
+  }

Also, related to #89, not caching this on its own (whether in addition to or instead of the full URLs) is unfortunate, because LinkGenerator calls $url->getInternalPath() which calls this, so this isn't speeding up link generation as much as it could be. Although LinkGenerator does have a @todo to remove that, that hasn't been done yet, so pending that being done, I think we should include getPathFromRoute() in what gets cached by this issue.

So, I'm a little confused about why we are generating the cache key off the system path.

Looking at core.services.yaml we currently have that:

  url_generator:
    class: Drupal\Core\Routing\UrlGenerator
    arguments: ['@router.route_provider', '@path_processor_manager', '@route_processor_manager', '@config.factory', '@settings', '@logger.channel.default']
    calls:
      - [setRequest, ['@?request']]
      - [setContext, ['@?router.request_context']]

Among other things, we shouldn't be using _system_path here (or anyplace) if we can avoid it. The problems with it or the route and parameters not being populated could instead be solved by loading the cache entry when the first generate call is made?

As above I think we might be better off just caching the part of the work that doesn't depend on the current request.

That call to the UrlGenerator should be removed ASAP, so let's not expand the scope of this issue for something that's deprecated.

Coming from #2335661: Outbound path & route processors must specify cacheability metadata, which led me to the issue that introduced route processors (#1798296: Integrate CSRF link token directly into routing system). Which led me to this issue and to #1798296-80: Integrate CSRF link token directly into routing system, which said:

We have talked about adding the path and route processors as a single processor, this should make things much better. I would prefer to do that as a critical follow up, how does that sound? If not, I can reroll and do it in this patch. I just don't want this patch to get too large.. and merging the processors will no doubt spark a whole discussion of its own.

This patch does not yet take into account the cacheability implications of route processors and path processors. Therefore it would currently happily cache links with CSRF tokens.

Optimizing this was a _requirement_ to get the link generator in as per:

- https://www.drupal.org/node/2047619#comment-7801557

AND

- https://www.drupal.org/node/2047619#comment-7813067

There was a 50% performance regression (!) at time this got in for route generation.

I can understand why chx' asks for a roll back of the router with the way this was added.

This is very poor process just to get an agenda done.

We could something similar to path aliases and preload all routes per route, this would already improve a bit here. Something similar is though already done for menu links, so we don't
load most of the routes in non-parallel code.

Is there anything we can do to improve the actual performance of generating the URL (and not having to go back to URLs again, as the amount of work here would be notestimatable).

@dawehner didn't you implement that already in #2058845: Pre-load non-admin routes? Have we regressed again since?

All I am saying is: This needs to go in before core is released as it was stated this would be ready soon to justify the LinkGenerator going in immediately.

It seems berdir is close though, so this should be a non-issue.

We originally introduced a 50% performance regression against l(), so with caching we should be seeing only 5-20% performance regression or such.

I agree this is independently critical of #1744302: [meta] Resolve known performance regressions in Drupal 8.

There is 'performance is critically bad overall', and then there is 'critical performance issue', and this is the latter.

Also for the record I said exactly the same thing before the original routing patch was even committed #1793520-7: Add access control mechanism for new router system but actually enforcing that this got dealt with has slipped through the cracks until now. Thanks Fabianx for re-reviewing some of these issues and re-identifying the blockers.

,

This is a fairly naive re-roll of #88 to keep the issue moving.

Somehow use DependencySerializationTrait; got in there twice.

Related issue

I think we have to ensure that some generated URLs aren't cacheable. You can also potentially cleanup a bunch of stuff.

Here is a patch with interdiff.
- fixing PHP error in CachedURLGenerator class (conflict in namespaces)
- fixing unit test for that class

Oh. Sorry. This one. The interdiff is the same.

Uhh. 9 fails, will try to identify the problem and fix them...

This patch contains a new class CachedUrlGeneratorTest

A couple of observations .... tomorrow night I may fix..

CachedUrlGenerator::generateFromRoute() - first line

  public function generateFromRoute($name, $parameters = array(), $options = array()) {
    // In some cases $name may be a Route object, rather than a string.

This maybe OK, but it is out of step with the annotations in UrlGeneratorInterface

inherited via

interface CachedUrlGeneratorInterface extends UrlGeneratorInterface {

Which require $name to be a string. Now I want to highlight the mechanism of how this interface change breaks the code....

I am still mulling over the fix.

$names which are not guaranteed to be strings enter RouteProvider::getRoutesByNames($names) as an array
and fall over at the the first function which requires them to be strings ( highlighted with an *)

 public function getRoutesByNames($names) {

    if (empty($names)) {
      throw new \InvalidArgumentException('You must specify the route names to load');
    }

    $this->routeBuilder->rebuildIfNeeded();

    $routes_to_load = array_diff($names, array_keys($this->routes));   <------ * breaks here!!!!
    if ($routes_to_load) {
      $result = $this->connection->query('SELECT name, route FROM {' . $this->connection->escapeTable($this->tableName) . '} WHERE name IN (:names)', array(':names' => $routes_to_load));
      $routes = $result->fetchAllKeyed();

      foreach ($routes as $name => $route) {
        $this->routes[$name] = unserialize($route);
      }
    }

    return array_intersect_key($this->routes, array_flip($names));
  }

I'll have a look at it forawhile, if you are okay with it

This could fix it.

Nice.
DownloadTest fails because it checks generated url of private file both when clean urls are on and off, and it always returns cached url for version with clean urls enabled.
(line 122 in the file)

One more thing.
As Berdir mentioned in #88 - "DestructableInterface needs the corresponding needs_destruction service tag to do anything which means we never write the cache from what I can see."
So for now we do not write anything into cache, and this is not handled by added tests.
Also "setRequest" method never called for CachedUrlGenerator service because setter injection is not defined for it in core.services.yml.

A little fix-up.

First off Drupal\block\Tests\BlockTest takes a long time to rerun. So here is a short cut

At full stack trace can easily be obtained by visiting, these 2 URLs in sequence.

/user ( correct response a 302 to redirect )
/USER ( correct response a 404 to indicate not found )

With the patch applied the visit to /USER will trigger and error.

What this highlights is that cache keys are case-sensitive.

Currently CachedUrlGenerator::fromRequestStack in response to each URL generate cache keys of "user" and "USER"
The problem is they are being fed into a CASE-INSENSTIVE SQL query in DatabaseBackend::getMultiple(0

    // When serving cached pages, the overhead of using ::select() was found
    // to add around 30% overhead to the request. Since $this->bin is a
    // variable, this means the call to ::query() here uses a concatenated
    // string. This is highly discouraged under any other circumstances, and
    // is used here only due to the performance overhead we would incur
    // otherwise. When serving an uncached page, the overhead of using
    // ::select() is a much smaller proportion of the request.
    $result = array();
    try {
      $result = $this->connection->query('SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM {' . $this->connection->escapeTable($this->bin) . '} WHERE cid IN (:cids)', array(':cids' => array_keys($cid_mapping)));
    }
    catch (\Exception $e) {
      // Nothing to do.
    }

So that is where I am tonight ... more maybe tomorrow:)

This is a one line fix to resolve the case sensitivity of cache keys

It is not without performance implications though

Also the issue is picked up ONLY in a unrelated test, which is subject to change in the future without thought for caching
.. so next I want to add more tests in CachedUrlGeneratorTest.

updates involving Symfony 2.6 went in today

changes affecting UrlGeneration - retesting out of a sense of paranoia.

A) CachedUrlGenerator::fromRequestStack() is not a descriptive function name. I see it follows the naming convention established in router.request_context, but I don't think that is a good enough reason to keep it.

The function selects/loads the appropriate cache of URLs required to service the current Request. I have changed to name to prepareCache() - naming things is hard, so I am open to suggestions... just something more descriptive please.

I see this as one of the key functions to focus on when analysing the design decisions good or bad ... so I have added some comments.

B) There are comments above about the costliness of generating the cacheKeys. While this an infrequently run task can someone justify the use of a sha256 hash? Given the use is for fast lookup rather than a cryptographic function. Additionally do we need a 128 bit digest? How many cache elements are expected?

From cuting and pasting from http://php.net/manual/en/function.hash.php

Milage may vary, but as a guide only .... here is the time in microseconds to calculate the various hashes ( see web page for full details )

1. md4 5307.912
2. md5 6890.058
3. crc32b 7298.946
4. crc32 7561.922
5. sha1 8886.098
...
29. sha256 19020.08

So in a non-rigorous handwavey way can I suggest we would get a speedup by a factor of 3.6 in one of the more expensive functions associated with the task
by dropping down to a md4 - a 32 bit digest.

C) In a separate issue CachedUrlGenerator::fromRequestStack/prepareCache - in #36 I adopted a sha1 as a work arround to case insensitive cache-keys lookups - md4 is a better choice there.

#135-#136 fixed the problem that the accumulated generated URLs for the following requests should be cached separately.

/user ( a 302 to redirect )
/USER ( a 404 )

Well here is a unit test that covers the issue.

So now we are not dependant on unrelated test accidently identifying the problems for us.
Note that this works by attaching a monitor that logs the number of distinct caches accessed. So indirectly CachedUrlGenerator::writeCache() is given some coverage.

Also in a minor note I have added the appropriate @coversDefaultClass to CachedGeneratorUnitTest.php

I noticed the cache problem somewhere else too (block cache when doing lower and upercase URL's), I think this should be fixed in a separate issue, in the cache API. The easiest fix might be to set the 'binary' => TRUE flag on the cid column in the cache table schema. Can you open an issue for that?

I agree, I would love to keep a relatively expensive md5 hash out of the critical path ( a hash that will be calculated for every page request! )

Can you open an issue for that?

Somebody already has :)

#2352207: Database cache backend does not treat cid as case sensitive

Ok So I think I have the kernel of an idea which will explain the failing tests...

There is a common link between ConfigImportAllTest and Download test

ConfigImportAllTest touches the URL generation and caching in a very limited way... it simulates a visit to /admin/config/development/configuration
I am including the blob that is pushed into the database when I visit the page manually.

I have pretty printed it so you can make out a serialized array of 30 key value pairs stored in a array ( 60 strings )

the last string of the pair contains the URL - This gnarly looking URL is stored.

/admin/config/user-interface/shortcut/manage/default/add-link-inline?token=nX__dWCXnUdBccb-P1bC3wjtkMejv-vjDI-kKJkFauQ&link=admin/config/development/configuration&name=Synchronize&destination=admin/config/development/configuration

the failing "download test" also includes a troublesome URL

How to store the URL in a database? google helps and suggests a few solutions

http://stackoverflow.com/questions/4923011/how-to-serialize-url-and-make...

I am busy and may not get to this this week... the weekend is looking better..

The unit test I have in my head is to demonstrate the storage and retrieval of some evil looking URLs

@martino107 that looks like it might be fixed by #2351015: URL generation does not bubble cache contexts - assuming the caching is breaking the CSRF token.

Yay #2352207: Database cache backend does not treat cid as case sensitive is in. @Berdir thanks for broadening my perspective. I have removed the md5 hash

#2351015: URL generation does not bubble cache contexts looks like a good solution ... Caching URLs with perishable tokens is a thorny issue. I am playing Catch's hunch and for now I am removing all shortcut.link_add_inline routes from the cache.

As a separate thorny issue. We should cache all possible URLs. So here is a reference http://www.w3schools.com/tags/ref_urlencode.asp
I don't think our current solution will work in the general case. but hey that is for the weekend.

Another thing that could break this, see the last few comments on #2382503: Not possible to render self-contained render array while a render stack is active.

Now that #2377281: Upgrade to Symfony 2.6 stable is in...

Since the last testbot pass, we take a new path through the code, so I am retesting.

The changes, I see are in the area of Crell's Symfony\Cmf\Component\Routing\ProviderBaseGenerator::generate()

PS
I have removed the now redundant tests.

A) Fixed DownloadTest::testFileCreateUrl()
As it simulates toggling between clean and unclean URLs, the contents of the URL cache will become invalid.

B) Trivial nit-pick
Drupal\Core\Routing\UrlGenerator::generate($name, $parameters = array(), $absolute = FALSE) which implements the following interface Symfony\Component\Routing\Generator\UrlGeneratorInterface
Where the third parameter $absolute/$referenceType is a bool|string ... one on the following constants


const ABSOLUTE_URL = true;
const ABSOLUTE_PATH = false;
const RELATIVE_PATH = 'relative';
const NETWORK_PATH = 'network';


As it still stands, this could propagate a string into the $options array when a bool was expected..

C) The retest was necessary - there is now a "Recursive router rebuild detected" exception hmm

I was reading #2359369: Render cache is not cleared when module is uninstalled and saw this :-

https://www.drupal.org/node/2359369#comment-9388575

+    // Flush all persistent caches.
+    // Any cache entry might implicitly depend on the uninstalled modules,
+    // so clear all of them explicitly.

I guess we need to do that here also

Why would you want to do that and when?

There are no important performance or database size implications worth talking about...

I just want to keep things tidy.

The scenario I have in mind is a site with a long uptime and where modules come and go.

Each module would be free to populate the cache_url table with entries set with no expiry date...
The accumulation of junk would be small, but I favour a design with a comprehensive cache invalidation strategy.

In the back of my mind I also have this security scenario

Evil module takes advantage of a good uninstalled module and finds a way to make use of a stale but still cached external link to
and good site gone bad ...

Far fetched I know but something that makes me pause.

For this I think we should move away from a permanent cache entry and instead use a long TTL - could default to one month and make it configurable (either by service override or in settings). Just means that backends like the database that never evict can eventually clear out old items.

Not sure if that should be done here, or added to the database cache backend itself though.

Cache garbage collection/invalidation is a topic that needs widening to a larger audience.

From a quick review of open cache issues, there are a couple of things that touch on this point... tangentially

I am have opened a new issue to try and draw all the loose threads together...

Reroll

This was triggered by #2342593: Remove mixed SSL support from core

which removed the following parameter from the constructor in UrlGenerator

\Drupal\Core\Site\Settings $settings

In D7 I stopped using cache permanent due to the database not evicting like catch said in #160. Just be sure to make it so the chance of a cache stampede is rare; just in case the cached entries got set in bulk via drush or something like that.

    // Write to the cache.
    // CACHE_PERMANENT isn't good here. Use 2 weeks from now + 0-45 days.
    // The random 0 to 45 day addition is to prevent a cache stampede.
    cache_set($cid, $cache->data, 'cache_', round(REQUEST_TIME + 1209600 + mt_rand(0, 3888000), -3));

Restoring the _cachable status of shortcut.link_add_inline before I forget ( see #149 )

@mikeytown2 and interesting refinement, something I would like to see added to all caches via #2384957: Garbage collection when module is uninstalled.

added the issue summary template to the summary, but it still needs someone familiar with the issue to update the remaining tasks.. and other parts of the summary.

I have unpicked LanguageUILanguageNegotiationTest:testLanguageDomain() and have a questions to ask...

looking at the HTTP option from UrlGenerator->generateFromPath
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Routing!UrlGenera...

HTTP has three states :-

unset - use current scheme
TRUE - must point to a secure location ( HTTPS )
FALSE - enforce http

So with regard to testLangaugeDomain() the three calls to _url ( aka generateFromPath ) :-

1) create URL with 'HTTP option unset - pick up default scheme ( HTTP )
2) create URL with 'HTTP' option set to TRUE enforce HTTPS
3) create URL with 'HTTP" options unset, BUT pickup that the default schema changed to HTTPS

in (3) the default schema is simulated by pushing a request onto the stack with 'HTTPS' set to on, and the test failure is the cached http version, from 2 is returned.

So the questions relates to mix-mode support for HTTP and HTTPS

I can't find any policy docs relating to drupal 8 and mixed mode support. Can someone help me out? At the moment the best I can find is https://www.drupal.org/https-information. Which I read as contrib modules need this so YOU can ignore the massive security problems is opens up.

If we support mixed mode, which I fear we must then :-

CachedUrlGenerator::prepareCache() needs to inspect the current request and generate the cackey key using a call to Request::isSecure()

So the questions relates to mix-mode support for HTTP and HTTPS

#2342593: Remove mixed SSL support from core

Also #92 still not addressed:

Among other things, we shouldn't be using _system_path here (or anyplace)

See #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API, #2362227: Replace all instances of current_path(), #2372507: Remove _system_path from $request->attributes

Thanks for the comment @znerol, that was very helpful.

LanguageUILanguageNegotiationTest now passes.

Also #92 still not addressed:

Yep, but I was prioritizing bug fixing over refinement. as a way to encourage more activity on the issue. :)
Unfortunately I can only spend a little time each day on open source stuff.

For me, the next priority is what is missing from our unit tests, that let those ConfigImportAll test failures slip through.

So I hope to ask some difficult questions about ConfigImportAllTest::testInstallUninstall()
and hope it sparks ideas in other....

testInstallUnistall :-
1) Install every module.
2) Uninstall every module that can be uninstalled.
3) Import the configuration thereby re-installing all the modules.
4) rebuild the container.
5) Check that all modules that were uninstalled are now reinstalled.

So far so good - I want to focus on step 4.

WebTestBase::rebuildContainer()

 /**
   * Rebuilds \Drupal::getContainer().
   *
   * Use this to build a new kernel and service container. For example, when the
   * list of enabled modules is changed via the internal browser, in which case
   * the test process still contains an old kernel and service container with an
   * old module list.
   *
   * @see TestBase::prepareEnvironment()
   * @see TestBase::restoreEnvironment()
   *
   * @todo Fix https://www.drupal.org/node/2021959 so that module enable/disable
   *   changes are immediately reflected in \Drupal::getContainer(). Until then,
   *   tests can invoke this workaround when requiring services from newly
   *   enabled modules to be immediately available in the same request.
   */
  protected function rebuildContainer() {
    // Maintain the current global request object.
    $request = \Drupal::request();
    // Rebuild the kernel and bring it back to a fully bootstrapped state.
    $this->container = $this->kernel->rebuildContainer();

    // Make sure the url generator has a request object, otherwise calls to
    // $this->drupalGet() will fail.
    $this->prepareRequestForGenerator();
  }

Ok so the "maintain the global request object" is ahem non-functional-- the local variable is not used!!!
that is ok as long as an identical request object is created and pushed onto the stack.

But as far as I can see at no point is this new request stack [ created in prepareRequestForGenerator() ] passed into the CachedUrlGenerator::prepareCache()
it is too late, the sequence is wrong, the kernel is rebuilt without it.

I have a second, difficult question ... but tomorrow is another day.

Ok so the "maintain the global request object" is ahem non-functional-- the local variable is not used!!!

This might have been introduced by #2016629: Refactor bootstrap to better utilize the kernel.

We have a cyclic path while rebuilding the container... see the only exception
As after reading the issues surrounding #2016629: Refactor bootstrap to better utilize the kernel. Well that suggest we haven't taken enough care with the order of things while simulating the rebuilding of the container.

Also it make me reprioritize the _system_path refactoring task. It is better to try and open the loop with the code in as final a state as possible.

#167 asked for a summary update... FWIW here is my todo list.

A) Bug FIx - work out if the following observations can help identify the rebuild loop.
1) The UrlGenerator service HAD a soft-dependance of the @?request_stack and now has a hard dependence ( see 'calls:' prepareCache )
2) The UrlGenerator is now a service that need explicit destruction. ( CachedUrlGenerator implements DestructibleInterface ) is that affecting the way services are being restored.

B) Add simple integration test (see #88 )

Unit tests are nice but this needs an integration test to make sure that we actually write something into the cache after a request. Also, cache stampedes could be an issue..

I agree My take on this is that is doesn't need to be anything fancy, apart from the stampede test. The unit tests are comprehensive.

C) Profiling - as well as the standard task. For me there is an additional question First off it may not matter, but I think the response to a unprimed cache
may be unnecessarily slow... but again that may not matter. ( see #141 )

D) Garbage Collection #160 outlines a perfectly reasonable solution - a separate issue was created ... but strictly we should either acknowledge that this
issue is dependant on that one .. and bump that one to critical ... or discuss it further here.

So I plumbed in @current_route_match service into the UrlGenerator as $this->routeMatch

so I could swap out

    $this->cacheKey = $request_stack->getCurrentRequest()->attributes->get('_system_path');

for

    $this->cacheKey = $this->routeMatch->getRouteName();

Unfortunately, as UrlGenerator is invoked prior to routing a Null RouteMatch is injected for $this->routeMatch and the whole thing becomes a bust..

Do anyone else have a different way forward.. otherwise this needs to be declared a 'special case'.

Looking back to June 2013, the very first patch. .. We started using setter injection to mimic the way the uncached url Generator was constructed.
Well things have changed .. the only reason to use setter injection is for optional parameters. The uncached UrlGenerator's style of construction has changed and this patch did not keep up.

[ In short I have moved the prepareCache() functionality into the constructor where it should be ]

Current route match only returns meaningful results after routing. Why not use $request->getUri(); as cache key instead? The page cache relies on that too.

Works for me. This also simplifies the cache selection algorithm.

It may look like I have taken an axe to the code. but just to be specific the old functionality is preserved.

1) URI's distinguishable by script name still utilise separate bins.
2) URI's with language dependant elements still utilise separate bins.

First cut version of integration test -

Just the simple version first - it just a "Is the cache is being populated test?"

As it stands I have looked for integration tests in some other caches.
cache.bootstrap, cache.menu and they don't bother ... I make no comment - as I am not sure if this is a good or bad thing.

Despite what the CacheFactory::get() documentation says if the cache not has been initialized the return type in not a
\Drupal\Core\Cache\CacheBackendInterface - If not initialized a call to get() returns bool FALSE.

I am about to file a documentation issue for that.

If this is acceptable, then we could just test at the appropriate moment that the cache has been initialize and signal everything is OK

Until that is definite... I have taken the paranoid approach reaching into the cache and confirming its contents.

Full disclosure .. For anyone thinking this is overkill -- this extra tests consumes an extra 1min17sec on my local test environment.

Reroll. as might be expected after the Ghent DA sprint.

no conflicts just merging

This issue has a concurrency problem, that is, it works fine when pages are requested one after the other but falls over when multiple pages are requested simultaneous.... Ugh

To expose it open up access of the admin pages to the anonymous user and run this load test

ab -n 100 -c 10 http://HOSTNAME/admin/structure/views

As always it is difficult to get a handle on these things, but as best I can guess, the longer it takes to write the cache to the persistent store so the greater the chance of the next request loading a bad partially copy.

Let me try and justify that with a story I was trying to do some initial profiling.

I want to capture a sample of the saving a typical user might see. I did typical admin things, added users, created view, was generally nosey...

Here is a summary of the smallest and the greatest potential savings out of the 25 pages I visited. It is based on a subsequent inspection the cache_url_generator table.
smallest admin/modules/uninstall/confirm - 1 cached url (140B )
next smallest admin/structure/views/view/who_s_online/preview/who_s_online_block (16 elements 2.5KB of cache )
largest /admin/structure/views 117 elements ( 15.5 KB of cache data )

So that gave 3 profiles for testing :-

Hampster-16 [ admin/structure/views/view/who_s_online/preview/who_s_online_block ]
Hippo-117 [ /admin/structure/views ]
Headless Hipster-1

The headless hipster would be the "Are we doing more harm than good?" test - You are trying to be a speedy as possible and you are serving a simple JSON object which contains a single URL. Does the overhead of the cache wrapper impact on the single minded speed objective?

Anyway back to load testing

Hampster-16 works fine without error and gives about a 19% speedup ( from 378ms to 305ms )
Hiipo-117 when run without concurrency gives about a 16% speedup ( 617 to 515ms ) but like I say gives errors when run concurrently.

We seem to get away with writing 2.5KB to cache safely, BUT the writing 15.5KB to cache is too much before the next request comes in.

Anyway I would welcome comments on how to proceed.

@martin107
Do you understand already why the ConfigImportAll test fails exactly?

The headless hipster would be the "Are we doing more harm than good?" test - You are trying to be a speedy as possible and you are serving a simple JSON object which contains a single URL. Does the overhead of the cache wrapper impact on the single minded speed objective?

We certainly should profile, whether the effort done here is worth it, no question!

As always it is difficult to get a handle on these things, but as best I can guess, the longer it takes to write the cache to the persistent store so the greater the chance of the next request loading a bad partially copy.

The reason here is that we write using the ::destruct() method. This one is executed AFTER the response is send out, so the cache entry is not nec. available for the next request. For real life I think this is fine, we should more concentrate us on the case of warm caching here.

Do you understand already why the ConfigImportAll test fails exactly?

@dawehner, sorry getting ready for Christmas got in the way ...

I thought I did, but no...the more I look the less I like my explanation :(

The newly failing tests SaveUploadTest and RemoteFileSaveUploadTest .. both pass locally for me

For now this is just a reroll.

Alright, let's fix it.

Rerolled.

Fixed it.

Awesome! Do we need to see some performance metrics?

Yes please, I tried outlining 3 test profiles in #186

Hampster-16 [ admin/structure/views/view/who_s_online/preview/who_s_online_block ]
Hippo-117 [ /admin/structure/views ]
Headless Hipster-1

They are only suggestions ... if want to follow a different path, please go ahead

Did a comparison of the views admin page (/admin/structure/views) with and without this patch.

https://blackfire.io/profiles/compare/c64355c1-14e7-4fbf-9bf1-feb79c9fd0...

To me, this looks like performace is worse, but I can't see a reference to CachedUrlGenerator in the graph. :-(

I'm testing on a vagrant image (see https://github.com/nickschuch/vd8) which has xdebug enabled so the results are possibly inaccurate.

I'm testing on a vagrant image (see https://github.com/nickschuch/vd8) which has xdebug enabled so the results are possibly inaccurate.

Yeah, don't do that.

One thing you can see is that there are some ms saved in the url generator, see https://blackfire.io/profiles/compare/c64355c1-14e7-4fbf-9bf1-feb79c9fd0...()&selected=Drupal%5CCore%5CRouting%5CUrlGenerator%3A%3AgetInternalPathFromRoute&settings%5Bdimension%5D=wt&settings%5BcallerDepth%5D=2&settings%5BcalleeDepth%5D=&settings%5BminNodeCost%5D=5&settings%5BminCallCost%5D=0.01

If you search for url generator on the left side, it seems not worth to do.
Maybe we have to figure out a caching strategy per page, much like path aliases.

First, let's post a reroll, got some conflicts.

A really simple frontpage with in total 6 links renderer in total 4% faster. Not sure how good these numbers are. I would like to see someone setting up a menu with more items.

PS: It turned out to be a bad idea to profile admin/index, given that it parses YML files on every request.

Hoping someone might be interested in an issue summary update

Nothing code related just minor comments improvements. BTW nice work on the patch @dawehner.

1. +++ b/core/lib/Drupal/Core/Routing/CachedUrlGenerator.php
   @@ -0,0 +1,232 @@
   +    $this->cacheKey = $request_stack->getCurrentRequest()->getUri();
   

   Correct me if I am wrong this means that we are storing all the urls of the page against its uri. If this is correct can we add this to class description?

2. +++ b/core/lib/Drupal/Core/Routing/CachedUrlGenerator.php
   @@ -0,0 +1,232 @@
   +    elseif (NULL === $route = clone $this->routeProvider->getRouteByName($name)) {
   

   WoW

3. +++ b/core/tests/Drupal/Tests/Core/Routing/CachedUrlGeneratorTest.php
   @@ -0,0 +1,195 @@
   +    $this->assertEquals('test-route-1', $this->cachedUrlGenerator->generateFromRoute('test_route'));
   +    $this->assertEquals('test-route-1', $this->cachedUrlGenerator->generateFromRoute('test_route'));
   

   Nice test. Can we add comment before 2nd assert that it is fetched from cache?

I'll have a crack at updating the issue summary on this. But with 200+ comments, it might take a while :)

addresses #204 and fixes some minor nits - only documentation changes I found during review - so rtbcing

Profiling:

Created 5 nodes with links to terms etc, promoted them
Loaded home page to prime caches
Loaded home page again - this time profiling
Truncated cache_url_generator table (but no other caches)
Loaded home page again (cold url generator cache)

Since this is performance-related, tossing catch's way. Great work, folks!