Problem/Motivation

This is a follow-up for #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence
Part of this is postponed on #2351015: URL generation does not bubble cache contexts

Now that we have all link generation centralized into one of two methods on the generator, we need to make it faster. The obvious way to do that is to cache requests to generateFromRoute() and generateFromPath(), so that we catch all of the work done by the outbound path processors.

Proposed resolution

For path alias lookups, we did that with a wrapping object. I think it makes sense to do the same here, with an object that has the same interfaces as the generator but simply forwards requests to it and then caches them. The pattern established by the path alias cache should work fine here. (I think it's now using the Destructable interface? If not, we should do that.)

We may also want to consider removing the cache wrapper from path alias lookups once we're done. We probably shouldn't do that in this patch, but a follow up would be to benchmark and see if it's still useful, and if not, get rid of it to avoid the complexity and extra cache storage.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary Instructions
Add automated tests Instructions
performance profiling

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#256 interdiff.txt11.16 KBrteijeiro
#256 1965074-256.patch34.67 KBrteijeiro
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,594 pass(es).
[ View ]
#242 interdiff.txt486 bytesdawehner
#242 1965074-242.patch34.72 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,039 pass(es).
[ View ]
#240 interdiff.txt3.75 KBdawehner
#240 1965074-240.patch34.7 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,107 pass(es), 5 fail(s), and 30,490 exception(s).
[ View ]
#229 1965074-url-cache-229.patch31.58 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,710 pass(es).
[ View ]
#228 1965074-url-cache-228.patch31.52 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,781 pass(es).
[ View ]
#225 1965074-url-cache-225.patch31.55 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,084 pass(es).
[ View ]
#219 url-cache.219.patch31.53 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,223 pass(es).
[ View ]
#219 interdiff.txt2.39 KBlarowlan
#218 url-cache.218.patch31.72 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,176 pass(es), 1 fail(s), and 20 exception(s).
[ View ]
#218 interdiff.txt6.98 KBlarowlan
#211 url-cache.211.patch29.29 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,983 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#209 url-cache.209.patch30.62 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch url-cache.209.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#209 interdiff-206-209.txt10.31 KBhussainweb
#207 Screenshot 2015-01-30 13.56.34.png52.67 KBlarowlan
#206 url-cache.205.patch29.54 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,961 pass(es).
[ View ]
#206 interdiff.txt7.21 KBlarowlan
#202 1965074-202.patch28.34 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,155 pass(es).
[ View ]
#200 comparison.png14.83 KBkim.pepper
#197 1965074-197.patch28.32 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,895 pass(es).
[ View ]
#197 interdiff.txt651 bytesdawehner
#171 interdiff-165-171.txt1.34 KBmartin107
#171 1965074-171.patch27.44 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,721 pass(es), 39 fail(s), and 1 exception(s).
[ View ]
#165 interdiff-162-165.txt697 bytesmartin107
#165 1965074-165.patch26.1 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,674 pass(es), 37 fail(s), and 2 exception(s).
[ View ]
#162 1965074-162.patch26.78 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,474 pass(es), 47 fail(s), and 1 exception(s).
[ View ]
#155 interdiff-153-155.txt2.07 KBmartin107
#155 1965074-155.patch26.79 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,563 pass(es), 43 fail(s), and 1 exception(s).
[ View ]
#153 interdiff-149-153.txt3.22 KBmartin107
#153 1965074-153.patch24.73 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,565 pass(es), 40 fail(s), and 1 exception(s).
[ View ]
#149 interdiff-143-149.txt1.85 KBmartin107
#149 1965074-149.patch26.96 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,385 pass(es), 41 fail(s), and 0 exception(s).
[ View ]
#147 storedUrls.txt3.48 KBmartin107
#143 interdiff-141-143.txt3.45 KBmartin107
#143 1965074-143.patch26.61 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,388 pass(es), 42 fail(s), and 0 exception(s).
[ View ]
#141 1965074-141.patch24.1 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,089 pass(es), 41 fail(s), and 0 exception(s).
[ View ]
#141 interdiff-136-141.txt2.03 KBmartin107
#136 interdiff-133-136.txt677 bytesmartin107
#136 1965074-136.patch23.69 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,880 pass(es), 39 fail(s), and 0 exception(s).
[ View ]
#133 interdiff-128-133.txt755 bytesmartin107
#133 1965074-133.patch23.65 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,726 pass(es), 40 fail(s), and 1 exception(s).
[ View ]
#128 interdiff.txt2.84 KBundertext
#128 1965074-128.patch23.64 KBundertext
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,321 pass(es), 1,215 fail(s), and 548 exception(s).
[ View ]
#124 interdiff.txt2.21 KBdawehner
#124 1965074-124.patch23.33 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,974 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#118 1965074-118.patch22.51 KBundertext
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,747 pass(es), 9 fail(s), and 12 exception(s).
[ View ]
#116 1965074-116.patch22.47 KBundertext
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#114 interdiff-114.txt3.91 KBundertext
#114 1965074-114.patch22.67 KBundertext
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1965074-114.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#111 1965074-111.patch21.95 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#111 interdiff.txt4.92 KBdawehner
#108 interdiff-106-108.txt754 bytescilefen
#108 add_cache_wrapper_to-1965074-108.patch21.26 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,472 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#106 add_cache_wrapper_to-1965074-106.patch22.31 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#103 1965074-url-generator-103.patch22.69 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,478 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#88 url_generator-1965074-88-interdiff.txt1.44 KBBerdir
#88 url_generator-1965074-88.patch25.69 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch url_generator-1965074-88.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#86 url_generator-1965074-86.patch24.25 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,490 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#84 url_generator-1965074-85.patch24.22 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,598 pass(es), 0 fail(s), and 96 exception(s).
[ View ]
#79 url_generator-1965074-79.patch24.22 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,626 pass(es), 0 fail(s), and 96 exception(s).
[ View ]
#76 url_generator-1965074-76.patch24.52 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch url_generator-1965074-76.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#74 url_generator-1965074-74.patch24.49 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,484 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#72 url_generator-1965074-72.patch24.49 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,699 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#70 interdiff-68-70.txt786 bytesmartin107
#70 url_generator-1965074-70.patch24.54 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,328 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#68 url_generator-1965074-68.patch24.53 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,129 pass(es), 0 fail(s), and 35 exception(s).
[ View ]
#68 interdiff-66-68.txt918 bytesmartin107
#66 url_generator-1965074-66.patch24.57 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,042 pass(es), 84 fail(s), and 72 exception(s).
[ View ]
#66 interdiff-65-66.txt1.42 KBmartin107
#65 interdiff-62-65.txt634 bytesmartin107
#65 url_generator-1965074-65.patch24.61 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch url_generator-1965074-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#62 interdiff-60-62.txt899 bytesmartin107
#62 url_generator-1965074-62.patch24.55 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,847 pass(es), 0 fail(s), and 36 exception(s).
[ View ]
#60 url_generator-1965074-60.patch24.72 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,788 pass(es), 1 fail(s), and 36 exception(s).
[ View ]
#60 interdiff-57-60.txt2.66 KBmartin107
#57 url_generator-1965074-57.patch24.63 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,740 pass(es), 7 fail(s), and 36 exception(s).
[ View ]
#57 interdiff-51.57.txt784 bytesmartin107
#51 url_generator-1965074-50.patch24.64 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#50 interdiff-47-50.txt2.96 KBmartin107
#50 url_generator-1965074-50.txt24.64 KBmartin107
#47 url_generator-1965074-47.patch23.88 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,821 pass(es), 54 fail(s), and 2,136 exception(s).
[ View ]
#42 url_generator-1965074-42.patch25.45 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,700 pass(es).
[ View ]
#37 url_generator-1965074-37.patch25.47 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch url_generator-1965074-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 1965074-35-37.increment.txt617 bytespwolanin
#35 1965074-32-35.increment.txt735 bytespwolanin
#35 url_generator-1965074-35.patch25.47 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,451 pass(es).
[ View ]
#32 url_generator-1965074-32.patch25.35 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,837 pass(es), 1,663 fail(s), and 51,887 exception(s).
[ View ]
#32 interdiff.txt10.04 KBdawehner
#31 1965074-30-31.increment.txt885 bytespwolanin
#31 cache-url_generator-1965074-31.patch30.4 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]
#30 cache-url_generator-1965074-30.patch29.54 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,603 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#30 1965074-29-30.increment.txt519 bytespwolanin
#29 1965074-27-29.increment.txt10.57 KBpwolanin
#29 cache-url_generator-1965074-29.patch29.56 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,550 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#27 cache-url_generator-1965074-27.patch21.12 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,390 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#27 1965074-21-27.increment2.txt3.14 KBpwolanin
#21 cache-url_generator-1965074-21.patch20.91 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 54,355 pass(es), 938 fail(s), and 482 exception(s).
[ View ]
#21 1965074-19-21.increment.txt10 KBpwolanin
#19 cache-url_generator-1965074-19.patch20.79 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,134 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#19 interdiff.txt4.03 KBdawehner
#17 cache-url_generator-1965074-17.patch17.22 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,142 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#10 drupal-1965074-10.patch15.61 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,855 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#10 interdiff.txt2.54 KBdawehner
#8 drupal-1965074-8.patch17.87 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,655 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#8 interdiff.txt2.95 KBdawehner
#6 drupal-1965074-6.patch15.73 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,380 pass(es), 13 fail(s), and 302 exception(s).
[ View ]
#4 1965074.cached-url-generator.patch10.26 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

katbailey’s picture

Assigned:Unassigned» katbailey
Status:Postponed» Active

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

Berdir’s picture

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?

Crell’s picture

Drupal::urlGenerator() was already added in the previous issue.

katbailey’s picture

StatusFileSize
new10.26 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Crell’s picture

I don't know why that would be breaking the installer, other than "it's the installer *cries*". It does look like what I expected it to look, though, so it's on the right track if nothing else. (Or I'm completely wrong, which is always a possibility.)

+++ b/core/lib/Drupal/Core/Routing/CachedURLGenerator.php
@@ -0,0 +1,178 @@
+  /**
+   * Passes all unknown calls to the decorated object.
+   */
+  public function __call($method, $args) {
+    return call_user_func_array(array($this->urlGenerator, $method), $args);
+  }

In my experience, this sometimes works out well and sometimes causes all sorts of insanity. What methods is this useful for, or is it "just in case"?

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new15.73 KB
FAILED: [[SimpleTest]]: [MySQL] 57,380 pass(es), 13 fail(s), and 302 exception(s).
[ View ]

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.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new2.95 KB
new17.87 KB
FAILED: [[SimpleTest]]: [MySQL] 57,655 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

This fixes a couple of the failures.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new2.54 KB
new15.61 KB
FAILED: [[SimpleTest]]: [MySQL] 57,855 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

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.

dawehner’s picture

katbailey’s picture

Assigned:katbailey» Unassigned

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

dawehner’s picture

Status:Needs work» Needs review

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

pwolanin’s picture

Also needs work since PathBasedGeneratorInterface has been replaced.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new17.22 KB
FAILED: [[SimpleTest]]: [MySQL] 58,142 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

Rerolled and added tests for generateFromRoute.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new4.03 KB
new20.79 KB
FAILED: [[SimpleTest]]: [MySQL] 58,134 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

I can't fix the language based failures now.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new10 KB
new20.91 KB
FAILED: [[SimpleTest]]: [MySQL] 54,355 pass(es), 938 fail(s), and 482 exception(s).
[ View ]

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.

pwolanin’s picture

Status:Needs work» Needs review

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

dawehner’s picture

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

pwolanin’s picture

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.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new3.14 KB
new21.12 KB
FAILED: [[SimpleTest]]: [MySQL] 58,390 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

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

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new29.56 KB
FAILED: [[SimpleTest]]: [MySQL] 58,550 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new10.57 KB

fix language stuff mostly

pwolanin’s picture

StatusFileSize
new519 bytes
new29.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,603 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

fix doxygen.

pwolanin’s picture

StatusFileSize
new30.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]
new885 bytes

