What i originally thought was an issue with lots of aliases is actually a silly display issue. I have designed a serie of unit tests for output.inc to cover that. It seems the bug lies within either Console_Table or within drush_table_column_autowidth(), xdebug profiling pointing to the latter, which abuses arrays a lot and could certainly use an optimization.

This problem becomes obvious when you run "drush @foo status" on any Aegir install with more than (say) a hundred sites. Drush will try to display the list of all aliases and this will take forever (12 seconds here on 200+ sites aegir install).

A test case

I have tried my luck at testing this through unit testing instead of writing a random script like last time. I am marking the issue as needs review, not because I have a fix for the actual issue, but because I have a test case that shows it breaking. Oh and note that I found another anomaly in the output of the table formatting, but I consider this to be a Console_Table bug, so I'm ignoring it right now. Once the unit test is accepted, this should be marked "needs work" until the performance issue is fixed.

Note how I built the test case here - I actually bootstrap drush and run it directly within the environment, which allows me to test drush functions directly. Not sure this is the right way to do things, but it sure does work for me. For this to work, I had to modify the alias bootstrap code too to ignore some errors (ie. when no argument is passed to drush).

I also do not derive the builtin Drush_TestCase class to make use of the PerformanceTestCase...

http://www.phpunit.de/manual/3.0/en/testcase-extensions.html#testcase-ex...

I think it's rather stupid of PHPUnit to split this in a different class, maybe we want to refactor the Drush_TestCase class to allow for performance tests too, or directly derive from that class instead of the base class.

At the very least the bootstrap code should probably be factored into the base class so that we can test functions instead of just testing commands.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anarcat’s picture

A workaround for this issue would be #1132906: lazy-loading of drush aliases.

anarcat’s picture

I have refactored the bootstrapping code in a new class here: #1133284: separate functional from unit tests.. With that patch, the patch here becomes the attached.

Note that because of single inheritance, we need to roll out our own timer functions, which I added to the base class. Besides, that can be useful in other places anyways *and* the PerformanceTest is dropped in later versions of PHPUnit...

moshe weitzman’s picture

Status: Needs review » Postponed (maintainer needs more info)

I agree that it is silly to do PerformanceTestCase as a separate class. I would add its functionality to our base class but I can't think of a need for timing tests.

anarcat’s picture

Version: » All-versions-4.x-dev
Status: Postponed (maintainer needs more info) » Needs review

well, the idea here is to be able to test if some functions are performing in a reasonable amount of time.

for example here in aegir we have around 300 aliases which, when loaded by php-console-table, take around 5 minutes to render (!) (see #2047863: php takes up 100% CPU on drush @hostmaster status for details). Not fun.

having this as a unit test outlines this bug, which probably lies within php-console-table more than drush, but still, this is the kind of stuff we could test in other places...

greg.1.anderson’s picture

Version: All-versions-4.x-dev » 8.x-6.x-dev
Status: Needs review » Closed (won't fix)
Issue tags: +Needs migration

This issue was marked closed (won't fix) because Drush has moved to Github.

If desired, you may copy this bug to our Github project and then post a link here to the new issue. Please also change the status of this issue to closed (duplicate).

Please ask support questions on Drupal Answers.