Problem/Motivation
I was doing a round of profiling on the simplest thing possible: a route that does
return new Response('hi!');
I'm doing this to see where baseline performance problems are.
Turns out initializing the route listener service takes ~20/25% of the response time, due to the mind boggling dependency tree that it comes with.
The critical path in that dependency tree is this:
@router_listener -> @router -> @router.no_access_checks -> @router.dynamic -> @route_enhancer.entity -> @form_builder -> @theme.manager -> @theme.initialization -> @theme_handler -> @asset.css.collection_optimizer -> @asset.css.optimizer
So we need the theme system to do routing when the entity route enhancer is present… (This also means the work dawehner et al. did in #2228093: Modernize theme initialization to not initialize the theme system on non-HTML routes was unfortunately only partially effective. All of that work still is valuable of course, it's just that the entity route enhancer is undoing quite a bit of that.)
This was introduced by #1874530: Add Symfony CMF Routing component ("route enhancers") and #2068471: Normalize Controller/View-listener behavior with a Page object (the entity route enhancer using the form builder service). Unfortunately, it makes it easy to unwantedly introduce huge additional dependency trees to the most fundamental service of all (routing).
Proposed resolution
- Move the HTML form controller and HTML entity form controller into services (like the other wrapper controllers)
- That means both controllers are initialized only when actually used
- route filters / enhancers running on every request will be handled in #2368769: All route enhancers are run on every request
Remaining tasks
TBD
User interface changes
None.
API changes
TBD
Comment | File | Size | Author |
---|---|---|---|
#35 | 2368275-35.patch | 13.26 KB | dawehner |
#35 | interdiff.txt | 5.4 KB | dawehner |
#32 | minroute-module-do-not-test.patch | 1.56 KB | znerol |
#24 | minroute-module-do-not-test.patch | 546 bytes | znerol |
#23 | interdiff-17-23.txt | 1017 bytes | martin107 |
Comments
Comment #1
Wim LeersThis became apparent by profiling with XHProf as follows:
This allowed me to clearly see which service initializations took the most time (inclusive wall time) because they have a big tree of dependencies.
Comment #2
znerol CreditAttribution: znerol commentedDoes this mean that anything in the critical path which depends on form builder will potentially nuke system performance?
Comment #3
Wim LeersYes.
Comment #4
catchOf the two options, if it's at all feasible I'd prefer option #2 (which reminds me of #1793520-7: Add access control mechanism for new router system two years ago...). That means an API change for route enhancers but there's not that many of them to convert.
Lazy services seems useful for things like logging, setting messages, CRUD operations - where a class might have a genuine dependency that's not required very often and where splitting up the class isn't helpful.
In this case the actual dependency chain is hidden behind event listeners, and it's the event listeners themselves that are unnecessary for the vast majority of routes. A route that just does
return new Respone('hi!');
by definition doesn't need the entity route enhancer or support for _entity_form.Comment #5
Gábor HojtsyThe only route enhancer change notice I found is https://www.drupal.org/node/2012100, but route enhancers predate that...
Comment #6
yched CreditAttribution: yched commentedSilly question: If there was work done to not initialize the theme system when not needed, wouldn't it be consistent to make @theme.manager or @theme.initialization the lazy service ?
Comment #7
webchickMinor issue summary tidying; let's stick to the problem at hand.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedComment #9
tim.plunkettSee also #2328871: Remove FormBuilder coupling to moduleHandler/themeManager.
The patch in #20 of that issue doesn't really help directly, but the patch in #6 would have.
Comment #10
Crell CreditAttribution: Crell commentedAn API change for Route Enhancers would be problematic, as those are an upstream concept from the CMF Routing component. We couldn't change that, only add another layer on it to delay execution.
Another option I hate to mention but will for completeness is to make EntityRouteEnhancer container aware. It only conditionally needs the services it uses, and in the case of FormBuilder isn't even using it; it's just passing it along to the wrapping HtmlEntityFormController. It doesn't use it itself.
In fact, looking at that class it appears all 3 of its dependencies exist only to be passed along to HtmlEntityFormController, which suggests that should be refactored anyway. I believe the best option would be to convert HtmlEntityFormController to a service (like FormController is) and let it handle its own dependencies. That would technically mean EntityRouteEnhancer has a hard-coded service name in it, but ContentControllerSubscriber does too for the same use case anyway. We should perhaps do the same for ContentFormControllerSubscriber/HtmlFormController, too, for the same reason.
(Also, side note: The provided test case is going to skip any work done in the view listener so is not an accurate representation of a full page request.
Comment #11
Crell CreditAttribution: Crell commentedAlso retitling; There's nothing special about route enhancers. They're just services. A service that depends on another service depends on all of that service's dependencies. That's just how code works. :-)
Comment #12
Crell CreditAttribution: Crell commentedComment #13
Crell CreditAttribution: Crell commentedSomething like this.
Turns out the dependency tree for all of the Form controller stuff was nutty. I was able to clean that up significantly. I also folded ContentFormControllerSubscriber into ContentControllerSubscriber, since once I was done refactoring it there were only 2 non-ceremony lines in it.
This should eliminate the "load all the things!" problem; the code is IMO cleaner and tidier as well, but the performance impact should be solely the load issue.
I'm a little curious why we're using a route enhancer for Entity routes but a request subscriber for the rest of them. I guess it works either way but we should normalize that, probably to an enhancer for all of them as the code is now quite simple and stand-alone.
I was very tempted to break EntityRouteEnhancerTest up into separate test methods while I was there; there's zero reason for them to all be separate, especially now. Follow-up.
Comment #15
catchThere is something special about them though. When a service is executed on every page request, and especially when it's in the critical path like routing, then every request has those dependencies. So any route enhancer with a long dependency chain is going to cause this performance issue, even if it only applies to an obscure admin path where otherwise you wouldn't care.
In 6.x/7.x where for example the _load()/%placeholder callbacks were by convention and hence stored (albeit implicitly) in the routing definition itself, this particular problem just doesn't exist, and taking the ->applies() route here would be closer to that.
Cleaning up the entity route enhancer will help though, so I've opened a separate issue to address route enhancers more generally, #2368769: All route enhancers are run on every request.
Comment #16
martin107 CreditAttribution: martin107 commentedI think the patch is a good step ... however it does not yet handle all cases .... yet
Instead of testing it is easy to push it over and get a stack trace, visit :-
/admin/config/development/logging ( _form: 'Drupal\system\Form\LoggingForm' )
/admin/config/development/performance ( _form: 'Drupal\system\Form\PerformanceForm' )
So form access is a little broken
I am sorry I am running out of time tonight, but in the spirit of post early partials... I have this comment
ContentControllerSubscriber::onRequestDeriveFormWrapper()
Stepping through the code as might be expected
$form equals 'Drupal\system\Form\PerformanceForm' when visiting /admin/config/development/performance AND $default['_content'] gets set BUT it is then promptly dumped on the floor - it goes nowhere as $defaults is a local variable!!!!!!
Anyway I will look more tomorrow.
Comment #17
martin107 CreditAttribution: martin107 commentedA) Routing using _form has been restored, to pages like /admin/config/development/logging
As Described in #16 the patch from #13 just needed a little nudge
ContentControllerSubscriber::onRequestDeriveFormWrapper() -- was not changing the state of the request :-
B) Minor housekeeping .... now that the unwanted dependencies have been stripped from EntityRouteEnhancer many 'use' statemnents can also be removed.
Comment #18
dawehner+1 for one less subscriber
I wonder why we don't use the route object using the route match, but it seems to be the thing in all that low level code.
Comment #19
catch#18/2 isn't that #2281619: Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead?
Comment #20
znerol CreditAttribution: znerol commented@dawehner: One subscriber less, but one more listener in another subscriber :)
@Wim Leers: Would you mind rerunning your benchmark with the patch applied?
Comment #21
znerol CreditAttribution: znerol commentedOverall the patch looks good. It is kind of weird that the
ContentControllerSubscriber
which according to the docs is used for "[...] setting the format of the request" is assigned more responsibility.Two nitpicks:
Remove one of the newlines.
No.
Comment #22
Crell CreditAttribution: Crell commentedI don't have a strong feeling about ContentControllerSubscriber; it just seemed silly to have an event subscriber class with only 2-3 non-ceremony lines in it that wasn't just bridging to a service. We could also update the docblock on the class if we want.
martin107: Wow, how did that work in my manual testing with such an obvious screw up? :-) Thanks for fixing.
Awaiting benchmarks from Wim or whoever is able to do so...
Comment #23
martin107 CreditAttribution: martin107 commentedMinor touches from #21.
Comment #24
znerol CreditAttribution: znerol commentedReproducing the test scenario outlined in the IS using the attached
minroute
module. The impact is as follows:The following services are not initialized if the patch is applied to head.
The overall performance benefit if measured with
ab -k -c1 -n500 http://localhost:3000/hi
:Without patch:
Time per request: 46.034 [ms] (mean)
With patch:
Time per request: 42.580 [ms] (mean)
Comment #25
alexpottLooking at the issue summary and the patch - I don't think we went for either option. Setting back to needs work to get an update. Also given @catch's interest in this issue we should assign to him once back to rtbc.
I think the current solution look good.
Comment #26
dawehnerUpdated the IS in order to understand what is going on.
Well, its okay, but it makes the patch a little bit harder to review
Comment #27
Crell CreditAttribution: Crell commentedTweaked the description a bit more; reassigning to catch as requested.
The array() => [] conversion dawehner notes is a quirk of my IDE, I think. Apparently PHPStorm has started converting array() to [] for me at times. *shrug*
Comment #28
catchWe don't have the phrase 'form definition string' anywhere in core yet, and it's not clear from the method docs what it is. Maybe an example would be enough, or is 'form definition string' really a specific thing? The param for getFormObject() is $form_arg, not $form_definition_string or $form_definition, so there's a mismatch.
Form is initialized but never used later in the method, no need to assign here.
Otherwise looks good.
Comment #29
Crell CreditAttribution: Crell commentedNote that this is going to have a not-difficult (I hope) conflict with #2376791: Move all _content routing definitions to _controller now. I just RTBCed that one so it probably wins.
I'll see if I can revisit this to find a better way to word "form definition string".
Comment #30
dawehnerWell, technically its a _form default value. The controller resolves names _controller as parameter
'Unable to look for the controller as the "_controller" parameter is missing'
.Comment #31
Wim Leers#21:
Agreed! #2352155: Remove HtmlFragment/HtmlPage fixed those docs :)
#24: Nice! 7.5% faster.
Also: your patch only lists the
git status
output, not the actual files of your testing module. Could you upload those? I think it could be handy for future profiling :)Note that the latest patch (in #23) is outdated anyway, #2352155: Remove HtmlFragment/HtmlPage has landed since and has changed
ContentControllerSubscriber
. Easy to fix, but the current patch doesn't apply.Super nitpicky nitpick: I thought we put a newline before
@return
?This method's documentation is definitely wrong.
Comment #32
znerol CreditAttribution: znerol commentedOh!
Comment #33
dawehnerOne really interesting bit about this patch is that it theoretically allows you much easier to not longer have all the special cases:
Comment #34
catchUnassigning me.
Comment #35
dawehnerRerolled the patch and fix some of the existing reviews.
One thing we could do in theory: Move that logic to add the
_controller
into route build time,let's do that in the future, unless someone disagrees.
Comment #36
dawehnerAdded a follow up for the latest comment: #2383675: Add _controller for _form and _entity_form/_entity_list
Comment #37
martin107 CreditAttribution: martin107 commentedComment #38
Wim Leers+1!
Why this rename?
Nice :)
Comment #39
dawehnerWell, the goal was to keep the variable and the method name more in sync:
Well, later is some code like
$form_arg = $this->getFormArgument
,Comment #40
Wim LeersMakes sense.
Plus, only now I notice it's
protected
… so then it's totally fine.I first RTBC'd, but then noticed there's just one nit left then, from #21:
… but actually, the class name already makes no sense anymore. This is now basically the
RequestRoutingSubscriber
, to handle the special/edge cases. The format handling is also going away from it. We can clean up the naming at any time.Comment #41
catchCommitted/pushed to 8.0.x, thanks!
Comment #43
dawehnerTotally agreed!
Awesome, another critical fixed.