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
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. |
Comment | File | Size | Author |
---|---|---|---|
#60 | increment.txt | 639 bytes | pwolanin |
#60 | 2074297-60.patch | 18.44 KB | pwolanin |
#58 | increment.txt | 8.55 KB | pwolanin |
#58 | 2074297-58.patch | 18.44 KB | pwolanin |
#49 | increment-48-49.txt | 3.22 KB | pwolanin |
Comments
Comment #1
Crell CreditAttribution: Crell commentedAs 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. :-)
Comment #2
dawehner.
Comment #3
Fabianx CreditAttribution: Fabianx commentedOptimizing 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.
Comment #4
dawehner@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.
Comment #5
Fabianx CreditAttribution: Fabianx commented@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.
Comment #6
dawehnerRelated issue
Comment #7
xjmComment #8
Wim Leers#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.Comment #9
xjmDiscussed 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.
Comment #10
xjmComment #11
Wim LeersThanks, that's fair :)
Comment #12
Fabianx CreditAttribution: Fabianx commentedComment #13
dawehnerI haven't profiled it, but from just reading the code I would guess that
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.
Comment #14
larowlanSent 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
Comment #15
webchickComment #17
larowlanGold, pro tip: don't write patches on a Saturday night
Comment #18
dawehner@larowlan
This is not about caching but rather about improving the actual code ... just saying.
Comment #19
larowlan@dawehner - no worries, guess I'll need to dig deeper
I got mixed algorithms
Comment #20
larowlanComment #22
hussainwebFixing (hopefully) at least 2 of the failures.
Comment #23
hussainwebThe 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.Comment #24
pwolanin CreditAttribution: pwolanin commentedThis 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.
Comment #25
pwolanin CreditAttribution: pwolanin commentedThe last patch here should really be part of #1965074: Add cache wrapper to the UrlGenerator
Comment #26
pwolanin CreditAttribution: pwolanin commentedLet me take a crack at this today
Comment #27
dawehnerComment #28
pwolanin CreditAttribution: pwolanin commentedClearly I didn't get to it last month - trying again.
Comment #29
pwolanin CreditAttribution: pwolanin commentedStarting 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.
Comment #30
pwolanin CreditAttribution: pwolanin commentedHere'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...
Comment #32
Fabianx CreditAttribution: Fabianx commentedEven if this fails tests, +1, thanks for your work on this Peter!
Comment #33
pwolanin CreditAttribution: pwolanin commentedMinor - missing param should fix most of the errors.
Comment #35
dawehnerBefore
After
Comment #36
pwolanin CreditAttribution: pwolanin commentedSince 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.
Comment #37
pwolanin CreditAttribution: pwolanin commentedThe 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.
Comment #38
pwolanin CreditAttribution: pwolanin commentedOops - wrong diff - ignore #37
Comment #40
hussainwebJust fixing various code formatting issues.
Comment #41
pwolanin CreditAttribution: pwolanin commented@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:
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.
Comment #42
dawehnerSuper dump question ... why can't we check this on route dump time?
... 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.
Comment #43
pwolanin CreditAttribution: pwolanin commented$token[2]
is checking the value being substituted into the slug by the generator - it's not something staticComment #44
hussainweb@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.
Comment #45
Fabianx CreditAttribution: Fabianx commented#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.
Comment #46
dawehnerNow 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.
Comment #47
pwolanin CreditAttribution: pwolanin commentedI 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?
Comment #48
pwolanin CreditAttribution: pwolanin commentedConflict in core/modules/system/src/Tests/Update/UpdateScriptTest.php
Here's a straight re-roll.
Comment #49
pwolanin CreditAttribution: pwolanin commentedDoc cleanup, add doxygen, another minor optimization.
Comment #50
pwolanin CreditAttribution: pwolanin commentedUsing 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
Comment #51
dawehnerSo 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.
Comment #52
pwolanin CreditAttribution: pwolanin commented1e-1 ms not µs, so roughly saving 1 ms?
Still, not a huge gain, but a step in the right direction.
Comment #53
dawehnerYou are right, we talk about ms. Once you talk about units you should be absolute sure, you use the right ones.
Comment #54
Fabianx CreditAttribution: Fabianx commentedRTBC + 1, computing we don't have to do is always a win!
Comment #55
alexpottNeither of these is used.
We're documenting the return but not the params? I think we should doc the params.
The the
Comment #56
dawehnerSo we should clearly document that.
Comment #57
pwolanin CreditAttribution: pwolanin commented@alexpott - we mostly just removed symfony code that Drupal doesn't use. Might need to update the title/summary here.
Comment #58
pwolanin CreditAttribution: pwolanin commentedNote 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.
Comment #60
pwolanin CreditAttribution: pwolanin commentedoops - missed the Null class.
Comment #61
Fabianx CreditAttribution: Fabianx commentedThis looks great to me! Back to RTBC! The code in doGenerate() is understandable and the copied from is clearly documented.
Comment #62
alexpottCommitted 95c2946 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.