Unit test performance significantly decreased since system list config conversion

+++ b/core/includes/bootstrap.inc
@@ -884,7 +884,7 @@ function drupal_get_filename($type, $name, $filename = NULL) {
-  if (!empty($filename) && file_exists($filename)) {
+  if (!empty($filename)) {
     $files[$type][$name] = $filename;
   }

Coudn't this cause troubles with removed modules and things like that? Or is that handled in the changed code below, haven't read that in detail.

+++ b/core/scripts/run-tests.sh
@@ -190,6 +190,7 @@ function simpletest_script_parse_args() {
+    'profile' => FALSE,
     'test_names' => array(),
     // Used internally.
     'test-id' => 0,
@@ -408,7 +409,11 @@ function simpletest_script_run_one_test($test_id, $test_class) {

@@ -408,7 +409,11 @@ function simpletest_script_run_one_test($test_id, $test_class) {
 function simpletest_script_command($test_id, $test_class) {
   global $args, $php;
 
-  $command = escapeshellarg($php) . ' ' . escapeshellarg('./core/scripts/' . $args['script']);
+  $command = escapeshellarg($php);
+  if ($args['profile']) {
+    $command .= ' -d xdebug.profiler_enable=On';
+  }

This looks useful, I've thought about this as well. Wondering if we should get it in as a separate issue, though, because we're for example missing documentation here.

Just a note on file_exists(), there's #1715222: Remove almost all file_exists() and is_file() calls from core and #752730: Remove file_exists() during bootstrap so I'm fine with that being done (we might need to mark those as duplicate unless they're touching other areas). We already use include_once() in drupal_load(), so it's a non-fatal error there, but yeah it'd be good to check for require_once() in other situations.

Patch looks good so far.

Looks good, don't know it enough, else I would say its RTBC.

Same feedback as Fabianx, this looks ok, catch confirmed that removing the file_exists() there is fine. I don't really know that area well enough either to RTBC.

Some more notes/feedback:

I picked a random unit test that seems to be running fast and executed it a few times. These improvements are low-level and generic, it doesn't matter if we run different tests or repeat the same one.

$ php core/scripts/run-tests.sh --url http://d8/ --class "Drupal\system\Tests\Uuid\UuidUnitTest,Drupal\system\Tests\Uuid\UuidUnitTest,Drupal\system\Tests\Uuid\UuidUnitTest,Drupal\system\Tests\Uuid\UuidUnitTest,Drupal\system\Tests\Uuid\UuidUnitTest,Drupal\system\Tests\Uuid\UuidUnitTest,Drupal\system\Tests\Uuid\UuidUnitTest,Drupal\system\Tests\Uuid\UuidUnitTest,Drupal\system\Tests\Uuid\UuidUnitTest"

Execution time before the patch: 7s, with the patch, 3s. I tried running different tests and other tests too and while the improvements are always a bit different based on the actual execution time of the test, it always was a considerable improvement, especially considering that many of these improvements also apply to normal page requests, especially when simpletest.module is enabled.

So, possible remaining tasks:
- Maybe someone with a deep knowledge of drupal_system_listing() and drupal_get_filename() functions can sign this off?
- The problem with excluding config also indicates that it is no longer possible to have modules called lib, templates, css or js. js/css/templates exist in 6.x versions on d.o. Is that really ok? If so, we should document it somewhere (not necessarly in this issue/code, but somewhere)
- We should check the issues mentioned by catch if they're still relevant after this and if there are maybe any indications in the discussions there about things that could be included here as well or need to be done differently.
- EDIT: Also, we should confirm that it's possible to remove modules without completely breaking the site. This is something that happens quite frequently when working together on sites in a team/multiple environments. Idea: Mess with the stored filename of a module (we still store that somewhere, right?) to fake a deletion. Check what happens.

+++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/GetFilenameUnitTest.phpundefined
@@ -50,6 +60,6 @@ function testDrupalGetFilename() {
     // for (e.g.) 'module' files in core/modules.
-    $this->assertIdentical(drupal_get_filename('script', 'test'), 'core/scripts/test.script', 'Retrieve test script location.');
+    $this->assertIdentical(drupal_get_filename('script', 'test'), 'core/scripts/test/test.script');
   }

Is the removal of the message on purpose?

Besides that it is RTBC now!

#23: Makes sense.

Looks good then for me. => RTBC

Committed and pushed to 8.x. Awesome!

Random testbot result before this patch: http://qa.drupal.org/pifr/test/379968 shows about a 45 minute test run. Let's see what happens next.

I thought this patch was supposed to help somehow, but the 8.x branch test is actually 5 minutes slower after the commit, from 45 to 50 minutes..

This does help.

Testbots are different, they take between 45 and almost 60 minutes for a test run. It's not possible to compare two different runs without at least making sure that it was executed on the same bot and even then there could be considerable differences (I think testbots are all virtual servers, so something else on the same physical machine could have affected it).