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.