Add concurrency to pm-download by default

There's an easy way and an ambitious way to handle this.

The ambitious way would be to add concurrency as a general-purpose feature of command records. If 'concurrency' => TRUE is set, and there is more than one command argument, then drush_dispatch will use drush_backend_invoke_concurrent to re-run the same command N times for the N arguments, with one argument per invocation. When there is only one argument, then it is dispatched straight through as usual. If we do this once, then any command could quickly and easily become concurrent. This would be very slick; however, I don't know how many commands would fit into this category. I also wonder if it might mess up scripts that called a command with multiple args, & were not expecting concurrent behavior, I suppose if we added extra code at the end to roll together the backend results... not sure if that works.

The more straightforward way would be to have pm-download itself check its argument count, and call drush_backend_invoke_concurrent very much the same way as described above. Thinking this "simpler" implementation through, I think that it's about the same code and has about the same issues as the general-purpose implementation above, so perhaps general-purpose is the way to go here.

This probably isn't much code, but I won't have time to take it on.

I forgot about confirmation messages; that's actually a sticky wicket, when you consider that different commands might have different things they want to confirm about. I think that overall the implementation and placement of #3 is good, but I'm going to suggest that we also require the user to specify --yes (explicitly or implicitly with --backend) in order to get concurrency. If you want to respond to the confirmation messages, you have to do it individually run the command non-concurrently. The alternative would be to go back to the idea of per-command concurrency, and put the drush_backend_invoke_concurrent call after the confirmation message. I think I prefer the current way better.

As far as output & c. is concerned, we could modify drush_confirm to supress all messages when --backend is set. That should clear things up nicely in a number of scenarios, and since --backend implies --yes already, it won't affect functionality much.

Yes, --backend still implies --yes. "Interactive" backend calls go through drush_shell_proc_open, and bypass backend invoke. We did discuss adding interactivity to drush_proc_open, but that hasn't been done yet.

I think that if you disable concurrency when --yes is not present, we can keep the general implementation.

Patch is looking good.

I think you mean drush_get_option('concurrency', FALSE) here:

+          if ($affirmative && $auto && count($command['arguments']) >= 2 && drush_get_option('concurrency', 4)) {

Or, based on your comment, perhaps what you mean is:

+          if ($affirmative && $auto && count($command['arguments']) >= 2 && (drush_get_option('concurrency', 4) > 1)) {

Sure, '0' is 'disable', but it doesn't really make sense to allow concurrency == 1. Might as well let the regular code run in this case. The comment can stay the same, though.

Concurrency in the sandwich command? Madness, I say. If you want to get really crazy here, you could have each process responsible for printing every N'th line, and use the drush logging mechanism to coordinate them. Well, that doesn't quite work. It would be easy to get the sandwich processes to send messages back to the parent process (indicating that they rendered their line, and the next process can go now), drush_proc_open is still closing stdin as soon as it writes the POST data, so there's no good way for the parent to communicate with the child process. Oh well. Printing N interleaved sandwiches is cool too.

I agree; there seems to be something rotten in the State of Backendinvokeconcurrenclyloggingmark. Sometimes, log messages are emitted twice. I don't know if that is related, but both Drush make and Drush site-upgrade do this. I will post an example of the later at #1410344: Remove site-upgrade command from Drush-4.x.. A new issue to track backend invoke logging would be helpful; I'll open one later when I have time, if someone doesn't get to it first.

The double output issue was fixed in make, and that seems to have cleared it up for this patch as well.

Re-rolled for latest 8.x-6.x. It does work, but does not make much of a difference in execution times.

Oh, here is the patch.

If we commit this, I think we should first bump the minimum project count up to ~8 or so.

Hi

I've tested the patch and it has been well applied. During the download of multiple modules we can see processes running at the same time.

To answer Moshe's suggestion I've made some tests and tried to download modules without and with the patch. I've measured the execution time of the drush_main() function. I used 15 random modules for my tests (views ctools context devel feeds token admin_menu pathauto webform captcha panels date xmlsitemap jquery_update menu_block)

Number of modules | Time dl without the patch | Time dl with the patch
1 | 7.66s | 11s
2 | 14.35s | 5.57s
3 | 17.21 | 17.05s
4 | 19.37 | 18.63s
5 | 23.68s | 26.71s
6 | 25.3s | 28.09s
7 | 27.75s | 30.3s
8 | 29.79s | 34.8s
9 | 32.2s | 38.36s
10 | 34.2s | 44.42s
11 | 39.61s | 40.95s
12 | 39.53s | 48.78s
13 | 42.77s | 51.24s
14 | 47s | 54.42s
15 | 51.97s | 57.7s

And I played with the number of processes in concurrency to download these 15 modules :

Number of concurrency | Time
2 | 85.93s
4 | 71.29s
6 | 71.56s
8 | 61.21s

I'm quite surprised about the results, I was expecting better results after applied the patch.

Has anybody tested this with modules that require the same dependencies? For instance, what happens if module A and module B are being processed concurrently and both require module C--does module C get downloaded twice?

#19: Drush dl does not download dependencies; that is done when the module is enabled.

#18: For systems on fast networks, it seems that the overhead of creating a new process is similar to or greater than the benefits of using multiple processes. There might be some larger benefit if, for example, 16 modules were split up into four processes, each of which downloaded four modules. Doing this, however, would mean that you would need two controls: concurrency level and number of arguments to pass to each subprocess. I'm not sure that the gains would be worth the cost.

If drush dl were used to download really large files, or if the network speeds were really really slow, then this patch might be of greater benefit. As it stands, I'm not sure that we should bother to work on this further unless it seems someone needs it.