should fix the last test

dawehner’s picture

StatusFileSize
new10.04 KB
new25.35 KB
FAILED: [[SimpleTest]]: [MySQL] 54,837 pass(es), 1,663 fail(s), and 51,887 exception(s).
[ View ]

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

pwolanin’s picture

Missing file for CachedUrlGeneratorInterface ?

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new25.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,451 pass(es).
[ View ]
new735 bytes

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

mradcliffe’s picture

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

pwolanin’s picture

StatusFileSize
new617 bytes
new25.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch url_generator-1965074-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

thanks.

pwolanin’s picture

Title:Add cache wrapper to Generator» Add cache wrapper to the UrlGenerator

updating title

pwolanin’s picture

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

dawehner’s picture

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

dawehner’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI
StatusFileSize
new25.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,700 pass(es).
[ View ]

reroll.

pwolanin’s picture

Status:Needs review» Needs work

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?

pwolanin’s picture

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.

dawehner’s picture

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

damiankloip’s picture

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

damiankloip’s picture

Issue summary:View changes

Updated issue summary.

martin107’s picture

Issue summary:View changes
StatusFileSize
new23.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,821 pass(es), 54 fail(s), and 2,136 exception(s).
[ View ]

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

martin107’s picture

Status:Needs work» Needs review
martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new24.64 KB
new2.96 KB

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();
martin107’s picture

