Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller

Okay proved this is possible. The tests are green. Will update the issue summary with more detail.

This makes an install via drush take 10 seconds instead of 90 seconds. I haven't compared the UI installer yet and that's a bit harder to time, but if it's even a third faster that makes this a significant UX improvement for new installs. Also looks like it shaves anywhere from 30-60s off test runs (and maybe 10 minutes of actual runner time if you add up the shorter test runs for all functional and functional js tests together), and probably opens up new possibilities from there too like shifting distribution of the jobs slightly etc.

Added some details about the API changes to the issue summary.

🤩 Wow, epic undertaking! 👏

We rebuild the container during every module install.

This issue is going to experiment with another approach.

I think that means you expect this to provide a noticeable speed-up of test runs?

I think that means you expect this to provide a noticeable speed-up of test runs?

The fastest pipeline on this MR is 7m6s https://git.drupalcode.org/project/drupal/-/pipelines/89610, the fastest I've seen for core otherwise is about 7m30s. It also looks like all of the functional and functional javascript tests are finishing 30-60s quicker than usual. So it should be both a headline speed improvement for pipelines finishing, and also a reduction in overall runner time. I've opened 📌 Mark some more tests with @group #slow Needs review for some overhanging tests that the pipeline here identified, which might squeeze out a few more seconds or so from the longest running jobs once both are in.

I'm very hopeful that this will fix 🐛 Memory usage increases linearly when (un)installing modules via config import Active

@acbramley I don't think it will. I think 🐛 Memory usage increases linearly when (un)installing modules via config import Active is likely due to do something like [#3416294[ - I know you are not using the plugin module but the amount of memory you are using points to something going wrong.

@alexpott that's a shame :(

but the amount of memory you are using points to something going wrong

Do you have any ideas of threads I could pull on? Pretty keen to get to the bottom of it!

Great work @alexpott 🙌

I've created 📌 Fix Container::reset() and provide DrupalKernel::resetContainer() Needs review to split of the DrupalKernel / Container changes to allow resetting.

Here's patches for 10.2.x and 10.1.x - to make testing on existing sites easier. These patches include 🐛 ConfigInstaller::isSyncing() is not reset before module preinstall Fixed due to conflicts.

alexpott → changed the visibility of the branch 3416522-10.1.x to hidden.

I've testing this on a site that's built from commerce kickstart. I've found two issues:

1. View data causes problems with modules/payment/config/install/views.view.commerce_order_payments.yml and modules/log/config/install/views.view.commerce_activity.yml
2. The config_rewriter module has a bug in it when it does not detect a config sync correctly.

I will open a issue against the config_rewriter module as it should not be rewriting configuration ever during a config sync. Funnily enough 🐛 ConfigInstaller::isSyncing() is not reset before module preinstall Fixed helps with this.

For the views data issue - need to do 2 things - work why we're using views data when creating config during an install and if this is 100% necessary I think the way to go is to split simple config from config entity install while installing configuration from config/install.

Oh and re #16 once I'd fixed that... recreating the site locally went from 67 secs to 34 secs and I have a highly optimised setup...

🤯

recreating the site locally went from 67 secs to 34 secs and I have a highly optimised setup...

🥴

Just RTBC'd the blocker 📌 Fix Container::reset() and provide DrupalKernel::resetContainer() Needs review .

Fixed the issue with views and commerce kickstart install. Need to add a test for this case. I've tried to cause what I'm seeing with kickstart with core test modules and so far no dice.

Adding patches for 10.2 and 10.1 testing.

Couple of tiny fixes to the 10.1 patch for testing ... hopefully should allow us to run the old Drupal CI tests...

Hmm #20 / the fixes to install simple config and config entities separately was not quite right here are some new patches.

@daniel.bosen has tried the patch out on Thunder and has run into a problem with the ConfigInstaller::installDefaultConfig() change because Thunder uses the config_selector module. That module decorates the config installer and changes the behaviour of installDefaultConfig(). See https://github.com/thunder/thunder-distribution/actions/runs/7832035165/...

So we need to have a think about that.

Install went fine on our install profile (138 installed modules and themes), didn't run tests yet.

