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

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.

🤩 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 #3419824: Mark some more tests with @group #slow 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 #3356739: Memory usage increases linearly when (un)installing modules via config import

@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 🙌

🤯

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

🥴

Just RTBC'd the blocker #3419987: Fix Container::reset() and provide DrupalKernel::resetContainer().

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.

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

This looks really interesting! Would this also fix the issue discussed in #3164982: Optional configuration from dependence modules is installed before installing module when installing profile? 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.

We could then open a follow-up to flip the null behaviour in Drupal 12.

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 several test modules, one of which needs a container rebuild, one which has a dependency on a module that needs a container rebuild etc.

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 compiled with --enable-debug. Maybe someone armed with such a build could run a drush si?

And/or the latest php minor could be interesting because https://github.com/php/php-src/issues/16601 is included in 8.3.14.

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 on that piece, 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 as discussed in slack.

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

I've opened #3492233: [meta] Reduce memory/cpu/io cost of attribute discovery 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.

Also opened #3492235: Default container_rebuild_required to FALSE for discussion - don't want that to hold up this getting committed, but I think we should discuss it.

I think all comments were addressed. RTBC +1

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.

edit: back to green.

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 which can be added later.

I added suggestions to the todos to reference the issues.

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: #3491780: Theme hooks not found warning on Umami install which is fixed by #2422681: Remove the automatic cron run from the installer

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

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 #3492235: Default container_rebuild_required to FALSE 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 #3492235: Default container_rebuild_required to FALSE and I think it could be under 5 minutes once #3490512: Improve run times of package manager tests 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 dev
$ 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

Just a note for the highlights post. I realise I am making work for someone that I will not personally do and not sure how tricky it would be to pull off.

It would be cool to have a split-screen video of the cli or UI installer of Drupal 11.1 vs Drupal 11.2 showing the batch module install. Drupal 11.2 should show a fraction of the batches and hopefully finish considerably faster. This could be done with Drupal CMS maybe, because it should be more dramatic with more modules to install.

@ressa's CLI video is a great start here and would cover the CLI install pretty well. It'd be easier to see the progress with the UI batch though.

You're right @catch, showing Drupal 11.2 modules install in a split second, compared to one-by-one in Drupal 11.1 is better, and I re-did the D11.2 vs. D11.1 split-screen install.

A Drupal CMS side-by-side GUI install of D11.1 and D11.2 is a great idea, but I get 40 seconds on both ... Do you see any difference?

I am including a site install as well, for completeness. Happy New year!

Drupal 11.2 vs. Drupal 11.1 module install

Drupal 11.2 vs. Drupal 11.1 site install

I figured out how to trigger commands simultaneously in two CLI windows with Terminator, I'll add the updated versions in the Issue Summary.

About getting 40 seconds for Drupal CMS on both 11.1 and 11.2, it looks like the related issue may improve that?

This causes a regression (flagging unmet dependencies) with our contrib module installing as part of a profile or in a phpunit test see https://www.drupal.org/project/drupal/issues/3529670 for more info

Found another possible regression, see #3416735-10: Stream wrappers not registered when installing module's default config
Which I suspect is coming from ::updateKernel call at the very end of ModuleInstaller::doInstall

Thanks, massive improvement for the distribution https://github.com/YCloudYUSA/yusaopeny/pull/327