StatusFileSize
new24.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

butter fingers

martin107’s picture

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

martin107’s picture

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

martin107’s picture

Status:Needs work» Needs review

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

martin107’s picture

StatusFileSize
new784 bytes
new24.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,740 pass(es), 7 fail(s), and 36 exception(s).
[ View ]

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

martin107’s picture

Status:Needs work» Needs review
martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new2.66 KB
new24.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,788 pass(es), 1 fail(s), and 36 exception(s).
[ View ]

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) {
martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new24.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,847 pass(es), 0 fail(s), and 36 exception(s).
[ View ]
new899 bytes

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.

tstoeckler’s picture

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

martin107’s picture

StatusFileSize
new24.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch url_generator-1965074-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new634 bytes

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

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
new24.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,042 pass(es), 84 fail(s), and 72 exception(s).
[ View ]

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

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new918 bytes
new24.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,129 pass(es), 0 fail(s), and 35 exception(s).
[ View ]

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.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new24.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,328 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
new786 bytes

Removed some exceptions.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new24.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,699 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Patch no longer applied. Reroll.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new24.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,484 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Patch no longer applies - reroll.

Nothing but merging and auto-merging.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new24.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch url_generator-1965074-76.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll.

martin107’s picture

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

martin107’s picture

StatusFileSize
new24.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,626 pass(es), 0 fail(s), and 96 exception(s).
[ View ]

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

martin107’s picture

Status:Needs work» Needs review
martin107’s picture

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

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new24.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,598 pass(es), 0 fail(s), and 96 exception(s).
[ View ]

Reroll.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new24.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,490 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Straight reroll.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:+Needs tests
StatusFileSize
new25.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch url_generator-1965074-88.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.44 KB

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

pwolanin’s picture

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);
effulgentsia’s picture