before:
 [notice] Performed install task: install_profile_themes [35.95 sec, 91.52 MB]
....
 [notice] Performed install task: install_finished [62.81 sec, 178.25 MB]

after:
 [notice] Performed install task: install_profile_themes [24.95 sec, 99.71 MB]
...
 [notice] Performed install task: install_finished [50.77 sec, 166.01 MB]

Total install time wennt from 62s to 50s, but as you can see, around 50% of that time is installing default content and stuff like that. module install itself went from 35s to 24s.

Seems to have some open threads if those could be closed or check marked

For it's worth I applied locally and installed a starterkit profile I use for new projects (maybe 40ish modules) and everything imported just fine.

@daniel.bosen has tried the patch out on Thunder and has run into a problem with the ConfigInstaller::installDefaultConfig() change because Thunder uses the config_selector module. That module decorates the config installer and changes the behaviour of installDefaultConfig().

Could we introduce a .info.yml key that forces a container rebuild on install? I think that would be OK in a minor release with a change record. Or worst case we could assume it's set in 10.x (so the behaviour doesn't actually change), then flip it in 11.x so that the performance improvement kicks in.

📌 Fix Container::reset() and provide DrupalKernel::resetContainer() Needs review is in … oh I see @alexpott is already on this! 😁

This looks really interesting! Would this also fix the issue discussed in 🐛 Optional configuration from dependence modules is installed before installing module when installing profile Needs work ? That's something we're running into at Open Social. It seems like possibly the following mentions that it does?

Rework \Drupal\Core\Config\ConfigInstaller::checkConfigurationToInstall() and \Drupal\Core\Config\ConfigInstaller::findDefaultConfigWithUnmetDependencies() to work with a list of modules. This has a side effect of fixing an existing bug in the config installer that hook_modules_installed is not called for already installed modules when a module is found that triggers an pre-exisiting configuration exception.

However, the initial "split up in multiple parts" would suggest that we go the opposite direction and make the site installer more different from how individual module installs work rather than bringing the two closer together.

Merged in 11.x (after a very quickly aborted attempt to rebase), and back to green - a couple of small merge conflicts, recipe required module install needed to move to multiple to fix that test.

Going back to #3416522-22: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller → I think we should consider making this opt-in per module at least to start with.

- add a .info.yml that is something like 'multiple_install: true' or 'container_rebuild_required: true', and default it so that the container is rebuilt immediately after module install if it's not set.

- add multiple_install: true/container_rebuild_required: false to all core modules
- then we can open issues against the top 100-200 modules to add it there too.

That should get us the bulk of the performance improvements here with very little risk of regressions.

Sounds like a plan to me.

