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

[Junio C Hamano]

Stefan Beller  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;
}

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Beller]

On Mon, Sep 21, 2015 at 6:08 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> +void default_start_failure(void *data,
>> +struct child_process *cp,
>> +struct strbuf *err)
>> +{
>> + int i;
>> + struct strbuf sb = STRBUF_INIT;
>> +
>> + for (i = 0; cp->argv[i]; i++)
>> + strbuf_addf(, "%s ", cp->argv[i]);
>> + die_errno("Starting a child failed:\n%s", sb.buf);
>
> Do we want that trailing SP after the last element of argv[]?
> Same question applies to the one in "return-value".

done

>
>> +static void run_processes_parallel_init(struct parallel_processes *pp,
>> + int n, void *data,
>> + get_next_task_fn get_next_task,
>> + start_failure_fn start_failure,
>> + return_value_fn return_value)
>> +{
>> + int i;
>> +
>> + if (n < 1)
>> + n = online_cpus();
>> +
>> + pp->max_processes = n;
>> + pp->data = data;
>> + if (!get_next_task)
>> + die("BUG: you need to specify a get_next_task function");
>> + pp->get_next_task = get_next_task;
>> +
>> + pp->start_failure = start_failure ? start_failure : 
>> default_start_failure;
>> + pp->return_value = return_value ? return_value : default_return_value;
>
> I would actually have expected that leaving these to NULL will just
> skip pp->fn calls, instead of a "default implementation", but a pair
> of very simple default implementation would not hrtut.

Ok, I think the default implementation provided is a reasonable default, as
it provides enough information in case of an error.

>
>> +static void run_processes_parallel_cleanup(struct parallel_processes *pp)
>> +{
>> + int i;
>
> Have a blank between the decl block and the first stmt here (and
> elsewhere, too---which you got correct in the function above)?

done

>
>> + for (i = 0; i < pp->max_processes; i++)
>> + strbuf_release(>children[i].err);
>
>> +static void run_processes_parallel_start_one(struct parallel_processes *pp)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < pp->max_processes; i++)
>> + if (!pp->children[i].in_use)
>> + break;
>> + if (i == pp->max_processes)
>> + die("BUG: bookkeeping is hard");
>
> Mental note: the caller is responsible for not calling this when all
> slots are taken.
>
>> + if (!pp->get_next_task(pp->data,
>> +>children[i].process,
>> +>children[i].err)) {
>> + pp->all_tasks_started = 1;
>> + return;
>> + }
>
> Mental note: but it is OK to call this if get_next_task() previously
> said "no more task".
>
> The above two shows a slight discrepancy (nothing earth-breaking).

I see. Maybe this can be improved by having the
run_processes_parallel_start_as_needed call get_next_task
and pass the information into the run_processes_parallel_start_one
or as we had it before, combine these two functions again.

>
> I have this suspicion that the all-tasks-started bit may turn out to
> be a big mistake that we may later regret.  Don't we want to allow
> pp->more_task() to say "no more task to run at this moment" implying
> "but please do ask me later, because I may have found more to do by
> the time you ask me again"?

And this task would arise because the current running children produce
more work to be done?
So you would have a
more_tasks() question. If that returns true
get_next_task() must provide that next task?

In case we had more work to do, which is based on the outcome of the
children, we could just wait in get_next_task for a semaphore/condition
variable from the return_value. Though that would stop progress reporting
end maybe lock up the whole program due to pipe clogging.

It seems to be a better design as we come back to the main loop fast
which does the polling. Although I feel like it is over engineered for now.

So how would you find out when we are done?
* more_tasks() could have different return values in an enum
  (YES_THERE_ARE, NO_BUT_ASK_LATER, NO_NEVER_ASK_AGAIN)
* There could be yet another callback more_tasks_available() and
   parallel_processing_should_stop()
* Hand back a callback ourselfs [Call signal_parallel_processing_done(void*)
  when more_tasks will never return true again, with a void* we provide to
  more_tasks()]
* ...

>
> That is one of the reasons why I do not think the "very top level is
> a bulleted list" organization is a good idea in general.  A good
> scheduling decision can seldom be made in isolation without taking
> global picture into account.
>
>> +static void run_processes_parallel_collect_finished(struct 
>> parallel_processes *pp)
>> +{
>> + int i = 0;
>> + pid_t pid;
>> + int wait_status, code;
>> + int n = pp->max_processes;
>> +
>> +

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

[Stefan Beller]

On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano  wrote:
> Stefan Beller  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();
while (pp.nr_processes > 0) {
run_processes_parallel_buffer_stderr();
run_processes_parallel_output();
run_processes_parallel_collect_finished();
run_processes_parallel_start_as_needed();
}


This eliminates the need for the flag and also exits the loop just
after the possible startup of new processes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Beller]

On Tue, Sep 22, 2015 at 2:41 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano  wrote:
>>> Stefan Beller  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();
>> while (pp.nr_processes > 0) {
>> run_processes_parallel_buffer_stderr();
>> run_processes_parallel_output();
>> run_processes_parallel_collect_finished();
>> run_processes_parallel_start_as_needed();
>> }
>
> 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())
; /* nothing */
if (!pp.nr_processes)
break;
buffer(..);
output_live_child(..);
cull_finished(..);
}

I'll think about that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano  wrote:
>> Stefan Beller  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();
> while (pp.nr_processes > 0) {
> run_processes_parallel_buffer_stderr();
> run_processes_parallel_output();
> run_processes_parallel_collect_finished();
> run_processes_parallel_start_as_needed();
> }

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, and without the
explicit "start only if we can afford to"), then I have to say that
our design tastes are fundamentally incompatible.  I have nothing
more to add.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Sep 22, 2015 at 2:41 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano  wrote:
 Stefan Beller  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();