Status:Needs review» Needs work

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.

effulgentsia’s picture

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

pwolanin’s picture

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.

Wim Leers’s picture

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.

Fabianx’s picture

Priority:Major» Critical

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.

Fabianx’s picture

Issue tags:+RC blocker
dawehner’s picture

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

catch’s picture

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

Fabianx’s picture

Issue tags:+Needs profiling

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.

catch’s picture

Issue tags:-RC blocker

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.

dawehner’s picture

Issue tags:+Needs reroll

,

kim.pepper’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new22.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,478 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

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

cilefen’s picture

Assigned:Unassigned» cilefen
Issue tags:+Needs reroll
cilefen’s picture

Assigned:cilefen» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new22.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
cilefen’s picture

Status:Needs work» Needs review
StatusFileSize
new21.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,472 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new754 bytes

Somehow use DependencySerializationTrait; got in there twice.

dawehner’s picture

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new4.92 KB
new21.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

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

undertext’s picture

Assigned:Unassigned» undertext
undertext’s picture

Status:Needs work» Needs review
StatusFileSize
new22.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1965074-114.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.91 KB

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

undertext’s picture

Status:Needs work» Needs review
StatusFileSize
new22.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

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

undertext’s picture

Status:Needs work» Needs review
StatusFileSize
new22.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,747 pass(es), 9 fail(s), and 12 exception(s).
[ View ]
undertext’s picture

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

martin107’s picture

Issue tags:-Needs tests

This patch contains a new class CachedUrlGeneratorTest

martin107’s picture

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));
  }

dawehner’s picture

Assigned:undertext» Unassigned

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

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new23.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,974 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.21 KB

This could fix it.

Status:Needs review» Needs work

The last submitted patch, 124: 1965074-124.patch, failed testing.

undertext’s picture

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)

undertext’s picture

Issue tags:+Needs tests

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.

undertext’s picture

StatusFileSize
new23.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,321 pass(es), 1,215 fail(s), and 548 exception(s).
[ View ]
new2.84 KB
undertext’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 128: 1965074-128.patch, failed testing.

undertext queued 128: 1965074-128.patch for re-testing.

The last submitted patch, 128: 1965074-128.patch, failed testing.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new23.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,726 pass(es), 40 fail(s), and 1 exception(s).
[ View ]
new755 bytes

A little fix-up.

Status:Needs review» Needs work

The last submitted patch, 133: 1965074-133.patch, failed testing.

martin107’s picture

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

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new23.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,880 pass(es), 39 fail(s), and 0 exception(s).
[ View ]
new677 bytes

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.

Status:Needs review» Needs work

The last submitted patch, 136: 1965074-136.patch, failed testing.

martin107’s picture

updates involving Symfony 2.6 went in today

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

martin107 queued 136: 1965074-136.patch for re-testing.

The last submitted patch, 136: 1965074-136.patch, failed testing.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new2.03 KB
new24.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,089 pass(es), 41 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 141: 1965074-141.patch, failed testing.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new26.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,388 pass(es), 42 fail(s), and 0 exception(s).
[ View ]
new3.45 KB

#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

Status:Needs review» Needs work

The last submitted patch, 143: 1965074-143.patch, failed testing.

Berdir’s picture

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?

martin107’s picture

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

martin107’s picture

StatusFileSize
new3.48 KB

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

