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.
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 the BlockEvents::CONDITION_CONTEXT event 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 in MenuTreeStorage.
menu_local_tasks spends 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.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 commentedComment #2
chx commentedComment #3
moshe weitzman commentedComment #4
benjy commentedComment #5
dawehnerComment #6
dawehnerComment #7
catchComment #8
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 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 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 commented#12: Is that link to the right issue?
Comment #14
effulgentsia commentedThis one is :)
Comment #15
wim leersOops, sorry, yes.
Comment #16
Crell commentedComment #17
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 commentedFYI, I posted a flame graph of just the bootstrap to #2380417: Bootstrap flame graph
Comment #19
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 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