Installation of modules is hugely inefficient

I think we'd all like to be able to run the installer in 6 seconds instead of 16 seconds, and cutting 3MB of memory usage helps us get back under 32MB which is really shocking, so bumping status to critical.

Sub.

sub

This problem has been around a while. A while back I dissected the function calls in D6 that parse info files during install.php (since it was consistently hogging enormous resources), and I was very sorry I did. It's a real mess :P The problem is when you have lots of directories in your install profile, i.e. tinymce, things kind of blow up.

I would love to see progress on this, not just for shared hosts with ~ 32 MB memory. Since with the introduction of drush installsite Drupal sites can be set up programmatically--I know a lot of software-as-a-service applications that would benefit from the installer being 10 seconds faster and not devouring the poor server's resources.

I don't know why we lost that fakeCache during install. seems quite a good idea to me ... there some ninja debugging left in the patch.

looks interesting to me. i think we need some registry experts in here like chx, crell, tha_sun, .... i don't see handling when disabling modules which maybe problematic.

Remove misleading title (not about batch api)

Any reason we can't just do something like the attached patch? It seems to provide the same speedup for me, and no obvious ill effects.

I do not consider myself being a Drupal core expert, but I decided to do a small research on general PHP memory gotchas - it appears that we could gain some memory if we consider the following items:

1. Reduce the scope of variables to the strict minimum required to function properly. After this, one may decide to trade memory for speed.
2. When using classes in PHP, we need to make sure they implement __unset() in order to allow unset() to properly clear resources.
3. If possible, we should avoid StdClass objects as they are known to leak memory (they're flexible objects, and it's not straightforward for the PHP interpreter to release their memory in use).
4. Watch out for objects with recursive references as they're known to leak memory. There are workarounds but they must be implemented and called. Apparently these workarounds are no longer needed when you're running PHP 5.3 or newer; however it appears you then still need to manually enable the garbage collector once by calling gc_enable()).
5. There's apparently a PHP memory leak in certain PHP versions when calling the include() method.

Hope this helps.

Olivier

@justinrandell (#24): That's certainly true, but also true of dozens of other places that query the {system} table. I think in general we don't expect that changes to the filesystem are immediately picked up by Drupal, and that it's up to the calling code to call system_rebuild_module_data() if it cares about the difference. So for example, one thing my patch should have done (but didn't do) is add an explicit call to system_rebuild_module_data() inside drupal_flush_all_caches(), since I removed the implicit one.... but that's a somewhat rare case.

I guess I can see why the registry might be an exception to this rule, though. And registry_add_modules() is a nice new function - just seems like it might be introducing a fair amount of duplicate code (at least at the moment).

@ShutterFreak: Can you post links for the first three points you mention? I'm familiar with 4, but 2 and 3 in particular surprise me. (And #5 doesn't surprise me at all, given that it is PHP...)

Drupal uses stdClass objects, um, *everywhere*. :-)

@Crell: here's one link explaining #1, #2 and #3: Why does this simple php script leak memory? | StackOverflow

In addition I came across another leaky PHP object pattern. The following will leak memory:

 foreach ($rss->channel as $channel) {
  /* do something */
} 

and the following apparently doesn't (or does to a lesser extent):

 $foo = $rss->channel;
foreach ($foo as $channel) {
  /* do something */
} 

At least, on certain PHP versions.

Finally it appears the ($list as $element) is also an interesting candidate for leaking memory as $element is not unset after running the iteration. This can be costly in terms of memory if the iterator runs over objects (see the other object related remarks regarding destructors).

Wow, that looks like a much more complicated patch than David's. I wonder why you considered David's approach to be "too simple".

Looks like the $exclude_files patch got mixed in with the last one.

I'd be very wary of anything which reduces the change of picking up filesystem changes, we already have outstanding bugs like #605374: Dont add nonexisting files to the registry

I ran the drush installsite command while tracing was enabled in xdebug. This installs Drupal in one php invocation - no batch API. Here is the ugliness, sorted by memory-own. As mentioned earlier, get_defined_constants() is killer. good news is that we have some optimization opportunities here. See http://drupal.org/node/659980 for the analysis tools.

                                               Inclusive        Own
