Reduce the number of times UpdateRegistry scans for extensions

I'd love ideas for how to optimize this.

When testing an issue I output the number of scans for a single kernel test and it was dozens.

I think this might qualify for major.

Rerolled the patch so we can review the tests.

I didn't review it just applied it and resolved conflicts.

Postponing on #3618545: Always use infoParser in ExtensionDiscovery

I tried profiling this in combination with #3618545: Always use infoParser in ExtensionDiscovery and it's not reducing the extension scanning yet. Still 9 calls to ExtensionDiscovery::scanDirectory() when running a curtailed version of node module's GenericTest.

It might be different with a real install via dr or drush (or the UI) which I did not try to profile yet.

Tried an alternative approach in MR!16818 which avoids threading a boolean flag through several methods, and includes test coverage to prove that we discover once and then cache.

That does look a lot better, is there a reason we shouldn't use a memory cache though?

I hid the other MR.

Got a related issue here too.

Switched to a memory cache, which means we can remove the explicit invalidation and use cache tags. Also rearranged the test coverage a bit to show that installing a module invalidates the cache correctly.

Profiled a curtailed version of node module's GenericTest again and this time looked for the right method in the profiling data, #13 was just looking for the wrong thing.

As far as I can tell, this is saving 20 calls to ExtensionDiscovery::scan(), which works out about 15ms or half the time spent in UpdateRegistry::getUpdateFunctions(). Uploading xhgui screenshots.

On a real test run we're doing this hundreds/thousands of times so it will add up.

Got a few suggestions I added to the MR, I switched to MemoryCacheInterface since #3590964: Update/Install-time service overrides for cache should support MemoryCacheInterface landed.
I also removed the constructor comment.

The bigger change though was I moved the caching logic to discoverExtensions so that if we ever call that elsewhere the calls are cached as well.

I also had one question on the test I'm not sure about.

I think you might have accidentally force pushed over the changes we made this morning, UpdateRegistry is back to CacheBackendInterface and the cache set is back in the call site instead of in discoverExtensions.

Sorry for making a mess here, trying to do too many things at once, hopefully this is better.

No worries, we've all done it, I use --force-with-lease now to prevent overwriting commits upstream.

There is a failing test though:

Service Provider (Drupal\KernelTests\Core\ServiceProvider\ServiceProvider)
     ✔ Service provider registration
     ✘ Service provider registration dynamic
       ┐
       ├ TypeError: Drupal\Core\Update\UpdateRegistry::__construct(): Argument #6 ($memoryCache) must be of type Drupal\Core\Cache\MemoryCache\MemoryCacheInterface, Drupal\Core\Cache\DatabaseBackend given

This looks great now!

I don't think we need a CR for this.

Wondering if we need constructor BC because I think that drush also uses the update registry for deploy hooks.

Probably, which means we would need a CR:
https://github.com/drush-ops/drush/blob/92fa219135a89a8aab5a30309af0655b...

Constructor parameters usually become required in the next version so 12, not 13.

NW for #33 and the change record.

I took a pass at the CR, it could use another set of eyes.

I added a suggestion for the CR update and version.

If the CR looks good I think this is RTBC assuming that test failure is random.

Added a line to the CR that explicitly mentions who is affected, accepted the suggestion, edited the test for the same change.

Committed/pushed to main and 11.x, thanks!

Core tests are failing since this was merged, opened a quick followup MR.

That change looks right, I actually came here to open the follow up fix.

Sorry I don't think I clear in 35, when I said:

I think this is RTBC assuming that test failure is random.

I was trying to flag it was failing and if it passed on rerun it was random since I couldn't rerun it.

I pushed the test fix to 11.x and main - thanks @amateescu and @nicxvan

Ah sorry I missed the failing test but also thanks for fixing it while I was out at the shops for 20 minutes...

Published change record