Drupal rebuilds the router on each module installation. This is useful when adding modules once Drupal has been installed.

However, it is causing a lot of CPU cycles when Drupal is installing and the routes may be rebuilt over and over. Here's a screenshot from a Blackfire profile shaal created while we were debugging the Umami demo's installation time. The router is rebuilt 55 times before Drupal has finished being installed.

Here is the current code:

      if (!\Drupal::service('router.route_provider.lazy_builder')->hasRebuilt()) {
        // Rebuild routes after installing module. This is done here on top of
        // \Drupal\Core\Routing\RouteBuilder::destruct to not run into errors on
        // fastCGI which executes ::destruct() after the module installation
        // page was sent already.
        \Drupal::service('router.builder')->rebuild();
      }

Adding a check to drupal_installation_attempted () greatly improves the installation time. Here is the comparison when we check if Drupal is installed first

      if (!drupal_installation_attempted() && !\Drupal::service('router.route_provider.lazy_builder')->hasRebuilt()) {
        // Rebuild routes after installing module. This is done here on top of
        // \Drupal\Core\Routing\RouteBuilder::destruct to not run into errors on
        // fastCGI which executes ::destruct() after the module installation
        // page was sent already.
        \Drupal::service('router.builder')->rebuild();
      }
CommentFileSizeAuthor
#56 drupal-core-improve-installer-performance-8.9.x-backport-3086307-56.patch20.99 KBmxr576
#48 3086307-48.patch20.96 KBalexpott
#48 45-48-interdiff.txt3.28 KBalexpott
#45 3086307-45.patch20.87 KBalexpott
#45 42-45-interdiff.txt2.44 KBalexpott
#42 3086307-42.patch20.73 KBalexpott
#42 41-42-interdiff.txt8.32 KBalexpott
#41 3086307-41.patch14.48 KBalexpott
#41 36-41-interdiff.txt1.53 KBalexpott
#36 3086307-36.patch14.31 KBalexpott
#36 33-36-interdiff.txt13.38 KBalexpott
#34 3086307-33.patch2.59 KBalexpott
#24 3086307-24.patch6.52 KBtstoeckler
#24 3086307-22-24-interdiff.txt733 byteststoeckler
#22 3086307-22.patch5.81 KBtstoeckler
#12 interdiff_4-12.txt1.49 KBshaal
#12 core-no-rebuild-router-during-installation-3086307-12.patch1.21 KBshaal
#6 blackfire-comparison.png8.09 KBshaal
#6 blackfire-after-patch-4.png8.92 KBshaal
#6 blackfire-before-patch.png8.89 KBshaal
#4 interdiff_2-4.txt1.13 KBshaal
#4 core-no-rebuild-router-during-installation-3086307-4.patch1.21 KBshaal
#2 3086307-2.patch989 bytesmglaman
Screen Shot 2019-10-07 at 10.08.01 AM.png77.57 KBmglaman
Screen Shot 2019-10-07 at 10.03.37 AM.png73.61 KBmglaman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mglaman created an issue. See original summary.

mglaman’s picture

Status: Active » Needs review
FileSize
989 bytes

This adds the installation check before rebuilding routes.

Status: Needs review » Needs work

The last submitted patch, 2: 3086307-2.patch, failed testing. View results

shaal’s picture

Fixing deprecation warning in tests.

mglaman’s picture

I don't know how reliable the times are... but

#4: 58 min build duration;
8.8.x commit test: 1 hr 1 min build duration;
random issue: 59 min build duration;

so it might not be easy to test based on DrupalCI build times.

shaal’s picture

I ran some blackfire.io tests using DDEV.
time blackfire run drush8 si demo_umami -y

1) Blackfire graph without the patch:
https://blackfire.io/profiles/a8f30835-bec9-4e70-90d9-cb3737f133c8/graph