catch’s picture

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

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new26.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,385 pass(es), 41 fail(s), and 0 exception(s).
[ View ]
new1.85 KB

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.

Status:Needs review» Needs work

The last submitted patch, 149: 1965074-149.patch, failed testing.

catch’s picture

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.

xjm’s picture

Issue tags:+Triaged D8 critical
martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new24.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,565 pass(es), 40 fail(s), and 1 exception(s).
[ View ]
new3.22 KB

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.

Status:Needs review» Needs work

The last submitted patch, 153: 1965074-153.patch, failed testing.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new26.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,563 pass(es), 43 fail(s), and 1 exception(s).
[ View ]
new2.07 KB

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

Status:Needs review» Needs work

The last submitted patch, 155: 1965074-155.patch, failed testing.

martin107’s picture

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

Berdir’s picture

Why would you want to do that and when?

martin107’s picture

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.

catch’s picture

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.

martin107’s picture

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

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new26.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,474 pass(es), 47 fail(s), and 1 exception(s).
[ View ]

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
mikeytown2’s picture

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.

<?php
   
// 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));
?>

Status:Needs review» Needs work

The last submitted patch, 162: 1965074-162.patch, failed testing.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new26.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,674 pass(es), 37 fail(s), and 2 exception(s).
[ View ]
new697 bytes

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.

Status:Needs review» Needs work

The last submitted patch, 165: 1965074-165.patch, failed testing.

YesCT’s picture

Issue summary:View changes
Issue tags:+Needs issue summary update

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.

martin107’s picture

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()

znerol’s picture

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

#2342593: Remove mixed SSL support from core

znerol’s picture

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new27.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,721 pass(es), 39 fail(s), and 1 exception(s).
[ View ]
new1.34 KB

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.

Status:Needs review» Needs work

The last submitted patch, 171: 1965074-171.patch, failed testing.

martin107’s picture

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.

znerol’s picture

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.

martin107’s picture

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.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new6.22 KB
new27.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,760 pass(es), 36 fail(s), and 1 exception(s).
[ View ]

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 ]

Status:Needs review» Needs work

The last submitted patch, 176: 1965074-176.patch, failed testing.

znerol’s picture

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.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new3.9 KB
new26.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,845 pass(es), 37 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 179: 1965074-179.patch, failed testing.

martin107’s picture

StatusFileSize
new1.83 KB
new27.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,999 pass(es), 40 fail(s), and 1 exception(s).
[ View ]

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.

martin107’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 181: 1965074-181.patch, failed testing.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new27.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,418 pass(es), 40 fail(s), and 2 exception(s).
[ View ]

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

no conflicts just merging

Status:Needs review» Needs work

The last submitted patch, 184: 1965074-184.patch, failed testing.

martin107’s picture

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.

dawehner’s picture

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

Status:Needs work» Needs review

martin107 queued 184: 1965074-184.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 184: 1965074-184.patch, failed testing.

martin107’s picture

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 :(

martin107’s picture

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

dawehner’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new27.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,118 pass(es), 36 fail(s), and 1 exception(s).
[ View ]

For now this is just a reroll.

Status:Needs review» Needs work

The last submitted patch, 192: 1965074-192.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new28.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,848 pass(es).
[ View ]
new725 bytes

Alright, let's fix it.

dawehner’s picture

StatusFileSize
new28.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, 195: 1965074-195.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new651 bytes
new28.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,895 pass(es).
[ View ]

Fixed it.

kim.pepper’s picture

Awesome! Do we need to see some performance metrics?

martin107’s picture

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

kim.pepper’s picture

StatusFileSize
new14.83 KB

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

Comparison

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.

dawehner’s picture

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.

dawehner’s picture

StatusFileSize
new28.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,155 pass(es).
[ View ]

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.

larowlan’s picture

Issue tags:+Critical Office Hours

Hoping someone might be interested in an issue summary update

jibran’s picture

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?

kattekrab’s picture

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

larowlan’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new7.21 KB
new29.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,961 pass(es).
[ View ]

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

larowlan’s picture

Issue tags:-Needs profiling
StatusFileSize
new52.67 KB

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)

webchick’s picture

Assigned:Unassigned» catch

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

hussainweb’s picture

StatusFileSize
new10.31 KB
new30.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch url-cache.209.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I am just fixing nitpicks and documentation mostly. I changed a couple of self:: to static:: in case of constants as constants can be overridden when the class is extended. I am also tempted to change all array() to [] in the new files but didn't so as to avoid confusion in this critical.

+++ b/core/tests/Drupal/Tests/Core/Routing/CachedUrlGeneratorIntegrationTest.php
@@ -0,0 +1,37 @@
+    // BAD - Use knowledge of an internal implementation detail to extract the
+    // cache data!
+    $data = $cache->get($name)->data;

