drupal_get_install_files() is slow with a large tree

subscribing,

had this problem trying to install open atrium today (I know that's Drupal 6, but I checked and this function is the same in 7).

Re-wrote the function in notepad, I haven't rolled a patch before and am currently on a box without the right tools. If this is still here tonight I'll give it a shot, otherwise for people having trouble installing profiles (500 errors, out of memory errors, scritpt timeout errors...) try the following:

function drupal_get_install_files($module_list = array()) {
  $installs = array();
    $installMainfests = drupal_system_listing('.install$', 'modules');
    foreach ($module_list as $module) {
      $installs[$module] = $installManifests[$module];
    }
  return $installs;
}

hmmm... I got stuck on this, got a DB error with the menu router table, so it wouldn't successfully install.

Changed the function to (had some typos):

/**
 * Get list of all .install files.
 *
 * @param $module_list
 *   An array of modules to search for their .install files.
 */
function drupal_get_install_files($module_list = array()) {
  $installs = array();
  $installManifest = drupal_system_listing('/.install$/', 'modules');
  //not every module in module_list has to have an .install file
  foreach ($module_list as $module) {
    if(isset($installManifest[$module])){
      $installs[$module] = $installManifest[$module];
    }
  }
  return $installs;
}

did a print_r on the old and new functions. New function wasn't picking up 'profile.install', I think this is due to a small bug in the former implementation where searching for /file.install$/ would match file.install & profile.install. Adding 'profile' to the default.info profile led to both functions producing the exact same array (in my eyes at least) yet the old function installs, and this one doesn't??

D'oh, first patch I've tried to make. My PDO menu_router error earlier seems to be because I had both install.inc and installNew.inc in the includes directory when making the patch. From what I can see the new class registry system scans for all files ending with .inc, so the classes in install.inc were getting added twice I think? Its strange though cause the error was exactly the same as this: http://drupal.org/node/313606

If the 'doubling up in the registry' sounds right I might go update the 'making a patch' entries in the handbook, as this was the recommended naming scheme for making patches....

Code is the same in D6, since we just today started packaging install profiles on Drupal.org, this is likely to appear more often, so adding backport tag. Also bumping to critical since it prevents people installing profiles without changing PHP settings.

Can't immediately see what's wrong with patch format - please make sure you're rolling with -up and no windows line breaks, although I'm not sure how picky the bot is with that.

Also a couple of code style things:

+  $installManifest

This should be $install_manifest - we don't use camelCase unless it's within a class or interface.

+  //not every module in module_list has to have an .install file

Should be

+  // Not every module in module_list has to have an .install file.

And

+    if(isset($installManifest[$module])){
+      $installs[$module] = $installManifest[$module];
+    }

should be:

+    if (isset($installManifest[$module])) {
+      $installs[$module] = $installManifest[$module];
+    }

See http://drupal.org/coding-standards

Cool thanks for the review, just with the last code format tip, is it the spacing in the
if (condition) {
that wasn't up to standard, or is there something else wrong with it?

Made the patch with winmerge as I'm at work at the moment, so windows line breaks may well be the problem. I'll re-roll tonight using the proper diff tool if no one beats me to it.

Just spacing as far as I can see, actual patch logic looks fine :)

Fixed the coding standards issues catch found, and rerolled using cygwin and diff so hopefully the patch will apply this time!

Hmm... re-downloaded from CVS and ran the patch through dos2unix as well.

Maybe this time the bot will like me.

I think a tab character snuck in instead of space, hopefully for the last time.

~

Could we see some before/after benchmarks on this?

Two minor things:

+  //Not every module in module_list has to have an .install file

Should be a space before Not, a period at the end of the sentence.

$install_manifest

Not sure about this variable name. I wouldn't know what that variable did without reading the code that populated it. Wonder if there's another way to write that (or alternately if I'm just really tired :P)

webchick asked for benchmarks, but I think I must've done something wrong...

First up, couldn't get catch's iteration tester to work, as soon as I included install.inc to it, I was redirected to install.php. To get around it, I just shoved the guts of it into a temporary install.inc to run some benchmarks.

so, replaced current function with:

function drupal_get_install_files($module_list = array()) {
  define('ITERATIONS', 5);
  $start = microtime(true);
  for ($i=0; $i < ITERATIONS; ++$i) {
    $installs = drupal_get_install_files_old($module_list);
    echo "memory usage: ". memory_get_usage() .PHP_EOL;
  }
  $stop = microtime(true);
  echo "function old install files: " . ($stop - $start) . " seconds". PHP_EOL;
  return $installs;
}



function drupal_get_install_files_old($module_list = array()) {
  $installs = array();
  
  foreach ($module_list as $module) {
    $installs = array_merge($installs, drupal_system_listing('/' . $module . '.install$/', 'modules'));
  }
  return $installs;
}

 function drupal_get_install_files_new($module_list = array()) {
  $installs = array();
  $install_manifest = drupal_system_listing('/.install$/', 'modules');
  //Not every module in module_list has to have an .install file
   foreach ($module_list as $module) {
    if (isset($install_manifest[$module])) {
      $installs[$module] = $install_manifest[$module];
    }
   }
   return $installs;
 }

you'll notice the get_install_files now loops 5 times over the old code and reports the time. Got this:

memory usage: 6730128
memory usage: 6730784
memory usage: 6730760
memory usage: 6730752
memory usage: 6730768
function old install files: 53.360243082 seconds

So about 10-11 seconds for the old function.

changed it to use the patched function:

memory usage: 6730120
memory usage: 6730288
memory usage: 6730248
memory usage: 6730256
memory usage: 6730256
function new install files: 2.13886713982 seconds

Not much difference in memory use, but the time has got me thinking I've made a logical error somewhere? Part of me thinks it is right, the real time sink in it is drupal_system_listing so only running that once instead of 27 times (the number of modules in the 'standard' install) really makes a difference.

Nice patch, and definitely looks right for Drupal 6. For Drupal 7, it might be possible to simplify it a bit using array_intersect_key() for PHP5?

However, this got me thinking a bit... why do we have this function at all? It is basically a special-case function for the installer that is closely duplicating what other API functions in Drupal do already. These functions already have some optimization (e.g., static caches and the like), so by simply getting rid of this function and moving the optimization to those other functions instead, we could perhaps wind up with something better that also reduces duplication and that other parts of Drupal can benefit from too.

The attached patch does this. It seems to work fine, although I haven't benchmarked it or anything. Any thoughts?

#16 worked fine for me and has similar performance improvements to earlier patches.

Agree with you about killing the function all together, as you said its duplicating functionality. I don't know much about bootstrap.inc so I can't give any comments on that part of the patch.

Thanks for checking that! Regarding the bootstrap part of the patch, if you want to you can check how it's related by looking at http://api.drupal.org/api/function/module_load_install/7 and clicking through the functions that get called - eventually you wind up at drupal_get_filename().

That drupal_get_filename() function is used in a number of other places (although I think that not too many hit this part of the function, because you can see from the code there that if the database is active and the file being searched for can be found in the database, it stops there). I'm not sure of all the details of how it's used, but my thought was that in general it could only help to cache the extra files - since potentially doing extra scans of the filesystem seems like it should always be much more expensive than potentially picking up and storing a few unneeded filenames while a single filesystem scan is taking place.

+1 subscribing

Looks like a good solution to a long-standing problem. In D5 and D6 my standard profile had ini_set('max_execution_time', 500); at the top since it contained huge folder structures like tinymce, which would often push install.php well over the standard time limit.

I did an informal benchmark, on a macbook running XAMPP. I placed a copy of tinymce in every module folder, for kicks.

HEAD:

wait after selecting language: 20 sec
wait after entering db info: 30 sec
batch took 1:05 to complete

(I ran this three times on HEAD and got the same results)

meatsack's patch:

wait after selecting language: 3 sec
wait after entering db info: 10 sec
batch took: 1:05 to complete

David's patch:

wait after selecting language: 3 sec
wait after entering db info: 10 sec
batch took: 1:05 to complete

Initial results of both patches look great!

David_Rothstein, I think you're right that if the db isn't active, caching all files with the extension while you're scanning is reasonable. Your change to drupal_get_filename() also fixes #659674: drupal_load in a cache backend dies.

#16 looks fairly RTBC but let's get a couple more opinions on it.

I think David's patch is a very nice strike at the heart of the problem.

A note on my benchmarks above, after merging this patch with David's patch in #661420: Installation of modules is hugely inefficient, batch installation went down from 1:05 to just 20 seconds.

Awesome. Nicely done, folks!

Committed to HEAD. Marking down to 6.x.

Is there any interest in this issue? Has this been fixed in the latest D6?

This won't have been fixed in D6.

So shouldn't this issue be moved to the D8 queue if we're not going to do a D6 patch?

First stab at a D6 backport. This would fail tests, because I can't figure out what to do with the 4th hunk. drupal_get_install_files() doesn't exist for D6.