Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.XHProf, a go to tool for profiling, actually impacts the profiling due to huge amount of function/method calls in Drupal 8. @msonnabaum suggested sampling as a lighter weight profiling technique. Initial results are promising. See the interactive SVG below, and annotated screenshot below that, where I list and circle potentially slow methods.
- The interactive flame graph
- Docs on how to read flame graphs. The gist is that you look for boxes that are wider than what you expect. Then you click on the box to more clearly see where the time is going, within that method. Hover over a box for sampling detail. Colors are not meaningful.
- Code for sampling and generating a flame graph
- This graph is taken from 1000 requests to the /user/password page of standard profile. I picked this page as it is relatively simple.
- Using PHP 5.4 with apc.stat=0 to reduce filesystem noise. Drupal is configured to use APC classloader.
service_container_prod::getStreamWrapperManagerService(). Perhaps a dead end as we take the inevitable hit of loading Module and DB classes here. Module loading can be moved to on-demand after #2353357: hook_stream_wrappers_alter() should be removed as it is broken since modules are not loaded on demand.service_container_prod::getRouterListenerService(). See #2368275: EntityRouteEnhancer and ContentFormControllerSubscriber implicitly depend on too many services.Drupal\Core\Block\BlockBase::access()See #2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager. More specifically:BlockBase::getConditionContexts()is the culprit. It should be statically cached, but isn't. It dispatches theBlockEvents::CONDITION_CONTEXTevent for every block. Epic fail. But also a super easy fix! :) On the default front page, with XDebug and XHProf disabled, this takes 6 to 8 milliseconds. Statically caching would bring this down to ~4.5. Of course, with more blocks, on actual sites, the difference would be much bigger.\Drupal\Core\Menu\MenuActiveTrail::getActiveTrailCacheKey(). Seems like an unoptimized query inMenuTreeStorage.- Drupal\Core\Routing\RoutePreloader::loadNonAdminRoutes (necessary probably to avoid other stuff)
menu_local_tasksspends a lot of time on initializing necessary services and then even more time on actually getting the local tasks. Hopefully #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees will bring salvation there.- Last but far from least:
UserLoginBlock! This is taking up a pretty large amount of the time (>2%). About half of that (1%) is spent on generating a link and finding the alias for a URL. For that, we have #1965074: Add cache wrapper to the UrlGenerator. The other half (1%) is just the building of a form being expensive. If we could render cache the login block, this would go away… and thanks to #2351015: URL generation does not bubble cache contexts we should be able to do that soonish.
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedComment #2
chx CreditAttribution: chx commentedComment #3
moshe weitzman CreditAttribution: moshe weitzman commentedComment #4
benjy CreditAttribution: benjy commentedComment #5
dawehnerComment #6
dawehnerComment #7
catchComment #8
Fabianx CreditAttribution: Fabianx commentedI just added support for XHProf-Lib / CLI to xhprof-kit, I will also add support for flamegraphs soon. This is really cool.
Comment #9
pwolanin CreditAttribution: pwolanin commentedMoshe points me to MenuTreeStorage::loadByRoute() as problematic.
Possible it would be faster here to do the sort in PHP (usually should just be 1 item), but the ORDER BY clauses may be confusing mysql.
And/or we should index the full length of route name?
Comment #10
Wim LeersI looked at the flamegraph in detail and added some more conclusions to the IS.
Comment #11
pwolanin CreditAttribution: pwolanin commentedQuick test with EXPLAIN suggests we should see if there is a clear benefit to pulling the loadByRoute() ordering into PHP, since it's usually 0 or 1 result, and hence a no-op.
Comment #12
Wim LeersLooks like #2370667: [Meta] user/password flame graph is addressing point 3: it makes the
/user/passwordpage 11% faster. AFAICT solely by dispatching the condition event only once instead of per block.Comment #13
Fabianx CreditAttribution: Fabianx commented#12: Is that link to the right issue?
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedThis one is :)
Comment #15
Wim LeersOops, sorry, yes.
Comment #16
Crell CreditAttribution: Crell commentedComment #17
moshe weitzman CreditAttribution: moshe weitzman commentedI have updated the flame graph for HEAD (no more HTMLFragment) - https://dl.dropboxusercontent.com/s/mc5ydaw137g63yd/result.svg?dl=0.
Still a top issue: #2368275: EntityRouteEnhancer and ContentFormControllerSubscriber implicitly depend on too many services
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedFYI, I posted a flame graph of just the bootstrap to #2380417: Bootstrap flame graph
Comment #19
Crell CreditAttribution: Crell commentedIf I am reading this correctly (and I may well not be), there may be some performance improvements to be had by speeding up the compiled Container, no? That code is all compiled off of the information provided to the container builder by upstream code.
In particular, it looks like any calls where a service is loaded that needs another service call back up through 2 layers of get() function, one of them ours. I think we could inline some more, no? (That would be best as a PR against Symfony if so.)
I am also not clear on the purpose of Drupal\Core\DependencyInjection\Container::get(). If a service is called but doesn't exist, that should fail with a message, no? So that we know what went wrong and how to fix it? Swallowing an error here seems like a bad idea and a slow-down, as it adds a stack call to every lookup of a service used by a service (which is, let's face it, a lot).
Comment #20
dawehnerNote: we do have an issue to micro-optimize that a bit already: #2307869: Remove Drupal's Container::get() but the actual problem seems to be that we do have quite some classes of all those dependencies which needs to be loaded.
That entirely depends on what you passed in as
$invalidBehaviourOne thing we can do, and we have a couple issues doing that already, is to mark services as not public. This will inline the service construction, indeed.
One of them is #2354705: Mark a couple of asset services as non public
Comment #21
Crell CreditAttribution: Crell commentedYes, if we have a lot of dependencies then there's 2 ways to make the container faster: Fewer dependencies or make the dependency lookup process faster. Both are worth pursuing, IMO.
It looks like our Container::get() is invalidating $invalidBehavior? Because instead of returning null in case of non-exception, you get a string. That... seems ill advised.
Comment #22
dawehnerPlease, discuss that in some other issue. But seriously, no, NULL is not an object ... you just return the result which is NULL already.
Comment #26
Wim Leers