Added support for container_rebuild_required: true (which also starts a new group if it's not set).

Then in a couple of separate commits, added container_rebuild_require: false to every core (non-test) module.

Less than three minute test run on https://git.drupalcode.org/project/drupal/-/jobs/2424744 shows the optimisation still working.

This should fix the situation in #3416522-22: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller → because every module that doesn't specify container_rebuild_required: false will be installed at the end of each batch of modules (and if there are lots of them sequentially, in their own individual single module batch).

I also addressed some of the outstanding minor feedback on the MR and closed a couple of threads where the status quo seemed OK.

Needs review again for the new approach and the contents of the MR in general.

This is still tagged for needs tests. We could add a testing module that increments a counter every container rebuild, then add assertions after install on how many times the container was rebuilt. Then we can add different combinations of test modules, with and without container_rebuild_required: true to different test install profiles and check the count. The problem with this is I can see it breaking due to changes that don't involve breaking the logic (like increasing or decreasing the default batch size, or adding/removing a module dependency somewhere), so it's not perfect. Better ideas would be better.

Could we open up follow ups for the todos? Just so they don't get lost in the code shuffle.

I see a new container_rebuild_required in the MR#6290, that needs a change record, maybe a documentation update about info.yml keys as well.

Added a change record https://www.drupal.org/node/3473563 →

That test approach sounds reasonable to me. Its OK that it may occasionally need updating if default batch size changes, and so on.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased. This really helps with both install time and memory usage even on standard profile:

11.x:

[notice] Performed install task: install_finished [13.62 sec, 252.17 MB]

This branch:

[notice] Performed install task: install_finished [9.38 sec, 70.35 MB]

I implemented the ConfigImporter changes that @alexpott noted we need.

Originally I thought we could just detect a module that needs a container build in ModuleInstaller::install() and do an extra container build, but it's not as simple as that. Maybe we can do that in module installer directly by moving half the ::install() logic to a helper, then implementing the same chunking logic there though?

So ConfigImporter now chunks modules in a similar way to the installer. I had to update one test to check a cumulative state key instead of a per-modules-installed state key which proves the change is doing what it should.

Moving back to needs review, I think we seriously need to consider going ahead here since this is the single biggest change that will mitigate the performance/memory impact of the OOP hooks bc layer (and also plugin attribute parsing and everything else).

Update on #39, added container_rebuild_required support to ModuleInstaller::install() directly, this means any code calling it will automatically respect the info key.

Pipeline is green again.

Added explicit test coverage to ModuleInstallerTest with three test modules, one of which needs a container rebuild.

This modernization is amazing.

However, I think it only makes consequences of memory leaks significantly less severe. But still, it looks like something leaks memory and maybe that leak should be found and plugged? If it's php, it has a memory leak detector when compile with --enable-debug. Maybe someone armed with such a build could run a drush si?

I think the plan is to find the leak, but this is still definitely worth doing and helps in the meantime.

One minor suggestion.

I reviewed it thoroughly, I don't know the config installer well enough to sign off, but it looks right to me.

The ModuleInstaller looks right. One minor suggestion.

I think the test coverage for the new flag covers all situations.

https://github.com/php/php-src/issues/16601 is interesting.

I've opened 🌱 Reduce memory/cpu/io cost of attribute discovery Active as a tracking issue, will start adding the various child issues attempting to tackle this. We should have a dedicated issue to try to find the memory leak on top of that though too.

I think all comments were addressed. RTBC +1

Here's a list of contrib projects that will need checking. They have code that implements ConfigInstallerInterface and the changes we are making to it are not truly inline with our BC policy. I can't see a way to make the changes necessary to that class without these changes unfortunately.

• https://www.drupal.org/project/rocketship_core → (not D11 compat yet)
• https://www.drupal.org/project/config_provider → (not D11 compat yet)
• https://www.drupal.org/project/config_selector → (I maintain it - these changes do break it but can be fixed)
• https://www.drupal.org/project/install_config_override → (not D11 compat yet)

I'm going to add a decent CR to this issue to explain the changes and how these modules can be fixed to work with D10 and D11 at the same time.

Do you want me to create an issue in those modules to keep an eye on this?

I think that revert broke some tests.

I think those are all random failures, re-running the jobs.

https://git.drupalcode.org/project/drupal/-/merge_requests/6290#note_423326 is about an actual (minor) bug. NW for that. I did open a bunch of other threads, mostly with suggestions, none of them blocking.

Resolved or answered all the latest feedback, back to needs reviews.

IS is not up to date to the current approach it seems. Have not really reviewed code.

Updated the proposed resolution in the issue summary. Also removing the needs tests tag since we have a kernel test for the new behaviour now.

I've reviewed this again thoroughly. It's been reviewed by several people and all comments have been addressed.

CR looks good.

I'm going to create issues for the remaining todos and add suggestions, I don't think that should be a blocker though.

All todos created, they need more detail, but there are suggestions.

Forgot to mention I also installed drupal standard and umami both with and without drush.

I then enabled every module except deprecated ones (I don't think I ever did that before).

I saw no errors there either beyond the known umami issue for missing hook theme: 🐛 Theme hooks not found warning on Umami install Active which is fixed by 📌 Remove the automatic cron run from the installer Needs work

If you think this needs more manual testing feel free to add that tag and put it back in needs review.

Removing part of the API changes in the issue summary that was done in 📌 Fix Container::reset() and provide DrupalKernel::resetContainer() Needs review

Added the CR for the config installer changes - https://www.drupal.org/node/3492559 → - and updated the issue summary to reflect the changes in the code.

Given this is quite a big change and we are already in RC for 11.1 this is only eligible for commit to 11.x and will be in 11.2.0. This gives us a few months for contrib to add the new flag or (if possible) for us to auto detect situations when the container rebuild is actually required.

Committed f8090e6 and pushed to 11.x. Thanks!

Also published the CRs.

EPIC work here! 🤩🤯👏

Q: what's the final performance impact on A) real-world sites, B) (core) tests?

See @alexpott's #17 for the original impact, I'm not sure if the impact is the same, or more, or less, because 📌 Consider defaulting requires_container_rebuild to FALSE Active and other follow-ups may be needed to achieve that original impact.

Perhaps that's worth capturing in a release notes snippet, because this is AFAICT one of the most monumental low-level changes to Drupal's extensibility capabilities in many years? 🤞 (The other one being the recent OOP hooks support.)

@Wim we just got a 5m30s test run on 📌 Consider defaulting requires_container_rebuild to FALSE Active and I think it could be under 5 minutes once 📌 Some of the package manager kernel tests are much slower than other kernel tests - mark them #slow Active is done. We've been down to 5m30s or close before, but not for a few weeks now. But yes it'll need that other issue to get the full benefit here.

Also see some drush si comparisons on #3492453-5: Memory leak in DrupalKernel when installing modules → . However because container rebuilds are more expensive with OOP hooks, it's only a small improvement compared to 10.4.x or 11.0.x at the moment. But we've dropped runtime hook discovery altogether so the real-world implications should be overall positive.

The release notes are for things that affect site owners, but this is worth calling out as a highlight in the announcement for 11.2.0.

I agree, this definitely deserves to get highlighted in the Drupal 11.2 release, big thanks to everyone here who made it happen!

I tried with ~60 modules, and the module install time went down from around 24 seconds to just 10 seconds.

Drupal 11.1 vs. Drupal 11.2

For anyone who wants to try it, this gets a Drupal 11.2 installation ready for bench marking with ~60 modules:

# set it up to ignore all constraints
$ ddev composer require drush/drush drupal/backward_compatibility mglaman/composer-drupal-lenient
$ ddev composer config --json extra.drupal-lenient.allow-all true
$ ddev composer config --json minimum-stability false
$ ddev drush site:install -y
$ ddev drush install backward_compatibility

# get modules
$ ddev composer require drupal/address drupal/admin_toolbar drupal/antibot drupal/backup_migrate drupal/better_exposed_filters drupal/blazy drupal/block_class drupal/captcha drupal/colorbox drupal/config_filter drupal/config_ignore drupal/config_split drupal/crop drupal/csv_serialization drupal/devel drupal/diff drupal/ds drupal/easy_breadcrumb drupal/editor_advanced_link drupal/eu_cookie_compliance drupal/externalauth drupal/extlink drupal/feeds drupal/field_group drupal/field_permissions drupal/focal_point drupal/fontawesome drupal/geofield drupal/google_analytics drupal/google_tag drupal/honeypot drupal/image_widget_crop drupal/menu_block drupal/menu_link_attributes drupal/metatag drupal/migrate_tools drupal/mimemail drupal/module_filter drupal/pathauto drupal/rdf drupal/recaptcha drupal/redirect drupal/rules drupal/scheduler drupal/search_api drupal/seckit drupal/simple_sitemap drupal/slick drupal/smtp drupal/svg_image drupal/token drupal/twig_tweak drupal/video_embed_field drupal/views_bulk_operations drupal/views_data_export drupal/views_infinite_scroll drupal/views_slideshow drupal/webform drupal/xmlsitemap

# install modules, and time it
$ time ddev drush install -y address admin_toolbar antibot backup_migrate better_exposed_filters blazy block_class captcha colorbox config_filter config_ignore config_split crop csv_serialization devel diff ds easy_breadcrumb editor_advanced_link eu_cookie_compliance externalauth extlink feeds field_group field_permissions focal_point fontawesome geofield google_analytics google_tag honeypot image_widget_crop menu_block menu_link_attributes metatag migrate_tools mimemail module_filter pathauto rdf recaptcha redirect rules scheduler search_api seckit simple_sitemap slick smtp svg_image token twig_tweak video_embed_field views_bulk_operations views_data_export views_infinite_scroll views_slideshow webform xmlsitemap