Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
In Drupal 8.9.x we are seeing a tenfold increase in memory consumption when installing a project using existing config. Using git bisect it appears this problem has been introduced in #474684: Allow themes to declare dependencies on modules.
Please note that I have been testing a project that uses a composer based workflow and the listed commits are from the drupal/core
subtree split.
Last known good commit
- Commit: 353773bfbc494c6d1cc4fd33c4463e3307434a24
- Issue: SA-CORE-2020-001
- Memory usage: 150113928 (142.2MB)
- Peak memory usage: 175713912 (167.6MB)
[drush] Executing command: /home/pieter/v/joinup-dev/vendor/bin/drush --verbose --yes --root="/home/pieter/v/joinup-dev/web" --db-url="mysql://root:@127.0.0.1:3306/joinup" --site-name="Joinup" --account-name="admin" --account-pass="admin" --account-mail="admin@example.com" --existing-config site-install joinup install_configure_form.enable_update_status_module=NULL install_configure_form.enable_update_status_emails='NULL' install_settings_form.sparql.host=localhost install_settings_form.sparql.port=8890 install_settings_form.sparql.namespace=Drupal\\Driver\\Database\\joinup_sparql
[info] Executing: command -v mysql
[info] Executing: mysql --defaults-file=/tmp/drush_Q3FA1B --database=joinup --host=127.0.0.1 --port=3306 --silent -A < /tmp/drush_v52JDB
// You are about to DROP all tables in your 'joinup' database. Do you want to continue?: yes.
[info] Sites directory sites/default already exists - proceeding.
[info] Executing: mysql --defaults-file=/tmp/drush_A5ztzC --database=joinup --host=127.0.0.1 --port=3306 --silent -A < /tmp/drush_ZS6uGF
[info] Executing: mysql --defaults-file=/tmp/drush_D6YPWC --database=joinup --host=127.0.0.1 --port=3306 --silent -A < /tmp/drush_MDWvmC
[info] Executing: mysql --defaults-file=/tmp/drush_KyY0aF --database=joinup --host=127.0.0.1 --port=3306 --silent -A < /tmp/drush_izwmfC
[info] Installing from existing config at /home/pieter/v/joinup-dev/config/sync
[notice] Starting Drupal installation. This takes a while.
[success] Installation complete.
[success] Memory usage: 150113928, peak memory usage: 175713912
First bad commit
- Commit: 6e9d3dea72537b3a557749c5e614abc92f55b8bf
- Issue: #474684: Allow themes to declare dependencies on modules
- Memory usage: 1378101456 (1314.3MB)
- Peak memory usage: 1403767040 (1338.7MB)
[drush] Executing command: /home/pieter/v/joinup-dev/vendor/bin/drush --verbose --yes --root="/home/pieter/v/joinup-dev/web" --db-url="mysql://root:@127.0.0.1:3306/joinup" --site-name="Joinup" --account-name="admin" --account-pass="admin" --account-mail="admin@example.com" --existing-config site-install joinup install_configure_form.enable_update_status_module=NULL install_configure_form.enable_update_status_emails='NULL' install_settings_form.sparql.host=localhost install_settings_form.sparql.port=8890 install_settings_form.sparql.namespace=Drupal\\Driver\\Database\\joinup_sparql
[info] Executing: command -v mysql
[info] Executing: mysql --defaults-file=/tmp/drush_FQExgb --database=joinup --host=127.0.0.1 --port=3306 --silent -A < /tmp/drush_a2dbk9
// You are about to DROP all tables in your 'joinup' database. Do you want to continue?: yes.
[info] Sites directory sites/default already exists - proceeding.
[info] Executing: mysql --defaults-file=/tmp/drush_d4t6b7 --database=joinup --host=127.0.0.1 --port=3306 --silent -A < /tmp/drush_xxtsx9
[info] Executing: mysql --defaults-file=/tmp/drush_raMCa9 --database=joinup --host=127.0.0.1 --port=3306 --silent -A < /tmp/drush_NoSTL6
[info] Executing: mysql --defaults-file=/tmp/drush_cL66g7 --database=joinup --host=127.0.0.1 --port=3306 --silent -A < /tmp/drush_ZZdGsa
[info] Installing from existing config at /home/pieter/v/joinup-dev/config/sync
[notice] Starting Drupal installation. This takes a while.
[success] Installation complete.
[success] Memory usage: 1378101456, peak memory usage: 1403767040
The project that is affected by this problem is open source, ref: https://github.com/ec-europa/joinup-dev. Ticket tracking this problem is https://github.com/ec-europa/joinup-dev/pull/2088.
Comment | File | Size | Author |
---|---|---|---|
#8 | 3126003-8.9.x-8.patch | 3.05 KB | alexpott |
#8 | 3126003-8.patch | 3.05 KB | alexpott |
#4 | 3126003-4.patch | 627 bytes | tedbow |
#4 | 3126003-3.patch | 627 bytes | tedbow |
#3 | 3126003-3-do-not-test.patch | 3.41 KB | tedbow |
Comments
Comment #2
pfrenssenComment #3
tedbowI pretty sure it was the change to the module installer from #474684: Allow themes to declare dependencies on modules. If I revert those changes I can do
drush si -y
locally with
memory_limit = 96M
using PHP 7.2.23 and sqlitewith 8.9.x I have to set
memory_limit = 300M
(I didn't check every value in between obviously but roughly)
I uploaded a patch if anyone want to test this out. Not testing this patch because it will break other tests.
Comment #4
tedbowHere is try that makes be able to install again at
memory_limit = 96M
In
\Drupal\Core\Extension\ModuleInstaller::updateKernel()
I am not sure we need to get the extension list from the container because we never actually uninstall themes we just use the list to check if they are enabled.
Comment #5
longwaveI wonder if we can add a test that checks maximum memory usage during install and fails if we go over a documented limit.
Comment #6
catchWondering whether this is some kind of memory leak, or whether the theme list is just massive. Themes do tend to have much larger .info.yml files than modules.
Comment #7
alexpott@catch Hopefully not 1200mb of theme data though :)
Given we only need the result of
themeExtensionList->getList()
in uninstall how about we do \Drupal::service('extension.list.theme')->getList() at the beginning of \Drupal\Core\Extension\ModuleInstaller::uninstall() like we do for$module_data = \Drupal::service('extension.list.module')->getList();
. Injecting dependent services here is fraught and ideally we'd change this thing to not have any services injected. #2380293: Properly inject services into ModuleInstaller has taken forever for a reason :)I should have remembered that issue and we should have avoided injecting things here... oops.
Comment #8
alexpottImplementing #7
Comment #9
pfrenssenI can confirm that removing this line resolves the problem in our project, with the experimental patch from #4 we are back at reasonable memory usage:
Nice sleuthing!
Comment #10
pfrenssenResult from the patch from #8:
Comment #11
lauriiiI did some profiling on this and can confirm increase of memory consumption after #474684: Allow themes to declare dependencies on modules. Based on some research it seems like the problem is with assigning
$this->themeExtensionList
with the new instance of the theme extension list, not in the process of gettingextension.list.theme
service.While we're waiting for @pfrenssen to confirm that this patch addresses the problem in their environment, we might want to consider adding a comment explaining why we are loading the theme extension list this way.
Comment #12
alexpottInstalling standard from config on 8.9.x With the patch applied
Without the patch
Comment #13
lauriiiI got very similar results to #12 when installing Drupal with standard install profile:
It seems like I crossposted with @pfrenssen on #11 because #10 already confirms that their concerns have been addressed.
Comment #14
longwave> I should have remembered that issue and we should have avoided injecting things here... oops.
While we are here we could add a comment as to why this service isn't injected for now, to avoid a future refactor making the same mistake?
Comment #15
alexpott@longwave check out all the other \Drupal::service() calls in this class. The same would happen if we injected
\Drupal::service('extension.list.module')
Comment #16
alexpottTo address #14 I've commented on #2380293: Properly inject services into ModuleInstaller and linked the issues.
Comment #17
catchFixing credit.
Comment #21
catchCommitted/pushed to 9.1.x, 9.0.x, 8.9.x, thanks!
Comment #22
alexpottPerhaps we need another push on #2495411: Make simpletest fail a test when it detects pages that need more than 64MB