Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
I'm investigating a site install profile that takes over 4 minutes to install. As a part of that, I noticed that nearly 20% of the installation time was in calls to \Drupal\Core\Update\UpdateRegistry::getRemovedPostUpdates() and its child functions.
This MR adds in a static memory cache so that each module is only ever scanned once per bootstrap.
Here's a diff of a custom profile from before this change and after.
And for something more reproducible, here's demo_umami with a 2% improvement:
.
Steps to reproduce
I think this is best tested on sites with many, many modules and configuration exports. In this case, the site has ~350 modules and ~2800 config objects. Looking at the test runs, it doesn't appear this improves the standard or testing profiles at all, but perhaps it shows up with umami.
Proposed resolution
Add a static cache.
Remaining tasks
Confirm this isn't a bc break for backporting to Drupal 10.
User interface changes
None
API changes
None, constructor changes only.
Data model changes
None
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3414349
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
deviantintegral commentedComment #4
deviantintegral commentedComment #5
deviantintegral commentedComment #6
deviantintegral commentedComment #7
deviantintegral commentedI'm kind of amazed tests are passing. With that in mind, I've rebased against 11.x, though this patch applies cleanly to several prior Drupal versions as well.
I haven't added new tests, because this feels like exactly the sort of thing that should be transparent to calling code.
Comment #8
deviantintegral commentedComment #9
deviantintegral commentedComment #10
smustgrave commentedLeft some comments
Comment #12
taraskorpachI've updated the MR based on the comments. I've also added change records. Since this is my first time doing this, it would be better to have it reviewed.
Comment #13
taraskorpachComment #14
deviantintegral commentedI made some minor changes to the CR formatting, as I believe we use human-readable sentences. Otherwise, the code changes look good!
Comment #15
taraskorpachHope it's okay within the Drupal community to interrupt in the middle of resolving an issue and make some MR changes. I just wanted to save you some time :)
Comment #16
deviantintegral commentedTotally, I forgot to unassign it for the weekend anyways.
Comment #17
catchI think we we can improve things further by moving the caching down a level - see comment on the MR.
Comment #18
deviantintegral commentedComment #19
catchIt'd be worth comparing the detail for ::scanExtensionsAndLoadUpdateFiles() with the two approaches - if we're also checking update hooks during install (which we should be), then we should see savings from that which we won't see when comparing getRemovedPostUpdates()
Comment #20
deviantintegral commentedHere's three different profiles testing against Umami. These all compare against a base of
b1fbff58cca271a88373115ceb3a818bff1762d2which is on the 11.x branch.First: comparing against
5605d11412from this merge request. This is the first pass with a cache just aroundgetRemovedPostUpdates():Overall pretty decent, a 2% reduction in time and ~182K less function calls.
Second: comparing against
c30cc7f014which moves the cache toscanExtensionsAndLoadUpdateFiles(). While there's still around 182K less function calls, it actually took ~455ms more CPU time.I was curious and wondered if the additional calls to delete() needed when caching that whole function were related. So here's a third comparison against
5605d11412with the additional call to clear caches as needed to pass tests when cachingscanExtensionsAndLoadUpdateFiles().As we can see, the additional delete() calls clearly have an effect.
With the above in mind, I think we should go with the first approach where we just cache
getRemovedPostUpdates. While the second approach may be conceptually better, in all cases so far it seems to be slower than the first approach, both in a small install profile like Umami and in a much larger install profile.Comment #21
catchI think I might have found an issue in the new caching logic - see comment on the MR.
Comment #22
smustgrave commentedAppears to be 1 open thread, have not reviewed or tested.
Comment #23
taraskorpachComment #24
alexpottThis is at least major. Fixing this will speed up site installs loads.
Comment #25
catchThere's only one failure in https://git.drupalcode.org/issue/drupal-3414349/-/jobs/664042 - so things might be close with the current approach?
Comment #27
alexpottI've just opened up a new MR that refactors UpdateRegistry a bit. I no longer scans for all update functions for all extensions when doing getUpdateFunctions()... it only looks are update functions for the provided $extension_name.
It also static caches if calls loadUpdateFile() for a particular extension / root /site / update type combination and then does not redo work in scanExtensionsAndLoadUpdateFiles() because once a file is included in PHP it cannot change and it cannot be unloaded.
I've got a legacy site where I've applied this MR and #3406929: Configuration being imported by the ConfigImporter sometimes has stale original data (which also has a big impact on what happens in \Drupal\Core\Update\UpdateRegistry::onConfigSave() and this results in the follow xphrof changes...
This MR is responsible for:
Comment #28
smustgrave commentedThere are 2 MRs, 1 all green and 1 with a test failure, which one is to be reviewed?
Comment #30
deviantintegral commentedNice! I like that we can go back to a simple static variable, I hadn't thought about the fact that PHP won't reload files once loaded. This also saves Drush from having to coordinate a release around this change (not that it's core's responsibility, but it's nice to save downstream users work even for constructor changes).
https://github.com/drush-ops/drush/blob/1f9e78ad1aace1efa26bbaaa2e30c628...
Comment #31
deviantintegral commentedComment #32
smustgrave commentedDo wonder if a performance test should be added for this.
Comment #33
catchTo add performance testing for this we'd need to be able to count function calls via zend_observer which requires a PHP extension. This is not out of the question and something I'd love us to be able to do eventually, but it'd be a completely new thing compared to the current performance testing and quite a lot of work to set up.
Comment #34
alexpottI deleted the CR because it was no longer relevant to the new approach. This would be great to land.
Comment #35
smustgrave commentedSeems all feedback has been addressed then
@catch if you think it's worth it I can open a follow up for the testing.
Would say this may fall under a task maybe but will leave as is.
Comment #38
catch@smustgrave I don't think we've got an issue open for counting function calls, but it would be good to have one yeah, would probably need to be a plan since it'd be a multi-step thing.
MR looks good and don't have any comments, so committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!
Comment #39
smustgrave commentedOpened #3419313: Possible meta around performance testing of function calls