Currently, drush features-revert-all is very slow on a site with many feature modules. A quick look in the command callback function reveals that drush_invoke_process() is called for each feature, which means that Drupal is bootstrapped once for every feature module on the site. This is should be an easy win, but there might be other performance gains to be had.

#9 features-2071859-improve-drush-performance-d6-9.patch7.87 KBq0rban
None View
#1 features-2071859-improve-drush-performance-1.patch7.96 KBq0rban
PASSED: [[SimpleTest]]: [MySQL] 36 pass(es). View


q0rban’s picture

Title:Improve the performance of drush features-revert-all (fra)» Improve the performance and output of drush commands for sites with lots of features
new7.96 KB
PASSED: [[SimpleTest]]: [MySQL] 36 pass(es). View

In doing this, I realized there were other commands using drush_invoke_process() instead of drush_invoke(), so I am expanding this issue to include those as well. In general, these commands are just not optimized for sites with lots of features, so I'm going to be adding some other tweaks to improve that.

Tested this out with drush features-revert-all -y on a site with just 14 features.

Timer before patch:

real 2m8.094s
user 1m44.729s
sys 0m5.251s

Timer after patch:

real 0m41.559s
user 0m31.515s
sys 0m1.151s

Here's the same test run with --force, increasing the number of feature modules to 16, but more importantly, increasing the number of components that are reverted. I didn't bother counting. :)

Timer before patch:

real 3m1.569s
user 2m21.260s
sys 0m15.798s

Timer after patch:

real 1m47.829s
user 1m21.698s
sys 0m8.041s

This patch includes the following changes:

  • Switch from using drush_invoke_process() to drush_invoke() in all commands, so that Drupal doesn't need to be bootstrapped for each feature. This makes command execution 2-3 times faster in this example.
  • If -y is passed to features-revert-all, the confirmation message is silenced to avoid extraneous output from the command.
  • If -y is passed to features-update-all, the confirmation message is again silenced.
  • The output message in features-revert-all now lists the feature name and component name. Before, just the component name was listed, so you weren't sure what feature it related to.
  • Switch to passing all feature modules directly to drush fr as arguments, instead of looping over them and doing individual calls to drush fr. This should lower the memory footprint of the command.
  • Cleans up some deeply nested code, for easier readability.
  • Switches from else if to elseif.
  • Adds some extra documentation where there was none before.
  • Use drush_log() instead of drush_print() to print out the warning that a module already exists in features-update-all.
mpotter’s picture

Status:Active» Needs review

Need to use the "needs review" status if you want the tester bot to run tests against this.

hefox’s picture

I've thought about this before and reason haven't persuade it is worry about timeouts, but as there's also a patch to change memory limit for features_revert/rebuild, that doesn't seem an issue. Looks good from a brief lookover, but haven't tested

q0rban’s picture

Whoops, sorry about that @mpotter. :)

markdorison’s picture


davexoxide’s picture

Status:Needs review» Reviewed & tested by the community

I've been developing sites with this patch for almost 2 weeks without any issues, only performance improvements.
I think this is RTBC.

mpotter’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 7d56c4f.

q0rban’s picture

Version:7.x-2.x-dev» 6.x-1.x-dev
Status:Fixed» Patch (to be ported)

Yay! I need this for some d6 sites as well. Should be fairly trivial to backport.

q0rban’s picture

Status:Patch (to be ported)» Needs review
new7.87 KB
None View
djdevin’s picture

Looks good, running this on our stage deployments with about 20 features.

apotek’s picture

A useful, tested, RTBC patch that constitutes a real improvement on both D6 and D7 and.... *crickets*

Matt V.’s picture


The D7 version of the patch has already been applied.

The D6 version is still marked "Needs review." If you're interested in moving the issue along, you might consider reviewing/testing the D6 version of the patch in Comment #9 above.

apotek’s picture

Issue summary:View changes

@Matt V

Thank you for the reply. Since the patch in comment 9 is written against the -dev version of features (appropriately), we won't really be able to test the patch with our current production codebase, so it wouldn't be a like-for-like test.

I'm going to see if I can write a modified version of the patch against the 6.x-1.2 release so we can at least test the concept.

  • mpotter committed 7d56c4f on 8.x-3.x
    Issue #2071859 by q0rban: Improve the performance and output of drush...