2) Blackfire graph with patch: #4
https://blackfire.io/profiles/fe5231f0-b122-4109-bfeb-5b7d7685d643/graph

3) Blackfire comparison graph between the 2 previous runs
https://blackfire.io/profiles/compare/5693f6b6-85b4-41f7-9369-e1ef12525f...

TLDR: it took 12% less time for demo_umami to install after applying patch #4.

Berdir’s picture

I'll try to test this as well a bit, a bit worried that it might break some weird module that tries to access routes e.g. in hook_install(), a think a bunch of them tend to do that.

One thing that I would suggest is to also just compare "time drush8 si demo_umami -y". blackfire, like any profiler, tends to increase fast function call costs a lot, I've seen total time execution growing by 3x or more in tests. Just the total time tends to be a better comparison. depending on how many fast function calls are inside the route rebuild vs outside, it speed it up less or theoretically even more.

Berdir’s picture

Ok, two positive results from testing.

a) Our massive install profile with 133 enabled extensions at the end installed just fine with with this patch, no errors. Only tested throug drush and not the UI, though.

b) Install time decreased significantly.

Before: 2m46.589s
After: 2m6.020s

That's 25% faster.

mglaman’s picture

One thing that I would suggest is to also just compare "time drush8 si demo_umami -y". blackfire, like any profiler, tends to increase fast function call costs a lot,

I've considered this, as well. The main thing I noticed is the call count went from 55 to 4.

I'll try to test this as well a bit, a bit worried that it might break some weird module that tries to access routes e.g. in hook_install(), a think a bunch of them tend to do that.

Is routing considered "stable" in hook_install? Also, this is called after that I believe. So hook_modules_installed may be the hook most affected.

That's 25% faster.

😱oh my, that is nice

andypost’s picture

25% is impressive!
As CR said install hooks must rebuild router if they need fresh list
So this looks more like bugfix

andypost’s picture

shaal’s picture

re-rolled patch #4

Testing real time installation (without blackfire)
drush si demo_umami -y

before -
97 seconds

after this patch -
84 seconds

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

chr.fritsch’s picture

I tried this patch with Thunder 3.
Sadly I was not able to pass the installation. It failed with

 [notice] Starting Drupal installation. This takes a while.

In SqlContentEntityStorage.php line 847:

  Route "entity.user.canonical" does not exist.


In RouteProvider.php line 208:

  Route "entity.user.canonical" does not exist.

Berdir’s picture

Can you try to get a full backtrace for this? SqlContentEntityStorage.php:847 is just where we catch the original exception and re-throw it, try a print (string) $e; or so there.

chr.fritsch’s picture

Sure. Here it is. Not installing the pathauto module would fix it. Performance improvement is then from 1:39m to 1:07m