I just don't know how to rephrase this.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 209: url-cache.209.patch, failed testing.

hussainweb’s picture

Status:Needs work» Needs review
StatusFileSize
new29.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,983 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

That was quick. :)

The conflict was in this part:

+++ b/core/modules/language/src/Tests/LanguageUILanguageNegotiationTest.php
@@ -472,10 +472,13 @@ function testLanguageDomain() {
-    // Test HTTPS via current URL scheme.
+    // Switch from HTTP to HTTPS support.
     $request = Request::create('', 'GET', array(), array(), array(), array('HTTPS' => 'on'));
     $this->container->get('request_stack')->push($request);
     $generator = $this->container->get('url_generator');
+    $generator->clearCache();
+
+    // Test HTTPS via current URL scheme.
     $italian_url = _url('admin', array('language' => $languages['it'], 'script' => ''));

The $generator was removed in #2364157: Replace most existing _url calls with Url objects, which just got in. Without the generator, it seemed to me that there was nothing to do here and hence this file does not appear in the reroll at all. This is why the status is back to Needs review instead of RTBC.

Status:Needs review» Needs work

The last submitted patch, 211: url-cache.211.patch, failed testing.

hussainweb’s picture

The failure occurs because we are now caching the URL and now the context is different. Earlier, it worked because we had:

     $generator = $this->container->get('url_generator');
+    $generator->clearCache();

But I am not sure if this is a proper test. Here, we are forcing the cache to be cleared before URL is regenerated so that HTTPS is picked up from the request stack. However, this may not happen in real life scenarios. If the user suddenly switches to HTTPS, won't the URL's continue to be served from the cache? To me, it seems that we have to account for specific request attributes when generating the cache key.

larowlan’s picture

Assigned:catch» Unassigned

Right, so we need to react to $this->container->get('request_stack')->push($request) and update our cache key.

znerol’s picture

+++ b/core/lib/Drupal/Core/Routing/CachedUrlGenerator.php
@@ -0,0 +1,236 @@
+    // Select cache based on the current request URI. We store one entry for
+    // each URI.
+    $this->cacheKey = $request_stack->getCurrentRequest()->getUri();

The cache key depends on the current request and hence should not be computed upon construction.

kattekrab’s picture

Sorry. I've read the whole issue, and can't update the issue summary. This needs someone with a deeper understanding of what's actually going on here. :/

larowlan’s picture

Assigned:Unassigned» larowlan
larowlan’s picture

Assigned:larowlan» Unassigned
Status:Needs work» Needs review
StatusFileSize
new6.98 KB
new31.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,176 pass(es), 1 fail(s), and 20 exception(s).
[ View ]

a start
needs work still to make sure all the cache keys are stored in this->cacheKeys so delete/clear work correctly - or that we just use the keys of the ->cachedUrls

larowlan’s picture

StatusFileSize
new2.39 KB
new31.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,223 pass(es).
[ View ]

should be finished now

The last submitted patch, 218: url-cache.218.patch, failed testing.

pwolanin’s picture

Given that we need to load the route for access checks, usually, I still think we should look at #2074297: Optimize the code in doGenerate() in the UrlGenerator to take advantage of Drupal path restrictions. as an alternative to *another* attempt to cache things.

larowlan’s picture

@pwolanin ok, but this is a critical, that isn't
can't we do both?

pwolanin’s picture

Yes, we can do this then maybe remove the caching of path alias as suggested in the summary and also optimize the code. Not sure what will really give us the best outcome, but having the code optimized is a win no matter what.

dawehner’s picture

We need some ways to mark CSRF protected URLs as not cacheable, at least for now, until #2351015: URL generation does not bubble cache contexts is in.

kim.pepper’s picture

StatusFileSize
new31.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,084 pass(es).
[ View ]

Re-roll of #219

dawehner’s picture

One thing we could do is to ask route processors whether they want to alter the route objects on built time. Once we do that,
the individual routes could be marked as non-cacheable.

pwolanin’s picture

Status:Needs review» Needs work

So, I think the static cache that larolan moved out should really be in this patch - we don't want to hit the cache backend at all if we keep it in memory.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new31.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,781 pass(es).
[ View ]

Just a reroll.

effulgentsia’s picture

StatusFileSize
new31.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,710 pass(es).
[ View ]

Sorry, #228 was a bad reroll. This is a correct reroll of #225.

effulgentsia’s picture

So, I think the static cache that larolan moved out should really be in this patch - we don't want to hit the cache backend at all if we keep it in memory.

@pwolanin: what do you mean by this? Looks to me like #219 and subsequent patches still retain the static $cachedUrls. The only thing that #219's interdiff removed was the static $cacheKeys, because the only thing that was in there was $request_stack->getCurrentRequest()->getUri(), which can be quickly fetched when needed.

pwolanin’s picture

@effulgentsia - nevermind, I was confused. They were trying to put it at a lower level in the generator there, but this looks like it does cache all generate URLs in memory during the request.

It seems like it caches and then loads every one that's ever been generated though?

effulgentsia’s picture

It seems like it caches and then loads every one that's ever been generated though?

Yes, each one that's been generated for the current request's URI. I wonder how efficient that is, given:

  • You can have a lot of links that were generated on a given page for an administrator that a plain authenticated user would not need, so stuff is fetched that isn't needed for the lower privileged user.
  • Once we finish #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, then even for a user in the same role, we'll be fetching a bunch of cached URLs that we don't need since the rendered menu is cached.

The first one can be fixed by adding role to the cache key. Not sure what we want to do about the second.

dawehner’s picture

You can have a lot of links that were generated on a given page for an administrator that a plain authenticated user would not need, so stuff is fetched that isn't needed for the lower privileged user.

We potentially can get rid of the cached path aliases when we can the URLs.

Once we finish #1805054: [PP-1] Cache localized, access filtered, URL resolved, (and rendered?) menu trees, then even for a user in the same role, we'll be fetching a bunch of cached URLs that we don't need since the rendered menu is cached.

Well, IMHO there are many more things which render URLs beside menus.

effulgentsia’s picture

Well, IMHO there are many more things which render URLs beside menus.

Really? Like what? Nodes and other content entities are already render cached. So are Views.

We potentially can get rid of the cached path aliases when we can the URLs.

Well, we need to at least be able to quickly do a getPathByAlias() if the current request's URI is an alias. However, this comment in AliasManager::writeCache():

  
   * Cache an array of the paths available on each page. We assume that aliases
   * will be needed for the majority of these paths during subsequent requests,
   * and load them in a single query during path alias lookup.

I think is suspect, because it was based on D7 experience where we were not caching rendered nodes or menus.

dawehner’s picture

Random links in local tasks, breadcrumbs, quick edit links, or simple parts of the admin area.

Well, we need to at least be able to quickly do a getPathByAlias() if the current request's URI is an alias. However, this comment in AliasManager::writeCache():

Well fair, but an index query should be fast? Alternative implementations could also use other storage engines for path alias, as always.
Note: For the inbound processing we don't rely currently on the alias manager but rather on the path alias storage itself:

<?php
    $request
= $event->getRequest();
   
$path = trim($request->getPathInfo(), '/');
   
$path = $this->pathProcessor->processInbound($path, $request);
   
$request->attributes->set('_system_path', $path);

   
// Set the cache key on the alias manager cache decorator.
   
if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
     
$this->aliasManager->setCacheKey($path);
    }
