Improve the performance of \Drupal\Core\Update\UpdateRegistry::getRemovedPostUpdates()

I'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.

Left some comments

I'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.

I made some minor changes to the CR formatting, as I believe we use human-readable sentences. Otherwise, the code changes look good!

Hope 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 :)

Totally, I forgot to unassign it for the weekend anyways.

I think we we can improve things further by moving the caching down a level - see comment on the MR.

It'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()

Here's three different profiles testing against Umami. These all compare against a base of b1fbff58cca271a88373115ceb3a818bff1762d2 which is on the 11.x branch.

First: comparing against 5605d11412 from this merge request. This is the first pass with a cache just around getRemovedPostUpdates():

Overall pretty decent, a 2% reduction in time and ~182K less function calls.

Second: comparing against c30cc7f014 which moves the cache to scanExtensionsAndLoadUpdateFiles(). 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 5605d11412 with the additional call to clear caches as needed to pass tests when caching scanExtensionsAndLoadUpdateFiles().

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.

I think I might have found an issue in the new caching logic - see comment on the MR.

Appears to be 1 open thread, have not reviewed or tested.

This is at least major. Fixing this will speed up site installs loads.

There's only one failure in https://git.drupalcode.org/issue/drupal-3414349/-/jobs/664042 - so things might be close with the current approach?

I'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 🐛 Configuration being imported by the ConfigImporter sometimes has stale original data Fixed (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:

There are 2 MRs, 1 all green and 1 with a test failure, which one is to be reviewed?

deviantintegral → closed merge request !6145

Nice! 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...

Do wonder if a performance test should be added for this.

To 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.

I deleted the CR because it was no longer relevant to the new approach. This would be great to land.

Seems 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.

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

Opened 🌱 Possible meta around performance testing of function calls Active

Automatically closed - issue fixed for 2 weeks with no activity.