Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor

From: Stefan Beller <sbeller@google.com>
Date: Sep 22, 2015, 21:54 UTC
Message-ID: <CAGZ79kZFoTUugFdOuikOBUbg0DG+TJ3tNTuZaCs7WSaa2r7=Bg@mail.gmail.com>
In-Reply-To: <xmqqh9mm1690.fsf@gitster.mtv.corp.google.com>

On Tue, Sep 22, 2015 at 2:41 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 36 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> So how would you find out when we are done?
>>>
>>>         while (1) {
>>>                 if (we have available slot)
>>>                         ask to start a new one;
>>>                 if (nobody is running anymore)
>>>                         break;
>>>                 collect the output from running ones;
>>>                 drain the output from the foreground one;
>>>                 cull the finished process;
>>>         }
>>>
>>
>> Personally I do not like the break; in the middle of
>> the loop, but that's personal preference, I'd guess.
>> I'll also move the condition for (we have available slot)
>> back inside the called function.
>>
>> So I am thinking about using this in the reroll instead:
>>
>>     run_processes_parallel_start_as_needed(&pp);
>>     while (pp.nr_processes > 0) {
>>         run_processes_parallel_buffer_stderr(&pp);
>>         run_processes_parallel_output(&pp);
>>         run_processes_parallel_collect_finished(&pp);
>>         run_processes_parallel_start_as_needed(&pp);
>>     }
>
> If you truly think the latter is easier to follow its logic (with
> the duplicated call to the same function only to avoid break that
> makes it clear why we are quitting the loop,

I dislike having the call twice, too.

> and without the
> explicit "start only if we can afford to"), then I have to say that
> our design tastes are fundamentally incompatible.

Well the "start only if we can afford to" is not as easy as just

>                 if (we have available slot)
>                         ask to start a new one;

because that's only half the condition. If we don't start a new one as the callback get_next_task returned without a new task. So it becomes

    while (pp.nr_processes > 0) {
        while (pp->nr_processes < pp->max_processes &&
                start_one_process(&pp))
                        ; /* nothing */
        if (!pp.nr_processes)
            break;
        buffer(..);
        output_live_child(..);
        cull_finished(..);
    }

I'll think about that.