When called the Drupal UrlGenerator, a lot of the work is delegated to \Symfony\Component\Routing\Generator:doGenerate()

This code is signifantly slower than the code in e..g the Drupal 7 url() function, in part since there are a number of regular expression calls (and more code). That means calls to the generator with a route instead of a (deprecated) system path are slower which will be bad for overall Drupal 8 speed.

Since we are enforcing path restrictions and other conventions that are stricter than what symfony allows, we should take advantage of those to make this faster. I addition, Drupal never uses host tokens, etc.

Proposed resolution - inline the symfony doGenerate code and remove the parts (like scheme handling) that are duplicate to the code in the Drupal UrlGenerator

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is a performance fix that does not resolve a specific bug.
Issue priority Major because this affects the performance of a frequently called method. Not critical because it is unlikely to improve performance by 100 ms for a cold cache or 10 ms for a warm cache (see #1744302-66: [meta] Resolve known performance regressions in Drupal 8).
Prioritized changes The main goal of this issue is performance. This is a prioritized change for the beta phase.
Disruption The only disruption introduced by this change would be the refactoring needed within the method itself and the risk of introducing other regressions.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

As long as we don't lose any functionality in the process and maintain parallelism with UrlMatcher (eg, anything that we can differentiate a route on for routing we should be able to differentiate on for generating, if applicable) then I'm fully OK with not extending the base UrlGenerator class and rolling our own entirely. That's the great thing about interfaces. :-)

dawehner’s picture

Component: menu system » routing system
Issue tags: +WSCCI

.

Fabianx’s picture

Priority: Major » Critical
Issue summary: View changes
Issue tags: +RC blocker, +Performance

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.

dawehner’s picture

@Fabianx
Please link to comments the next time in case you want to write down the same things down.

I don't know yet where exactly things are so much slower in the url generator but I assume that using the outbound path processors are part of the problem.

Fabianx’s picture

@dawehner: Thanks. Normally would do, but I will explain in private.

I assume so, too. It seems we can quite easily improve some things here, so lets do it.

dawehner’s picture

Related issue

xjm’s picture

Issue tags: -RC blocker
Wim Leers’s picture

#7: Why? Because #1965074: Add cache wrapper to the UrlGenerator should be sufficient?

Performance of UrlGenerator is abysmal currently, so if this issue helps us regain some speed, we should do it.

xjm’s picture

Priority: Critical » Major
Issue summary: View changes
Issue tags: +needs profiling

Discussed with @alexpott, @effulgentsia, and @dawehner. We don't actually have any data indicating what the performance impact of this specific optimization would be. Per #1744302-66: [meta] Resolve known performance regressions in Drupal 8, this issue would be critical if it improved performance by 100ms for a cold cache, or 10 ms with a warm cache (which will be possible with #1965074: Add cache wrapper to the UrlGenerator). Pending data that demonstrates that either would be the case, demoting to major.

Because this is a performance issue, it is a prioritized change during the beta phase and so can go forward assuming the impact is greater than the disruption.

Thanks @Fabianx for putting this back on the radar.

xjm’s picture

Issue summary: View changes
Wim Leers’s picture

Thanks, that's fair :)

Fabianx’s picture

dawehner’s picture

I haven't profiled it, but from just reading the code I would guess that

$compiledRoute = $route->compile();

is the crazy part. We don't have any host tokens for example, so that part of the code is never executed.

At least this part will be fixed in #2366043: Upgrade to Symfony 2.6.0-beta1.

On top of that we could maybe remove a bit of the logic in case we take over the full code, but whether that particular
part is worth to try, has to be evaluated.

larowlan’s picture

FileSize
1.2 KB

Sent here from #1965074: Add cache wrapper to the UrlGenerator

doGenerate is called multiple times for the same parameters in the same request.

Adding a static cache to see if that boosts.

Will profile later

webchick’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: do-generate-perf.1.patch, failed testing.

larowlan’s picture

Gold, pro tip: don't write patches on a Saturday night

dawehner’s picture

@larowlan
This is not about caching but rather about improving the actual code ... just saying.

larowlan’s picture

FileSize
858 bytes
1.2 KB

@dawehner - no worries, guess I'll need to dig deeper

I got mixed algorithms

larowlan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: do-generate-perf.2.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
886 bytes
1.23 KB

Fixing (hopefully) at least 2 of the failures.

hussainweb’s picture

The tests pass, but there were other class properties used by the doGenerate() method as well. Some don't really change the URL but change the behavior at least, e.g., $this->strictRequirements throws exceptions in certain cases. I don't know if we should account for that when generating the key as well.

pwolanin’s picture

This patch doesn't look related to the actual goal of the issue.

This patch should basically replace the underlying symfony UrlGenerator calls with Drupal ones that e.g. make assumptions such as all path slugs being full path segments.

pwolanin’s picture

Status: Needs review » Needs work

The last patch here should really be part of #1965074: Add cache wrapper to the UrlGenerator

pwolanin’s picture

Let me take a crack at this today

pwolanin’s picture

Clearly I didn't get to it last month - trying again.

pwolanin’s picture

Starting to dig into the code - looks like we actually need to define a route compiler class also, though I'm not sure where that gets set on the routes.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
12.08 KB

Here's a totally new patch that removes a lot of the code and a couple level of inheritance we don't need by directly and fully implementing the Drupal UrlGeneratorInterface.

Possibly need to merge with #22, though I think that patch is wrong since it doesn't account for the fact that the request context could be changed.

For context of what's removed in doGenerate() look at: https://github.com/symfony/symfony/blob/2.6/src/Symfony/Component/Routin...

Status: Needs review » Needs work

The last submitted patch, 30: 2074297-30.patch, failed testing.

Fabianx’s picture

Even if this fails tests, +1, thanks for your work on this Peter!

pwolanin’s picture

Status: Needs work » Needs review
FileSize
12.67 KB
1.35 KB

Minor - missing param should fix most of the errors.

Status: Needs review » Needs work

The last submitted patch, 33: 2074297-33.patch, failed testing.

dawehner’s picture


$url_generator = \Drupal::urlGenerator();
$url_generator->generateFromRoute('entity.node.canonical', ['node' => 2]);

$start = microtime(TRUE);
for ($i = 0; $i < 1000; $i++) {
  for ($j = 2; $j < 102; $j++) {
    $url_generator->generateFromRoute('entity.node.canonical', ['node' => $j]);
  }
}
$end = microtime(TRUE);

print_r($end - $start);

Before

3.5287961959839
3.5764729976654
3.6825568675995

After

3.2032999992371
3.1817519664764
3.0803670883179
3.1974589824677
pwolanin’s picture

Since those are all the same route, we only pay once for compiling. Though, certainly a reasonable comparison if you were; rendering a page of node links.

We should figure out how to compile a route every time and look at optimizing there and trying to avoid the preg_match of the variable value if we can.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
263.3 KB
1.35 KB

The update batch failed because the query string op=start from the batch system used to get merged with and treated as a route parameter. So, works if we actually pass the route parameter.

Also fixes the update test to use the proper route name instead of hack $GLOBAL and old file name.

pwolanin’s picture

FileSize
15.27 KB
1.35 KB

Oops - wrong diff - ignore #37

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

hussainweb’s picture

FileSize
3.51 KB
15.3 KB

Just fixing various code formatting issues.

pwolanin’s picture

@hussainweb - better a to post review, perhaps, so we can discuss each problem. In particular, I'm not sure we have a coding standard for these:

-      if ('variable' === $token[0]) {
+      if ($token[0] === 'variable') {

And better to leave them the way they were so it's easier to compare to the original symfony code I think?

There's also good reasons to have the variable on the right.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
    @@ -158,10 +158,10 @@ protected function doGenerate(array $variables, array $defaults, array $tokens,
    -          if (!preg_match('#^'.$token[2].'$#', $mergedParams[$token[3]])) {
    +          if (!preg_match('#^' . $token[2] . '$#', $mergedParams[$token[3]])) {
                 $message = sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given) to generate a corresponding URL.', $token[3], $name, $token[2], $mergedParams[$token[3]]);
                 throw new InvalidParameterException($message);
               }
    @@ -177,20 +177,22 @@ protected function doGenerate(array $variables, array $defaults, array $tokens,
    

    Super dump question ... why can't we check this on route dump time?

  2. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
    @@ -177,20 +177,22 @@ protected function doGenerate(array $variables, array $defaults, array $tokens,
     
    -    if ('' === $url) {
    +    if ($url === '') {
           $url = '/';
         }
     
    -    // the contexts base URL is already encoded (see Symfony\Component\HttpFoundation\Request)
    +    // The contexts base URL is already encoded (see Symfony\Component\HttpFoundation\Request).
         $url = strtr(rawurlencode($url), $this->decodedChars);
     
    -    // the path segments "." and ".." are interpreted as relative reference when resolving a URI; see http://tools.ietf.org/html/rfc3986#section-3.3
    +    // The path segments "." and ".." are interpreted as relative reference when
    +    // resolving a URI; see http://tools.ietf.org/html/rfc3986#section-3.3
         // so we need to encode them as they are not used for this purpose here
    -    // otherwise we would generate a URI that, when followed by a user agent (e.g. browser), does not match this route
    +    // otherwise we would generate a URI that, when followed by a user agent
    +    // (e.g. browser), does not match this route.
         $url = strtr($url, array('/../' => '/%2E%2E/', '/./' => '/%2E/'));
    -    if ('/..' === substr($url, -3)) {
    +    if (substr($url, -3) === '/..') {
           $url = substr($url, 0, -2).'%2E%2E';
    -    } elseif ('/.' === substr($url, -2)) {
    +    } elseif (substr($url, -2) === '/.') {
           $url = substr($url, 0, -1).'%2E';
         }
     
    @@ -199,7 +201,7 @@ protected function doGenerate(array $variables, array $defaults, array $tokens,
    
    @@ -199,7 +201,7 @@ protected function doGenerate(array $variables, array $defaults, array $tokens,
         if ($query_params && $query = http_build_query($query_params, '', '&')) {
           // "/" and "?" can be left decoded for better user experience, see
           // http://tools.ietf.org/html/rfc3986#section-3.4
    -      $url .= '?'.strtr($query, array('%2F' => '/'));
    +      $url .= '?' . strtr($query, array('%2F' => '/'));
    

    ... regarding all those changes ... they are inherited from the old parent class, so changing that would increase a bit the maintenance, we should think about it.

pwolanin’s picture

$token[2] is checking the value being substituted into the slug by the generator - it's not something static

hussainweb’s picture

@pwolanin, for #41, I remember reading somewhere that Yoda conditions are not in the coding standard, but I can't find it now. I did find this though: #2372543: [policy, no patch] Explicitly disallow yoda conditions.

I would have posted a review under normal circumstances but like I said, I remember that this was not as per the coding standard and straight away fixed it.

Fabianx’s picture

#44 This is completely different.

This is a class that is ported from upstream and simplified.

Therefore we can use the 'Coding Standards Exception for ported code to improve synchronization' rule.

We already have some files that use Symfony standard for that exact reason.

dawehner’s picture

Now that we have some code of the parent implementation I think we have to copy some of the tests as well. We probably don't have the proper test coverage of symfony now.

pwolanin’s picture

I don't care that strongly about the code style, or if we revert it.

I feel like our existing unit test is pretty thorough and we simply don't support some of the symfony features (host tokens, etc). Is there a specific gap in tests?

pwolanin’s picture

FileSize
15.2 KB

Conflict in core/modules/system/src/Tests/Update/UpdateScriptTest.php

Here's a straight re-roll.

pwolanin’s picture

FileSize
15.83 KB
3.22 KB

Doc cleanup, add doxygen, another minor optimization.

pwolanin’s picture

Using dawehner's test script above + drush scr:

with patch (seconds)
8.35
8.20
8.19

without patch - 8.0.x
11.81
11.94
11.88

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

So we talk about 1e1[s] / 1e5 = 1e-4 [s] = 1e-1 [ms] per generated URL, saving 30%
when you generate let's say 20 URLs isn't that bad, but its on the order of 1 [ms].

I guess for big listings like a view with 50 entries and multiple links, we would save much more.

pwolanin’s picture

1e-1 ms not µs, so roughly saving 1 ms?

Still, not a huge gain, but a step in the right direction.

dawehner’s picture

You are right, we talk about ms. Once you talk about units you should be absolute sure, you use the right ones.

Fabianx’s picture

RTBC + 1, computing we don't have to do is always a win!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. It does not seem to be obvious what is Drupal specific and what is Symfony in the new doGenerate method without having to read the symfony method.
  2. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
    @@ -8,23 +8,27 @@
     use Psr\Log\LoggerInterface;
    ...
    +use Symfony\Cmf\Component\Routing\RouteObjectInterface;
    

    Neither of these is used.

  3. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
    @@ -100,6 +131,100 @@ public function getPathFromRoute($name, $parameters = array()) {
    +   * @return string
    +   *   The url path, without any base path, including possible query string.
    ...
    +  protected function doGenerate(array $variables, array $defaults, array $tokens, array $parameters, array $query_params, $name) {
    

    We're documenting the return but not the params? I think we should doc the params.

  4. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
    @@ -100,6 +131,100 @@ public function getPathFromRoute($name, $parameters = array()) {
    +    // Tokens start from the end of the path and work to the beginning. The the
    

    The the

dawehner’s picture

It does not seem to be obvious what is Drupal specific and what is Symfony in the new doGenerate method without having to read the symfony method.

So we should clearly document that.

pwolanin’s picture

@alexpott - we mostly just removed symfony code that Drupal doesn't use. Might need to update the title/summary here.

pwolanin’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
18.44 KB
8.55 KB

Note that none of the code in doGenerate is Drupal specific - basically code was just removed from the symfony version.

Looking in more detail - we were not honestly documenting our support for Symfony\Cmf\Component\Routing\VersatileGeneratorInterface This change tries to make that more correct.

I'm really not even 100% sure we want to support that - is there even a good use case for supporting passing an instantiated route object into the generator? I think 100% of Drupal code passes in the string name.

Status: Needs review » Needs work

The last submitted patch, 58: 2074297-58.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
18.44 KB
639 bytes

oops - missed the Null class.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

This looks great to me! Back to RTBC! The code in doGenerate() is understandable and the copied from is clearly documented.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 95c2946 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 95c2946 on 8.0.x
    Issue #2074297 by pwolanin, hussainweb, larowlan: Optimize the code in...

Status: Fixed » Closed (fixed)

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