When you have a large install profile containing features, it will slow down to a dismal crawl during batch enabling of modules due to the time taken for features to call features_modules_enabled() - all the time sucked up by that is actually going to features_get_info() because it's loading info on all modules and there's no caching during install.

So here's a much more efficient way of doing it:

/**
 * Implements hook_modules_enabled().
 */
function features_modules_enabled($modules) {
  // Find any features modules that were enabled.
  if (variable_get('install_task') != 'done') {
    // During an installation we want to avoid expensive calls to
    // features_get_features().
    foreach ($modules as $module) {
      $info = drupal_parse_info_file(drupal_get_path('module', $module) . '/' . $module . '.info');
      if (isset($info['features'])) {
        features_enable_feature($module);
      }
    }
  }
  else {
    if ($modules = array_intersect(array_keys(features_get_features()), $modules)) {
      foreach ($modules as $module) {
        features_enable_feature($module);
      }
    }
  }
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

exratione’s picture

Patch attached.

exratione’s picture

Here is a better patch that fixes more of the same issues elsewhere in Features that occur during install profile use.

exratione’s picture

Patch in #2 is not in fact better. Two issues:

1) Stupid typo made in haste,

2) Field features are handled interestingly during an install profile: the actual field tables are only installed at the very end of the profile run when features_flush_caches() is called. This can be very slow, but the install has to happen somewhere. The patch in #2 was overzealous and prevented this install of tables.

Attached is a better better patch. What I'd like to see is the field feature actually create the tables at the time of install rather than at the very end of the install process - having it work this way means that no other module can assume the existence of those tables, which is somewhat inconvenient.

hefox’s picture

Status: Active » Needs work
+++ b/features.module
@@ -213,9 +213,14 @@ function features_theme() {
+  if (variable_get('install_task') != 'done') {
+    features_rebuild();
+    return array();
+  } else {
+    features_rebuild();
+    features_get_modules(NULL, TRUE);
+    return array();
+  }

Two things: why not put the if (variable_get just around the divergence point, e.g. features_get_modules?
Other than that, coding style issue with the else.

+++ b/features.module
@@ -278,9 +283,21 @@ function features_modules_disabled($modules) {
+      $info = drupal_parse_info_file(drupal_get_path('module', $module) . '/' . $module . '.info');

Assuming there's no cached function with this info? Also, bypasses hook_system_alter (if that's still in d7).

then checking of install mode and using parse file; it feels sorta misplaced. Perhaps features_load and features_get_features should be handling that logic instead.

exratione’s picture

My patches should of course be taken as illustrative only; they are more in the way of pointing out the problem than anything else. They form the quick win for my present situation, not what should finally happen.

Patch in #3 gives a speedup of 3x or so, which makes an enormous difference in our install profile, since we have on the order of a megabyte of features module files in there.

There are only static caches available during install. So use of all functions and data structures that are costly to build and normally cache has to be examined carefully.

bojanz’s picture

Status: Needs work » Needs review
FileSize
3.91 KB

Let's try one more time.

1) Added the info parsing to feature_load(). The returned object does not contain all the information (since we are not querying the system table), just the name, type, and info file, which is what we need during install (actually only $file->info['features'] is examined);
2) Cleaned up features_modules_enabled() and features_modules_disabled() to avoid calling features_get_features().
Note that the current code means that during install the info file of each installed module will still get parsed, which is still not ideal.
Opened #1572578: Rethink the way features are enabled / disabled to get consensus on fixing that and other related problems.
3) Removed features_enable_feature() and features_disable_feature() since they are no longer used. However, there's nothing wrong with them, so we can put them back if that's preferable (some maintainers dislike dead code, other dislike breaking APIs...)

Also includes the patch from #1265168: Rebuild the file list properly when a feature is enabled or disabled, so I've closed that issue as duplicate.

bojanz’s picture

Title: features_modules_enabled() exceedingly and unnecessarily slow during install profile » Avoid unnecessary cache rebuilds and improve installation performance
Category: bug » task

Retitling.

Would like to hear from exratione if the current patch still offers the same performance improvements. On my side the installation feels snappier, but I haven't measured it.

EDIT: Here's a related issue, this time for field handling: #1574716: Avoid unnecessary cache rebuilds when creating and updating fields.

exratione’s picture

I'll get to evaluating the patch in #6 shortly.

As an aside, we're noticing a slowdown again for our initial patch since updating to 7.14 code, so I was planning to revisit profiling of the features code in our install profile to see what might have happened there - but hopefully #6 catches whatever case I missed.

I'm also due an update of features version; we're still on 7.x-1.0-beta6 at present.

exratione’s picture

I updated to the dev branch and patched with patch #6. That provides the same performance boost as my original patch running on 7.x-1.0-beta6. So good - it works as advertised.

I'm off to look at the related issue you pointed out, thanks for that.

kotnik’s picture

Patch from #6 breaks features' drush support.

As you can see here and here, we need feature module filename as well.

Patch attached adds that information.

dixon_’s picture

We've seen huge performance improvements with this patch. Installation of our in-house distribution went from ~ 90 sec down to ~ 60 sec. I know it doesn't say much, but as a relative measure it's quite good.

exratione’s picture

You might also want to look at this issue (and suggested hacky solution), which has caused a big slowdown for our install profile in 7.14: #1599146: field_modules_enabled() greatly slows down profile installs, especially with features modules included

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready. We've been using it in our install profile for more than a month, plus there's #11.

mpotter’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Seems to work for my sites here as well, so committed and pushed a858e77.

pfrenssen’s picture

This causes the test FeaturesCtoolsIntegrationTest to fail. See issue #1804262: Chaos Tools test fails with latest versions of core + contrib.

mpotter’s picture

dooug’s picture

Is anyone working on porting this to 6.x-1.x-dev? I took a stab at it, but this is beyond my understanding of features (and time to figure it out).

I work on a site with 10+ features, and any feature drush command can take minutes. So, any performance improvement would be greatly appreciated!!

  • mpotter committed a858e77 on 8.x-3.x
    Issue #1530386 by exratione, bojanz, kotnik: Avoid unnecessary cache...