>>> while (pp.nr_processes > 0) {
>>> run_processes_parallel_buffer_stderr();
>>> run_processes_parallel_output();
>>> run_processes_parallel_collect_finished();
>>> run_processes_parallel_start_as_needed();
>>> }
>>
>> 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.
> ...
> I'll think about that.

Don't waste too much time on it ;-)  This is largely a taste thing,
and taste is very hard to reason about, understand, teach and learn.

Having said that, if I can pick one thing that I foresee to be
problematic the most (aside from these overlong names of the
functions that are private and do not need such long names), it is
that "start as many without giving any control to the caller"
interface.  I wrote "start *a* new one" in the outline above for a
reason.

Even if you want to utilize a moderate number of processes, say 16,
working at the steady state, I suspect that we would find it
suboptimal from perceived latency point of view, if we spin up 16
processes all at once at the very beginning before we start to
collect output from the designated foreground process and relay its
first line of output.  We may want to be able to tweak the caller to
spin up just a few first and let the loop ramp up to the full blast
gradually so that we can give an early feedback to the end user.
That is not something easy to arrange with "start as many without
giving control to the caller" interface.  We probably will discover
other kinds of scheduling issues once we get familiar with this
machinery and would find need for finer grained control.

And I consider such a ramp-up logic shouldn't be hidden inside the
"start_as_needed()" helper function---it is the way how the calling
loop wants its processes started, and I'd prefer to have the logic
visible in the caller's loop.

But that is also largely a "taste" thing.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Stefan Beller  writes:

> +void default_start_failure(void *data,
> +struct child_process *cp,
> +struct strbuf *err)
> +{
> + int i;
> + struct strbuf sb = STRBUF_INIT;
> +
> + for (i = 0; cp->argv[i]; i++)
> + strbuf_addf(, "%s ", cp->argv[i]);
> + die_errno("Starting a child failed:\n%s", sb.buf);

Do we want that trailing SP after the last element of argv[]?
Same question applies to the one in "return-value".

> +static void run_processes_parallel_init(struct parallel_processes *pp,
> + int n, void *data,
> + get_next_task_fn get_next_task,
> + start_failure_fn start_failure,
> + return_value_fn return_value)
> +{
> + int i;
> +
> + if (n < 1)
> + n = online_cpus();
> +
> + pp->max_processes = n;
> + pp->data = data;
> + if (!get_next_task)
> + die("BUG: you need to specify a get_next_task function");
> + pp->get_next_task = get_next_task;
> +
> + pp->start_failure = start_failure ? start_failure : 
> default_start_failure;
> + pp->return_value = return_value ? return_value : default_return_value;

I would actually have expected that leaving these to NULL will just
skip pp->fn calls, instead of a "default implementation", but a pair
of very simple default implementation would not hrtut.

> +static void run_processes_parallel_cleanup(struct parallel_processes *pp)
> +{
> + int i;

Have a blank between the decl block and the first stmt here (and
elsewhere, too---which you got correct in the function above)?

> + for (i = 0; i < pp->max_processes; i++)
> + strbuf_release(>children[i].err);

> +static void run_processes_parallel_start_one(struct parallel_processes *pp)
> +{
> + int i;
> +
> + for (i = 0; i < pp->max_processes; i++)
> + if (!pp->children[i].in_use)
> + break;
> + if (i == pp->max_processes)
> + die("BUG: bookkeeping is hard");

Mental note: the caller is responsible for not calling this when all
slots are taken.

> + if (!pp->get_next_task(pp->data,
> +>children[i].process,
> +>children[i].err)) {
> + pp->all_tasks_started = 1;
> + return;
> + }

Mental note: but it is OK to call this if get_next_task() previously
said "no more task".

The above two shows a slight discrepancy (nothing earth-breaking).

I have this suspicion that the all-tasks-started bit may turn out to
be a big mistake that we may later regret.  Don't we want to allow
pp->more_task() to say "no more task to run at this moment" implying
"but please do ask me later, because I may have found more to do by
the time you ask me again"?

That is one of the reasons why I do not think the "very top level is
a bulleted list" organization is a good idea in general.  A good
scheduling decision can seldom be made in isolation without taking
global picture into account.

> +static void run_processes_parallel_collect_finished(struct 
> parallel_processes *pp)
> +{
> + int i = 0;
> + pid_t pid;
> + int wait_status, code;
> + int n = pp->max_processes;
> +
> + while (pp->nr_processes > 0) {
> + pid = waitpid(-1, _status, WNOHANG);
> + if (pid == 0)
> + return;
> +
> + if (pid < 0)
> + die_errno("wait");
> +
> + for (i = 0; i < pp->max_processes; i++)
> + if (pp->children[i].in_use &&
> + pid == pp->children[i].process.pid)
> + break;
> + if (i == pp->max_processes)
> + /*
> +  * waitpid returned another process id
> +  * which we are not waiting for.
> +  */
> + return;

If we culled a child process that this machinery is not in charge
of, waitpid() in other places that wants to see that child will not
see it.  Perhaps such a situation might even warrant an error() or
BUG()?  Do we want a "NEEDSWORK: Is this a bug?" comment here at
least?

> + if (strbuf_read_once(>children[i].err,
> +  pp->children[i].process.err, 0) < 0 &&
> + errno != EAGAIN)
> + die_errno("strbuf_read_once");

Don't we want to read thru to the end here?  The reason read_once()
did not read thru to the end may not have anything to do with
NONBLOCK (e.g. xread_nonblock() caps len, and it does not loop).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html