Problem/Motivation

During install most of the time is spent building the container. See https://blackfire.io/profiles/0aae6482-d431-43e3-8bef-ee0916976585/graph...() which clearly shows the 14 module installs of a minimal install and their subsequent container rebuilds.

Our container by default registers a number of container passes to optimise the container. Considering we're going to throw the container away maybe the time spent making the optimisations is not worth it.

Proposed resolution

Remove InlineServiceDefinitionsPass, RemoveUnusedDefinitionsPass, AnalyzeServiceReferencesPass and ReplaceAliasByActualDefinitionPass during the installer do not do unnecessary work. This results in 1000s of less function calls and a quicker install. See the comparison for a standard install - https://blackfire.io/profiles/compare/612e435c-5c03-48b3-99a3-80c1846396...

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#9 3185768-5.patch2.17 KBalexpott
#7 3185768-9.0.x.patch4.62 KBalexpott
#6 3185768-8.9.x.patch3.52 KBalexpott
#5 3185768-5.patch2.17 KBalexpott
#3 3185768-3.patch1.43 KBalexpott
#2 3185768-2.patch1.62 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes
Status: Active » Needs review
FileSize
3185768-2.patch1.62 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3185768-3.patch1.43 KB

I think we can remove even more of these passes. Here's a comparison of the latest patch and installing standard - https://blackfire.io/profiles/compare/612e435c-5c03-48b3-99a3-80c1846396...

Here's a comparison of the standard install using time..

Without patch

Executed in    8.00 secs   fish           external
   usr time    5.01 secs   78.00 micros    5.01 secs
   sys time    1.11 secs  450.00 micros    1.11 secs

With patch

Executed in    6.55 secs   fish           external
   usr time    4.34 secs   81.00 micros    4.34 secs
   sys time    0.92 secs  555.00 micros    0.92 secs
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3185768-5.patch2.17 KB

Here's a slightly better way of removing the compiler passes. It means that Symfony's PassConfig can change and we'll get the benefits (if they are benefits). Also the ResolveHotPathPass pass is useless for Drupal. It would only work with Symfony's PHP dumper which do not use. We use our own dumper. On a minimal install this results in over 7000 less calls to Symfony\Component\DependencyInjection\Definition::hasTag() (taking it down to 324,594 :D)

I've not bothered with interdiffs because the same piece of code is changing again and again.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3185768-8.9.x.patch3.52 KB

Here's the equivalent patch for 8.9.x for anyone who wants to try it out there.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3185768-9.0.x.patch4.62 KB

And here's a 9.0.x patch ... the patch in #6 applies but that is for SF3 only.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

So after, the usual patch reroll party, I've updated our install profile with ~133 modules being installed to Drupal 9.1 and tested this.

With blackfire and xdebug enabled but not active, just "time" to report execution time:

D9.1 rc3 (-ish, 32 or so patches)
1m57.685s

+ this patch:
1m43.925s

That's nice. I suspect this is something that active profiling with blackfire massively overcounts as we're mostly saving a lot of very fast function calls. But even without that, if I got my basic math right, that's an improvement of 18%. That's great, but even not active, those extension are a massive performance hit. I disabled them to see just how much and was surprised...

Without the patch and those extensions, install time for me was merely 48s and the patch made it just ~2s (or less, results seem to vary by ~.5s) faster, that's ~4%, not 18%. Still nice and I can confirm that this seems to work fine for us, but clearly the first thing you should do if you want a fast install is disabling those extensions :)

Watching top while doing those installs shows that PHP CPU usage is between 50-90% and mysql up to 50%, so I suspect that the bottleneck is then mysql and IO to it more than PHP itself. It's worth pointing out that we create a lot of fields/tables (250) during install and we install a ton of default content and even generate some thumbnails with convert, that's all included in that time.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3185768-5.patch2.17 KB

Re-upping the 9.2.x patch. I still think a 4% boost on a massive site install like the one tested by @Berdir is a nice result. Plus standard and minimal are quicker.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
+++ b/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php
@@ -61,6 +66,24 @@ public function register(ContainerBuilder $container) {
+    $pass_config->setRemovingPasses(array_filter($pass_config->getRemovingPasses(), function ($pass) {

Do we need any of the "removing" passes if we are more interested in container build time than runtime performance?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Here's the list of passes that are still there... RemovePrivateAliasesPass, RemoveAbstractDefinitionsPass, DefinitionErrorExceptionPass

I think we need RemovePrivateAliasesPass because

They were only used to establish dependencies between services, and these dependencies have been resolved in one of the previous passes.

Therefore we're removing things we've added. I wouldn't we something in the installer to be abler to use one of these

I think a similar thing goes for the RemoveAbstractDefinitionsPass - although it's a bit more tenuous. I guess we can remove this pass - it's not that costly but it does mean we do less.

I think we need DefinitionErrorExceptionPass in order to report errors. Not sure about the impact of removing RemoveAbstractDefinitionsPass on that.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

This is still relevant and results in 1000s of less function calls for a standard install... https://blackfire.io/profiles/compare/822bbc6d-f165-4431-a59b-596f66b219...()

It doesn't really result in much of a speed up locally for me but in more constrained environments it will probably make a difference. Note with the performance timings on blackfire they need to be take with a pinch of slat because the thousands of function calls less result in bigger savings when blackfire is profiling.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

RemoveAbstractDefinitionsPass is very simple so let's not worry about a nano-optimisation here, to ensure we don't accidentally use an abstract service somehow.

DefinitionErrorExceptionPass also looks necessary in case there is an error in the dependency graph somewhere, so let's keep that too.

Therefore this is RTBC as it stands, we can always revisit later if we want to trim more off somewhere.

  • catch committed c671ff1 on 9.3.x 
    Issue #3185768 by alexpott, longwave, Berdir: Optimise container...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

This seems worth doing, apart from the time it's also noise removed if we're trying to find more installer optimisations. Being relatively conservative with which passes we removed seems sensible. Also putting this into 9.3.x-only for the same reason.

Committed c671ff1 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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