?>

The aliasManager cache key will be set afterwards, because before you don't have the system path available.

dawehner’s picture

@effulgentsia
If you think this is not a critical issue, please discuss it with @fabianx and @catch
I still think making the url generator fast is important, given how complex it is in Drupal.

effulgentsia’s picture

Yep, I'll discuss with @catch. My concern is not only with it maybe not being critical, but possibly that the solution here is worse than the problem. Because we're doing a cache get in the constructor, and the data in that single cache record can be large, and then not used in the very common case of 90+% of those URLs already being inside of a larger cached structure (like a rendered node, menu, or View). I think the approach here made a lot of sense when this issue started 1-2 years ago, but D8's improved approach of caching larger rendered things makes the approach here either moot or counterproductive.

dawehner’s picture

I totally agree with your point that the constructor should not be slow ... but rather just be retrieved, when needed.

dawehner’s picture

You could argue that you want to also care about the cases, in which something is not rendercacheable.

dawehner’s picture

StatusFileSize
new34.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,107 pass(es), 5 fail(s), and 30,490 exception(s).
[ View ]
new3.75 KB

Just want to be sure, we can already mark specific routes as not-cacheable. IMHO we don't need a separate mechanism from the existing event subscribers.
This interdiff marks all routes with _csrf_token requirements as not cacheable.

Status:Needs review» Needs work

The last submitted patch, 240: 1965074-240.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new34.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,039 pass(es).
[ View ]
new486 bytes

Ups ...

Wim Leers’s picture

Random links in local tasks, breadcrumbs, quick edit links, or simple parts of the admin area.

Quick edit links are cached on the client side.

Local tasks and breadcrumbs will become cacheable once we have fixed menu tree render caching.

You could argue that you want to also care about the cases, in which something is not rendercacheable.