Symfony\Component\Routing\Exception\RouteNotFoundException: Route "entity.user.canonical" does not exist. in /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Routing/RouteProvider.php:208
Stack trace:
#0 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Routing/UrlGenerator.php(426): Drupal\Core\Routing\RouteProvider->getRouteByName('entity.user.can...')
#1 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Routing/UrlGenerator.php(130): Drupal\Core\Routing\UrlGenerator->getRoute('entity.user.can...')
#2 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php(68): Drupal\Core\Routing\UrlGenerator->getPathFromRoute('entity.user.can...', Array)
#3 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Url.php(791): Drupal\Core\Render\MetadataBubblingUrlGenerator->getPathFromRoute('entity.user.can...', Array)
#4 /Users/d430774/Sites/thunder/thunder3/docroot/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php(31): Drupal\Core\Url->getInternalPath()
#5 /Users/d430774/Sites/thunder/thunder3/docroot/modules/contrib/pathauto/src/PathautoFieldItemList.php(33): Drupal\path\Plugin\Field\FieldType\PathFieldItemList->computeValue()
#6 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php(34): Drupal\pathauto\PathautoFieldItemList->computeValue()
#7 /Users/d430774/Sites/thunder/thunder3/docroot/modules/contrib/pathauto/src/PathautoFieldItemList.php(15): Drupal\path\Plugin\Field\FieldType\PathFieldItemList->ensureComputedValue()
#8 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Field/FieldItemList.php(191): Drupal\pathauto\PathautoFieldItemList->delegateMethod('preSave')
#9 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(881): Drupal\Core\Field\FieldItemList->preSave()
#10 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(831): Drupal\Core\Entity\ContentEntityStorageBase->invokeFieldMethod('preSave', Object(Drupal\user\Entity\User))
#11 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Entity/EntityStorageBase.php(500): Drupal\Core\Entity\ContentEntityStorageBase->invokeHook('presave', Object(Drupal\user\Entity\User))
#12 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(700): Drupal\Core\Entity\EntityStorageBase->doPreSave(Object(Drupal\user\Entity\User))
#13 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Entity/EntityStorageBase.php(454): Drupal\Core\Entity\ContentEntityStorageBase->doPreSave(Object(Drupal\user\Entity\User))
#14 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(838): Drupal\Core\Entity\EntityStorageBase->save(Object(Drupal\user\Entity\User))
#15 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Entity/EntityBase.php(395): Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object(Drupal\user\Entity\User))
#16 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php(315): Drupal\Core\Entity\EntityBase->save()
#17 [internal function]: Drupal\Core\Installer\Form\SiteConfigureForm->submitForm(Array, Object(Drupal\Core\Form\FormState))
#18 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Form/FormSubmitter.php(112): call_user_func_array(Array, Array)
#19 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Form/FormSubmitter.php(52): Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object(Drupal\Core\Form\FormState))
#20 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Form/FormBuilder.php(591): Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object(Drupal\Core\Form\FormState))
#21 /Users/d430774/Sites/thunder/thunder3/docroot/modules/contrib/autosave_form/src/Form/AutosaveFormBuilder.php(144): Drupal\Core\Form\FormBuilder->processForm('install_configu...', Array, Object(Drupal\Core\Form\FormState))
#22 /Users/d430774/Sites/thunder/thunder3/docroot/core/lib/Drupal/Core/Form/FormBuilder.php(487): Drupal\autosave_form\Form\AutosaveFormBuilder->processForm('install_configu...', Array, Object(Drupal\Core\Form\FormState))
#23 /Users/d430774/Sites/thunder/thunder3/docroot/core/includes/install.core.inc(971): Drupal\Core\Form\FormBuilder->submitForm('Drupal\\Core\\Ins...', Object(Drupal\Core\Form\FormState))
#24 /Users/d430774/Sites/thunder/thunder3/docroot/core/includes/install.core.inc(625): install_get_form('Drupal\\Core\\Ins...', Array)
#25 /Users/d430774/Sites/thunder/thunder3/docroot/core/includes/install.core.inc(578): install_run_task(Array, Array)
#26 /Users/d430774/Sites/thunder/thunder3/docroot/core/includes/install.core.inc(117): install_run_tasks(Array, NULL)
#27 /Users/d430774/Sites/thunder/thunder3/vendor/drush/drush/includes/drush.inc(229): install_drupal(Object(Composer\Autoload\ClassLoader), Array)
#28 /Users/d430774/Sites/thunder/thunder3/vendor/drush/drush/includes/drush.inc(214): drush_call_user_func_array('install_drupal', Array)
#29 /Users/d430774/Sites/thunder/thunder3/vendor/drush/drush/src/Commands/core/SiteInstallCommands.php(145): drush_op('install_drupal', Object(Composer\Autoload\ClassLoader), Array)
#30 [internal function]: Drush\Commands\core\SiteInstallCommands->install('thunder', Array)
#31 /Users/d430774/Sites/thunder/thunder3/vendor/consolidation/annotated-command/src/CommandProcessor.php(257): call_user_func_array(Array, Array)
#32 /Users/d430774/Sites/thunder/thunder3/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
#33 /Users/d430774/Sites/thunder/thunder3/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#34 /Users/d430774/Sites/thunder/thunder3/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(302): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#35 /Users/d430774/Sites/thunder/thunder3/vendor/symfony/console/Command/Command.php(255): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#36 /Users/d430774/Sites/thunder/thunder3/vendor/symfony/console/Application.php(1000): Symfony\Component\Console\Command\Command->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#37 /Users/d430774/Sites/thunder/thunder3/vendor/symfony/console/Application.php(255): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#38 /Users/d430774/Sites/thunder/thunder3/vendor/symfony/console/Application.php(148): Symfony\Component\Console\Application->doRun(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#39 /Users/d430774/Sites/thunder/thunder3/vendor/drush/drush/src/Runtime/Runtime.php(118): Symfony\Component\Console\Application->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#40 /Users/d430774/Sites/thunder/thunder3/vendor/drush/drush/src/Runtime/Runtime.php(49): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
#41 /Users/d430774/Sites/thunder/thunder3/vendor/drush/drush/drush.php(72): Drush\Runtime\Runtime->run(Array)
#42 /Users/d430774/Sites/thunder/thunder3/vendor/drush/drush/includes/preflight.inc(18): require('/Users/d430774/...')
#43 phar:///usr/local/bin/drush/bin/drush.php(141): drush_main()
#44 /usr/local/bin/drush(10): require('phar:///usr/loc...')
#45 {main}
Berdir’s picture