function                               #calls  time     memory  time     memory
-------------------------------------------------------------------------------
get_defined_constants                    2818  1.0079 720178048  1.0079 720178048
system_schema                             150  0.0636 31649840  0.0636 31649840
preg_match_all                           3001  0.4581 30317896  0.4581 30317896
_menu_router_cache                        936  0.1739 23520568  0.1100 22988616
array_merge                              6435  0.1375 22192248  0.1375 22192248
explode                                 46766  0.8950 17583112  0.8950 17583112
DatabaseCondition->compile               8211  1.4374 23838472  0.8333 16859728
get_defined_functions                      23  0.0280 14049456  0.0280 14049456
file_uri_scheme                         42960  3.3510 26801904  1.9458 11758448
preg_split                              26544  0.4198 10362776  0.4198 10362776
module_list                              2616  0.3920 10750568  0.0790  9639864
drupal_load                                88  0.1189 10191600  0.0817  8664672
PDOStatement->fetch                      2444  0.0598  7873608  0.0598  7873608
PDOStatement->execute                    5823  9.6628  7421088  9.6628  7421088
SelectQuery->addField                    5971  0.1583  7061728  0.1583  7061728
InsertQuery->values                      1247  0.2209  8977776  0.1680  6377624
DatabaseCondition->condition             5378  0.2392  5889272  0.1715  5889272
file_get_contents                        3001  0.2870  5344952  0.2870  5344952
_drupal_depth_first_search               3675  0.1899  5556824  0.1573  4777616
DatabaseConnection->defaultOptions       5823  0.1183  4638264  0.1183  4638264
_menu_link_build                          925  0.0391  4635352  0.0391  4635352
implode                                 21348  0.3053  4390320  0.3053  4390320
DatabaseCondition->mapConditionOperator  6185  0.1464  3708016  0.1460  3707536
install_begin_request                       1  0.1966  6992216  0.0267  3685472
DatabaseConnection->expandArguments      5823  1.1516  3447728  0.5928  3445192

Is the 2818 calls to get_defined_constants in #36 that makes the memory increase?

Can we use one cached drupal_get_defined_constats function, will improve?

Subscribing.

@justinrandell: If I am not mistaken the theme registry was required to be invoked since Drupal 6 (D6) when performing some routine administrative tasks, e.g., visit the module list (admin/build/modules). This is apparently one of the main reasons why Drupal 5 (D5) is so fast. I don't know however which 'fundamental' change happened between D5 and D6 to require the theme registry (and menu router) to be rebuilt so frequently since D6.

It would be great if we were able to defer rebuilding the theme registry (and probably the menu router as well) until the point where it only needs to be run once. Or twice at most. But I fear this won't be easy to fix without changing the Drupal core API.

I have the impression there are a couple hooks that interfere (e.g., run hook_FOO requires to check which other FOO hooks were called, or will be called) and as a side effect require triggering a theme (or menu router) rebuild. Maybe those hooks are doing 2 unrelated things hence they should be split?

@ShutterFreak: Drupal 5 had no theme registry and the menu system was completely different. Those systems as we know them now didn't exist in D5.

Do you know of any such hooks that indirectly trigger rebuilds? I can't think of any off hand that would do so at odd times, and even without that the memory usage and page runtime are simply unacceptable at the moment.

@Crell: Unfortunately no. I am not too acquainted with the entire Drupal core API, and it's starting to become rather difficult to still understand what is going on when registering a module nowadays.

The only hook-linked method call I know propagates into all modules implementing a certain hook is module_invoke_all(). This method happens to be triggered in quite some places, and even a lot more in D7 than in D6, at least according to the API documentation.

module_invoke_all() is called a ton throughout Drupal, as it is one of Drupal's main bend-points for extensibility. It does not itself trigger a menu rebuild or a theme rebuild, though, and if anything it should be cheaper in Drupal 7 than it was in Drupal 6.

Re #34, #35 and others - if we want to preserve the current behavior of registry_rebuild(), there's still a much simpler way to do it. See the attached patch.

I do agree that the registry_add_files() approach is a bit cleaner in terms of API, but it's also adding a fair amount of complexity to the registry. And I'm not sure how that works with hook_registry_files_alter(); the latest patch in #40 just removes that hook altogether? ... But, any patch that gets rid of that crazy $disable_modules_installed_hook parameter can't be too bad :)

+1 to #48, it makes a lot of sense to split that up into two functions. module_enable() is so often used in loops and doing system_rebuild_module_data() every time is just silly :P

I merged David's patch from #358815: drupal_get_install_files() is slow with a large tree with #48, and wow. We have a very very much faster installer.

In my benchmark I used the default profile and sprinkled my module folders with tinymce to simulate a real-life module folder mess. Regardless of how many modules are in the profile, each folder takes extra resources to scan for .info files. I also used a macbook for my server which isn't incredibly fast.

HEAD (I ran this 3x for reinforcement):

wait after selecting language: 20 sec
wait after entering db info: 30 sec
batch took: 1 min 5 sec

w/ David's merged patches:

wait after selecting language: 3 sec
wait after entering db info: 10 sec
batch took: 20 sec (!!)

So that's 115 seconds down to 33 seconds. The difference is exaggerated since I have so many useless folders in there, but there are lots of cases where you have a pre-packaged Drupal app with tons of folders.

drush installsite would of course run faster still, since the batch API overhead is skipped.

Committed to CVS HEAD. We can't keep waiting for the test bot to come back so I ran the tests locally. I got 3 failures but they seemed unrelated. Hopefully someone can kick the testbots and figure out those failures in parallel. If not, I can always stop committing patches later on. Thanks all.

Hope it's alright to get this ported to 6.x

Not possible. D7 has changed too much.

Right, I would suggest creating a different issue if there is a problem in D6 that can be fixed.

(The patch committed here involved fixes to the registry, which did not exist in D6 at all, so it would not be a matter of porting the patch but rather writing an entirely different, unrelated one.)