Absolutely!


I think to make a decision here, we need to know where the links live that aren't being cached as part of a bigger structure (like menus, nodes, etc.). Do we have an idea about that? Could we collect that data automatically by using debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 20) and thus collect the callers of "uncached links"?

dawehner’s picture

I'm so tired of optimizing for the easy cases only.

jibran queued 242: 1965074-242.patch for re-testing.

catch’s picture

If we're able to replace path alias caching with this then it definitely makes sense regardless of caching elsewhere - since it'll be higher-level than the path alias caching by itself.

Not sure how many links we'll end up that aren't cached within menus or entities and yes that'd be good to know about.

xjm’s picture

Marking for reassessment based on the feedback from @efffulgentsia.

Fabianx’s picture

Assigned:Unassigned» Fabianx

I will give this a review as its green now.

I still think a semi-persistent router cache is good, because the larger structures:

a) have usually more granularity by design
b) this cache would not be used at all, when the things are already render cached, so this cache existing is still helpful as a backing of the uncached case.

The router and link generators have been one of the slowest components introduced so I think its important to not make them as fast as possible, where caching is one reason.

That said I don't know how much of my reasoning applies to the particular patch here in the issue.

Fabianx’s picture

Quick feedback:

- The idea to cache generated URLs per request is good, but the implementation needs work as this re-implements a cache collector, which we already have in core, which has the advantage of lazy loading the cache only when its needed. @see Drupal/Core/Cache/CacheCollectorInterface.php (lets hold off on porting that though, the implementation here also has a keys which can always just be one.)

- The good thing is that this has a global granularity. (I can't see any cache contexts it depends on except uncacheable for CSRF, which is fine) - however this is set back that it is unlikely to have a cache hit for anything not yet render cached.

On the render caching:

- Based on the granularity it can be that we need to re-generate some links again for different roles, but not sure if this would be on the same page already visited.

Question:

- Is it possible to cache one entry of a route per link OR is the route generation always dependent on the current request?

Rephrased:

Can we have (regardless that this cache might grew large for links never used for now) one cache item per generated route?

[ It might be also possible to e.g. use some CacheCollectorLRU or something like that, possibly only in APCu for most frequently generated links. ]

Thanks,

Fabian

Fabianx’s picture

Assigned:Fabianx» Unassigned
Berdir’s picture

I lost a bit track of what which of the various router/url/menu performance issues are doing, what problem they're solving and how they are related to each other.

Having a summary of those issues would be awesome, maybe in a new meta issue or the existing performance critical meta issue.

dawehner’s picture

jibran queued 242: 1965074-242.patch for re-testing.

catch’s picture

Priority:Critical» Major
Issue tags:-Needs D8 critical triage

I think everywhere that we'd render a link, we're doing render caching already.

On a render cache miss, between route pre-loading which we already do, and path alias caching which we already do, this won't provide enough of an additional optimization to be worth it. And it'll only kick in when the render cache is a miss but the url generation cache is a hit.

However, I do think it's worth doing if we can successfully replace the path alias cache with it - since that'd be caching at a higher level, and feels like the more appropriate place generally.

dawehner’s picture

+1
Agreed that this isn't critical.

rteijeiro’s picture

StatusFileSize
new34.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,594 pass(es).
[ View ]
new11.16 KB

Re-rolled and fixed a few nitpicks.

Fabianx’s picture

Status:Needs review» Needs work

Could someone please answer these questions:

Question:

- Is it possible to cache one entry of a route per link OR is the route generation always dependent on the current request?

Rephrased:

Can we have (regardless that this cache might grew large for links never used for now) one cache item per generated route?

[ It might be also possible to e.g. use some CacheCollectorLRU or something like that, possibly only in APCu for most frequently generated links. ]

effulgentsia’s picture

Issue tags:-Critical Office Hours

Since this issue is no longer critical, untagging for critical office hours. Don't know if we have or should start a "Major Office Hours" or similar tag.

Fabianx’s picture

Just a short ping to answer my own questions:

I think we can cache url generation with that at a much more granular level now that all Path processors specify cacheable metadata. (yeah!)

So the cache collector per page is no longer needed, but something that generates a link based on current url we could just choose to not cache.

That together with the alias routing could indeed speed up things for large lists of links.

Fabianx’s picture

Status:Needs work» Active

Setting back to active as the approach needs to be changed from collecting all links per request url, to just cache links in general and depend on their cacheable metadata.

pwolanin’s picture

Given the other page and block caching, and route pre-loading is the extra complexity of more caching worth it?

Fabianx’s picture

Issue tags:+Needs profiling

We should check it out for some list of things that is itself uncacheable (e.g. form with lots of links?) and see what it gives us.