Hm, I also have pathauto, not for users though. But that doesn't seem related to pathauto, it's just passed throuh to its methods as we override computeValue(), but then just continues to the parent? I don't quite see why disable pathauto fixes that.

mglaman’s picture

Status: Needs review » Needs work

Interesting for #16. I wonder if that means the Umami demo would also raise exceptions during installation if pathauto was configured (because it generates content during the install process, which would run pathauto.)

When saving a non-new entity, the PathFieldItemList tries to build a route:

    if (!$entity->isNew()) {
      $conditions = [
        'source' => '/' . $entity->toUrl()->getInternalPath(),
        'langcode' => $this->getLangcode(),
      ];

So if any content is generated during the installation which sets a path, this error would occur.

Perhaps we should adjust the patch to have routes rebuild at the end of each of these install tasks?

    'install_profile_modules' => [
      'display_name' => t('Install site'),
      'type' => 'batch',
    ],
    'install_profile_themes' => [],
    'install_install_profile' => [],

Or does this prove there may be unintended consequences.

mglaman’s picture

Actually, I wonder if an implementation of \Drupal\Core\Entity\EntityTypeListenerInterface::onEntityTypeCreate just needs to rebuild routes. Then new entity routes provided by route providers would exist, solving the pathauto problem.

Proposal:

The patch as is, and a new event subscriber for routing that subscribes to \Drupal\Core\Entity\EntityTypeEvents::CREATE. There is \Drupal\Core\Entity\EntityTypeEventSubscriberTrait available to simplify the subscribe.

On \Drupal\Core\Entity\EntityTypeEventSubscriberTrait::onEntityTypeCreate and \Drupal\Core\Entity\EntityTypeEventSubscriberTrait::onEntityTypeDelete we should rebuild the router.

andypost’s picture

While working on help topics stabilization (search module integration) we faced that sometimes install/uninstall caused fatals on route rebuild
I did not touch it deep but we filed separate issue #2901590: Investigate route rebuilding problem wrt Search module

effulgentsia’s picture

Title: Do not rebuild the router for each module installation if Drupal is being installed » Improve installer performance by ~20% by rebuilding the router after the entire installation is complete rather than after each module
Category: Feature request » Task
Priority: Normal » Major
Issue tags: +Performance

IMO, ~20% (average of #8 and #12) is a significant enough improvement to warrant raising the priority to Major, including that in the issue title, and tagging it as a Performance issue.

Also categorizing as a task due to the information that's in #10. If/when we discover other BC considerations, let's add them to the issue summary.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
5.81 KB

Another possible solution would be to not rebuild the router during installation at all, but catch a RouteNotFoundException from within RouteProvider::getRouteByName() and then rebuild the router on-demand. This assumes that a) we don't catch the RouteNotFoundException in ::getRouteByName() from within the installer (or at least not a lot) as the additional rebuild would be bad for performance then and b) that even in complex scenarios the number of times a missing route will be encountered (as is the case with Pathauto per the above) is small enough that the additional cost of throwing the exception is outweighed by the saved router rebuilds.

Here's a WIP patch that does that. I verified that it fixes the issue with Pathauto noted above, but I didn't do any performance tests.

Status: Needs review » Needs work

The last submitted patch, 22: 3086307-22.patch, failed testing. View results

tstoeckler’s picture

Hmm... I guess there can be the case then when we never actually get around to rebuilding the router. Not sure why only the multilingual test is failing, but this should hopefully fix it. Still WIP, but hopefully something that can be used to validate any potential performance benefits.

mglaman’s picture

Circling back, I just encountered #14 with the patch from #12.

Due to preSave op on a User and ensureComputedValue fro Drupal\\pathauto\\PathautoFieldItemList. It's not due to pathauto, though.

    if (!$entity->isNew()) {
      /** @var \Drupal\path_alias\AliasRepositoryInterface $path_alias_repository */
      $path_alias_repository = \Drupal::service('path_alias.repository');

      if ($path_alias = $path_alias_repository->lookupBySystemPath('/' . $entity->toUrl()->getInternalPath(), $this->getLangcode())) {
        $value = [
          'alias' => $path_alias['alias'],
          'pid' => $path_alias['id'],
          'langcode' => $path_alias['langcode'],
        ];
      }
    }

This is in the path module's path field list class.

Rajab Natshah’s picture

Patch #24 is working with Drupal 8.8.0
Thank you

Rajab Natshah’s picture

Status: Needs review » Needs work

After a number of testing cases.

Reverted this patch #24 and #12
Waiting to untile it been committed to Drupal core
Extra modules which been installed in profiles may run into a number of issues
like devel, external links, dashboard, toolbar links

It could be the type of routes, dynamic, static, ...

I like this patch very much, It saves time on development with distributions!!

mglaman’s picture

Following up here. Pathauto kills this when `user` is an enabled type. For my Pathauto borks when SiteConfigureForm saves uid 1

'#0 d8/web/core/lib/Drupal/Core/Routing/UrlGenerator.php(426): Drupal\\Core\\Routing\\RouteProvider->getRouteByName(\'entity.user.can...\')
#1 d8/web/core/lib/Drupal/Core/Routing/UrlGenerator.php(130): Drupal\\Core\\Routing\\UrlGenerator->getRoute(\'entity.user.can...\')
#2 d8/web/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php(68): Drupal\\Core\\Routing\\UrlGenerator->getPathFromRoute(\'entity.user.can...\', Array)
#3 d8/web/core/lib/Drupal/Core/Url.php(791): Drupal\\Core\\Render\\MetadataBubblingUrlGenerator->getPathFromRoute(\'entity.user.can...\', Array)
#4 d8/web/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php(32): Drupal\\Core\\Url->getInternalPath()
#5 d8/web/modules/contrib/pathauto/src/PathautoFieldItemList.php(33): Drupal\\path\\Plugin\\Field\\FieldType\\PathFieldItemList->computeValue()
#6 d8/web/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php(34): Drupal\\pathauto\\PathautoFieldItemList->computeValue()
#7 d8/web/modules/contrib/pathauto/src/PathautoFieldItemList.php(15): Drupal\\path\\Plugin\\Field\\FieldType\\PathFieldItemList->ensureComputedValue()
#8 d8/web/core/lib/Drupal/Core/Field/FieldItemList.php(191): Drupal\\pathauto\\PathautoFieldItemList->delegateMethod(\'preSave\')
#9 d8/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(881): Drupal\\Core\\Field\\FieldItemList->preSave()
#10 d8/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(831): Drupal\\Core\\Entity\\ContentEntityStorageBase->invokeFieldMethod(\'preSave\', Object(Drupal\\user\\Entity\\User))
#11 d8/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(500): Drupal\\Core\\Entity\\ContentEntityStorageBase->invokeHook(\'presave\', Object(Drupal\\user\\Entity\\User))
#12 d8/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(700): Drupal\\Core\\Entity\\EntityStorageBase->doPreSave(Object(Drupal\\user\\Entity\\User))
#13 d8/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(454): Drupal\\Core\\Entity\\ContentEntityStorageBase->doPreSave(Object(Drupal\\user\\Entity\\User))
#14 d8/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(837): Drupal\\Core\\Entity\\EntityStorageBase->save(Object(Drupal\\user\\Entity\\User))
#15 d8/web/core/lib/Drupal/Core/Entity/EntityBase.php(395): Drupal\\Core\\Entity\\Sql\\SqlContentEntityStorage->save(Object(Drupal\\user\\Entity\\User))
#16 d8/web/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php(315): Drupal\\Core\\Entity\\EntityBase->save()
#17 [internal function]: Drupal\\Core\\Installer\\Form\\SiteConfigureForm->submitForm(Array, Object(Drupal\\Core\\Form\\FormState))
#18 d8/web/core/lib/Drupal/Core/Form/FormSubmitter.php(112): call_user_func_array(Array, Array)
#19 d8/web/core/lib/Drupal/Core/Form/FormSubmitter.php(52): Drupal\\Core\\Form\\FormSubmitter->executeSubmitHandlers(Array, Object(Drupal\\Core\\Form\\FormState))
#20 d8/web/core/lib/Drupal/Core/Form/FormBuilder.php(591): Drupal\\Core\\Form\\FormSubmitter->doSubmitForm(Array, Object(Drupal\\Core\\Form\\FormState))
#21 d8/web/core/lib/Drupal/Core/Form/FormBuilder.php(487): Drupal\\Core\\Form\\FormBuilder->processForm(\'install_configu...\', Array, Object(Drupal\\Core\\Form\\FormState))
#22 d8/web/core/includes/install.core.inc(970): Drupal\\Core\\Form\\FormBuilder->submitForm(\'Drupal\\\\Core\\\\Ins...\', Object(Drupal\\Core\\Form\\FormState))
#23 d8/web/core/includes/install.core.inc(624): install_get_form(\'Drupal\\\\Core\\\\Ins...\', Array)
tstoeckler’s picture

Re #27 and #28: Are the errors with patch #24? That is unclear to me.

Started some very basic profiling with Xdebug on this on the weekend. I only got so far, and the concrete times are not reliable because of Xdebug itself but also because I was doing other things on the machine while profiling and I only did one run each vs. having an average of multiple identical runs, etc. I do still think it's possible to make an assessment going by the relative numbers, however.

Overall installing the Umami profiles lead to 55 less router rebuilds with the patch. In 8.8.x RouteBuilder::rebuild() is called 57 times (which makes sense because Umami installs 50+ modules) which makes up 10.0% of execution time while with the patch NormalInstallerRouteBuilder::forceRebuild() is called only 2 times which makes up 1.6% of execution time. This in itself is about an 8.5% reduction of the total execution time. The execution time as reported by Xdebug drops by 10%.

I am assuming that most actual sites have at least that many modules and I assume that many sites have double the amount of modules due to contrib and custom modules. That would make the savings even more significant.

So would still love to see some less handwavy profiling from someone more adept at this and maybe some input from people working on bigger distributions, but I think that at least validates that the patch does what it says on the label and provides a significant reduction of installation time.

And of course would love to have some input on the concrete approach, as well.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

catch’s picture

If we're concerned about hook_modules_installed(), we could try to do the same thing that's proposed in #3115543: Defer running of hook_modules_installed() during updates and defer it to after the final route rebuild.

Note that #3157866: Remove unnecessary route rebuild from installer is going in the opposite direction (also for a performance improvement) but that's not really incompatible - we'd have to revert that patch in here and it gives us a better optimised baseline to compare against.

alexpott’s picture

Yay for more profiling of the installer. Also we should look to see how #2488350: Switch to a memory cache backend during installation has affected this.

I think we also should do #3157866: Remove unnecessary route rebuild from installer and #3157954: Remove unnecessary route rebuild from tests first.

alexpott’s picture

I really like the idea of deferring hook_modules_installed until we install the profile. That should be when the router is properly built for the first time during an install.

I think there might be a neater way to achieve what we want here. During module install we swap out the route provider for the lazy builder... \Drupal::getContainer()->set('router.route_provider', \Drupal::service('router.route_provider.lazy_builder'));

What we could do is to this in the \Drupal\Core\Installer\NormalInstallerServiceProvider and then routes would only be rebuilt during an install when someone tries to use a route.

alexpott’s picture

Here's a build that tries to avoid the rebuilds using the lazy router provider all the time during the installer. It's a different approach so no interdiff. With this patch installing umami goes from 57 route rebuilds to 4. And we can definitely reduce that by one with some of #3157866: Remove unnecessary route rebuild from installer.

tstoeckler’s picture

Wow, very impressive! That is certainly a lot nicer than #24.

alexpott’s picture

So #34 was not quite right - but the idea works :)

Here's a fixed patch with testing for interactive and non-interactive installs.

Still need to add a test for accessing a route in a hook_modules_installed() implementation during an install.

alexpott’s picture

Without the decorated service in NormalInstallerServiceProvider and ModuleInstaller...

InstallerTest fails (installing testing install profile):

1) Drupal\FunctionalTests\Installer\InstallerTest::testInstaller
Failed asserting that 8 is identical to 2.

