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.

CommentFileSizeAuthor
#9 features-2071859-improve-drush-performance-d6-9.patch7.87 KBq0rban
#1 features-2071859-improve-drush-performance-1.patch7.96 KBq0rban
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

q0rban’s picture

q0rban CreditAttribution: q0rban commented
Title: Improve the performance of drush features-revert-all (fra) » Improve the performance and output of drush commands for sites with lots of features
FileSize
features-2071859-improve-drush-performance-1.patch7.96 KB

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

mpotter CreditAttribution: mpotter commented
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

hefox CreditAttribution: hefox commented

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

q0rban CreditAttribution: q0rban commented

Whoops, sorry about that @mpotter. :)

markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison commented

q0rban++

davidburns’s picture

davidburns
he/him
Primary language English
Location Philadelphia
CreditAttribution: davidburns commented
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

mpotter CreditAttribution: mpotter commented
Status: Reviewed & tested by the community » Fixed

Committed to 7d56c4f.

q0rban’s picture

q0rban CreditAttribution: q0rban commented
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

q0rban CreditAttribution: q0rban commented
Status: Patch (to be ported) » Needs review
FileSize
features-2071859-improve-drush-performance-d6-9.patch7.87 KB
djdevin’s picture

djdevin
Location Philadelphia
CreditAttribution: djdevin commented

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

apotek’s picture

apotek CreditAttribution: apotek commented

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

Matt V.’s picture

Matt V. CreditAttribution: Matt V. commented

@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.

apotek’s picture

apotek CreditAttribution: apotek commented
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...
kenorb’s picture

kenorb CreditAttribution: kenorb commented
Version: 6.x-1.x-dev » 7.x-2.x-dev
Status: Needs review » Fixed

Marking as fixed, as 6.x is no longer supported.

kenorb’s picture

kenorb CreditAttribution: kenorb commented

Btw. This change by changing drush_invoke_process into drush_invoke caused disadvantage that now the feature command can't be altered.

This is trouble for me, because I wanted to add extra logic before running the feature revert, and I can't do that for fra, because not all arguments are available.

It's maybe quicker, but made things less flexible. So I'll have to write my own wrapper anyway to use drush_invoke_process instead.

Or patch the Features to use drush_invoke_process again, e.g.

drush_invoke_process("@self", "features-revert", drupal_map_assoc($features_to_revert));

See: How to alter command being run by drush_invoke?

mpotter’s picture

mpotter CreditAttribution: mpotter at Phase2 commented

Please open a new issue for that and suggest a patch. Although rather than changing the drush commands I'd probably recommend implementing one of the many hooks that get called before or after various features operations, such as revert, rebuild, etc.

Also, you'll need to show some performance numbers indicating that changing back to drush_invoke_process() doesn't re-introduce the performance issue that was fixed here.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.