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.

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

Comments

Title:Improve the performance of drush features-revert-all (fra)Improve the performance and output of drush commands for sites with lots of features
StatusFileSize
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.

Status:Active» Needs review

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

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

Whoops, sorry about that @mpotter. :)

q0rban++

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.

Status:Reviewed & tested by the community» Fixed

Committed to 7d56c4f.

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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new7.87 KB
Test request sent.
[ View ]

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

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

@apotek

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.