InstallerPerformanceTest fails (installing standard install profile):

1) Drupal\FunctionalTests\Installer\InstallerPerformanceTest::testInstaller
Failed asserting that 46 is identical to 3.

Status: Needs review » Needs work

The last submitted patch, 36: 3086307-36.patch, failed testing. View results

Berdir’s picture

@alexpott is it possible that the number of rebuilds depends on the number of batch steps we have? that would make for an ugly random fail.

alexpott’s picture

@Berdir I don't think so. We're only rebuilding if the routing system is actually used. I don't think \Drupal\Core\Routing\RouteBuilder::destruct() is not called at all during installation.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
14.48 KB

Fixing the test fail.

alexpott’s picture

And here we go it is possible to only build the router once during an instal :) - with this patch the standard install will only do it once.

mglaman’s picture

@alexpott this looks great 🙌

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    --- a/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php
    +++ b/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php
    
    +++ b/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php
    @@ -49,6 +51,17 @@ public function register(ContainerBuilder $container) {
    +    // Don't register the lazy route provider in the super early installer.
    +    if (get_called_class() === NormalInstallerServiceProvider::class) {
    

    Curious why we need this? Aren't we in NormalInstallerServiceProvider? Is this just in case someone has swapped the service,

    per

      // Remove the service provider of the early installer.
      unset($GLOBALS['conf']['container_service_providers']['InstallerServiceProvider']);
    
      // Add the normal installer service provider.
      $GLOBALS['conf']['container_service_providers']['InstallerServiceProvider'] = 'Drupal\\Core\\Installer\\NormalInstallerServiceProvider';
    
     

    But even then, do we need to check if it was this exact class?

  2. +++ b/core/lib/Drupal/Core/Routing/RouteProviderLazyBuilder.php
    @@ -81,6 +82,12 @@ public function getRouteCollectionForRequest(Request $request) {
    +      // to determine the front page. At this point builder the router for this
    +      // is unnecessary work.
    

    s/builder/building ?

    I also tripped up "the router for this is.."

    Maybe "building the router for this route is unnecessary work"?

Status: Needs review » Needs work

The last submitted patch, 42: 3086307-42.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
20.87 KB

Thanks for the review @mglaman.

Fixing the broken test and addressing #44.2

re #44.1 This is because of \Drupal\Core\Installer\InstallerServiceProvider - which is for the super-super early installer (before the system module is around). This is why #36 failed - because using this route provider there results in the router rebuild from being fired - and the menu tree being created :) - this will happen on the redirect to core/install.php from the frontpage.

borisson_’s picture

I had two small questions, but I'm not sure if they are relevant. If they aren't this is rtbc as far as I'm concerned.

  1. +++ b/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php
    @@ -49,6 +51,17 @@ public function register(ContainerBuilder $container) {
    +    // Don't register the lazy route provider in the super early installer.
    

    If we add another installer next to super early and normal, this comment would be outdated. Do we need to change this comment?

  2. +++ b/core/lib/Drupal/Core/Routing/RouteProviderLazyBuilder.php
    @@ -81,6 +82,12 @@ public function getRouteCollectionForRequest(Request $request) {
       public function getRouteByName($name) {
    +    if (InstallerKernel::installationAttempted() && ($name === '<none>' || $name === '<front>')) {
    +      // During the installer template_preprocess_page() uses the routing system
    +      // to determine the front page. At this point building the router for this
    +      // is unnecessary work.
    +      return new Route('/');
    +    }
    

    This will increase performance during install time, but are we sure that does not decrease performance during normal operation?

alexpott’s picture

Status: Needs review » Needs work

Thanks for the review @borisson_ I think #46.1 is not too important it is unlikely we'll introduce yet another installer service provider - famous last words and all but the 2 should be enough.

Re #46.2 - that's a good point. Atm InstallerKernel::installationAttempted() is very cheap (but it is a method call... but we can do better. We can have a special installer version of the lazy router builder and avoid this completely for runtime.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.28 KB
20.96 KB

Fixing #46.2

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

The (relevant) remark I had earlier is fixed.

The performance of the installed application should not change compared to the current situation and installation should be a lot faster, this issue seems like a clear winner :) RTBC.

alexpott’s picture

Also I realised that the lazy route provider is only used in module installation so the performance improvement of #48 over #45 is irrelevant for regular runtime. It is a bit neater in terms of conditions and things to think about so #48 seems preferable.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed e0d5786 and pushed to 9.1.x. Thanks!

  • catch committed e0d5786 on 9.1.x
    Issue #3086307 by alexpott, shaal, tstoeckler, mglaman, Berdir, andypost...

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: +9.1.0 highlights
ressa’s picture

Thanks a lot for making this improvement, performance is so much better, cutting the default install time in half, and reducing Umami profile install time with roughly 25%. Testing with Lando 3.0.16 in Ubuntu 20.04:

Default installation

time drush site-install -y

Drupal 9.0.7 11,132s
Drupal 9.1.0-alpha1 6,643s

Umami profile installation

time drush site-install demo_umami -y

Drupal 9.0.7 29,487s
Drupal 9.1.0-alpha1 21,648s
mxr576’s picture

I created a 8.9.x backport of #52 when I tested this fix as a possible solution for an issue that I reported in Argument 2 passed to Drupal\jsonapi\Routing\Routes::Drupal\jsonapi\Routing\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceType, NULL given. This patch did solve the issue completely but reduced warnings by ~80%. See comment #163 and #165.

smustgrave’s picture