Regression in functional test performance with a large number of modules

Fixed Failed commands on #6
Attached patch

@phma really nice find.

+++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
@@ -279,10 +287,13 @@ public function getModuleUpdateFunctions($module_name) {
-    $extension_discovery = new ExtensionDiscovery($this->root, FALSE, [], $this->sitePath);
-    $module_extensions = $extension_discovery->scan('module');
-    $theme_extensions = $this->includeThemes() ? $extension_discovery->scan('theme') : [];
-    $profile_extensions = $extension_discovery->scan('profile');
+    if (!isset($this->extensionDiscovery)) {
+      $use_file_cache = Settings::get('update_registry_use_file_cache', FALSE);
+      $this->extensionDiscovery = new ExtensionDiscovery($this->root, $use_file_cache, [], $this->sitePath);
+    }
+    $module_extensions = $this->extensionDiscovery->scan('module');
+    $theme_extensions = $this->includeThemes() ? $this->extensionDiscovery->scan('theme') : [];
+    $profile_extensions = $this->extensionDiscovery->scan('profile');

I think we should consider setting file cache to TRUE I think we should be using it here. I'm on the fence about adding an extension discovery as a class property. I think if we do this we should do this in the constructor.

We need to use extension discovery here because this is happening very very early... i.e. before the module handler is updated with the new module info.

I think we can make this much better by limiting the extensions returned by scanExtensionsAndLoadUpdateFiles() in specific circumstances... see patch attached. How does this compare for you?

@phma I think it is worth doing these separately. It's really interesting that basename() is so expensive. It would also be great to know what was the test run time with #3259188: Extend post update system to provide themes a way to install newly-required dependencies reverted.

+++ b/core/lib/Drupal/Core/Extension/Extension.php
@@ -61,14 +68,17 @@ class Extension {
    */
-  public function __construct($root, $type, $pathname, $filename = NULL) {
+  public function __construct($root, $type, $pathname, $filename = NULL, $name = NULL) {
     // @see \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName()
     assert($pathname === 'core/core.info.yml' || ($pathname[0] !== '/' && file_exists($root . '/' . $pathname)), sprintf('The file specified by the given app root, relative path and file name (%s) do not exist.', $root . '/' . $pathname));

Should this always be optional or should it be the penultimate argument and deprecate passing NULL?

Bringing back #10 as I think this is the patch to commit on this one. And yeah I'm suspicious about the cost of profiling and benefit of the changing the basename. #10 just does not do as much.

We could file a separate issue for #13 but in order to prove that that is necessary I think we'd need to profile it without any profiling tools and just look at raw execution times.

@phma can we do the smaller change that brings us back in line with the before times that's in #16 and make further improvements in separate issues?

I don't think the scan extensions flag is necessary and it add complexity and new ways to fail. Upping #9 again.

Alright, first off, this seems fine. I've gone over it a bit, I see no problems here, just an obvious efficiency improvement code-wise. I wonder if it could be improved slightly more with

    // Limit to a single extension.
    if ($extension && isset($extensions[$extension]) && in_array($extension, $this->enabledExtensions)) {
      $this->loadUpdateFile($extensions[$extension]);
    }
    else {
      $this->loadUpdateFiles($extensions);
    }

Just to avoid looping over all the enabled extensions in loadUpdateFiles() when we know there's only one we are interested in, but we are really talking minimal cycles saved there, and the current patch code is cleaner anyway.

Sadly, I was unable to really get a feel for what this saves in the typical case. I don't have a functional test that enables 150 modules, but I wanted to see what it would do for the typical core test suite and the improvement was not as exciting as I hoped. I ran the migrate MultilingualReviewPage test half a dozen times locally, which enables 10ish modules, and got averages of 39.47s without and 39.23s with this patch, which are well within the range of times this test took locally. Looking at the 3 times Alex posted the same patch, we have Functional test runtimes of 23:18m, 31:42m, 31:02m (avarege of 28:40m) against 9.4, and between the latest branch test and the one from yesterday, we have times of 30.47m and 31.43m for an average of 31:15, but if I take the 23:18 out as an outlier, then with this patch the average is 31:22, again with all the variance of machines we get spot instances of on amazon, its all well within margin of error.

So, I say it's still a good improvement, if not as useful in saving the DA on the AWS bill as I had hoped it would be. It will definitely help for some specialized tests.

Thanks for getting those numbers @mikelutz, I think that proves that this is a small but real improvement for core +1

There's no indication that #21 meant to un-rtbc this :)

Committed/pushed to 10.1.x, cherry-picked to 10.0.x, 9.5.x, and 9.4.x, thanks!