git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Stefan Beller <sbeller@google.com>
Date
Sep 22, 2015, 18:28 UTC
Message-ID
<CAGZ79kbUkUSAP+muhYxTwHZdD+ojJYXjogZfRXs0PemEdcqfbA@mail.gmail.com>
In-Reply-To
<xmqqfv276z1q.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 21, 2015 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Stefan Beller <sbeller@google.com> 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(&sb, "%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
Show 24 quoted lines
>
>> +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.

Show 7 quoted lines
>
>> +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
Show 28 quoted lines
>
>> +     for (i = 0; i < pp->max_processes; i++)
>> +             strbuf_release(&pp->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,
>> +                            &pp->children[i].process,
>> +                            &pp->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.

Show 6 quoted lines
>
> 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()]
* ...
Show 46 quoted lines
>
> 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, &wait_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(&pp->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).
right.
>
Previous: Junio C HamanoNext: Junio C Hamano
Message 33 of 48 in “[PATCHv3 00/13] fetch submodules in parallel and a preview on parallel "submodule update"”
  1. Stefan BellerSep 21, 2015
  2. 01/13 Sending "Fetching submodule <foo>" output to stderrStefan Beller, Sep 21, 2015
  3. Junio C HamanoSep 21, 2015
  4. 02/13 xread: poll on non blocking fdsStefan Beller, Sep 21, 2015
  5. Junio C HamanoSep 21, 2015
  6. Torsten BögershausenSep 22, 2015
  7. Jacob KellerSep 22, 2015
  8. Torsten BögershausenSep 22, 2015
  9. Junio C HamanoSep 22, 2015
  10. Jeff KingSep 22, 2015
  11. Junio C HamanoSep 22, 2015
  12. Stefan BellerSep 23, 2015
  13. Junio C HamanoSep 23, 2015
  14. Jeff KingSep 23, 2015
  15. Eric SunshineSep 21, 2015
  16. Junio C HamanoSep 22, 2015
  17. Stefan BellerSep 22, 2015
  18. Junio C HamanoSep 22, 2015
  19. Stefan BellerSep 22, 2015
  20. 03/13 xread_nonblock: add functionality to read from fds nonblockinglyStefan Beller, Sep 21, 2015
  21. Junio C HamanoSep 22, 2015
  22. Junio C HamanoSep 22, 2015
  23. Jacob KellerSep 22, 2015
  24. Jacob KellerSep 22, 2015
  25. Junio C HamanoSep 22, 2015
  26. 04/13 strbuf: add strbuf_read_once to read without blockingStefan Beller, Sep 21, 2015
  27. Junio C HamanoSep 22, 2015
  28. Jacob KellerSep 22, 2015
  29. 05/13 run-command: factor out return value computationStefan Beller, Sep 21, 2015
  30. Junio C HamanoSep 22, 2015
  31. 06/13 run-command: add an asynchronous parallel child processorStefan Beller, Sep 21, 2015
  32. Junio C HamanoSep 22, 2015
  33. Stefan BellerSep 22, 2015
  34. Junio C HamanoSep 22, 2015
  35. Stefan BellerSep 22, 2015
  36. Junio C HamanoSep 22, 2015
  37. Stefan BellerSep 22, 2015
  38. Junio C HamanoSep 22, 2015
  39. 07/13 fetch_populated_submodules: use new parallel job processingStefan Beller, Sep 21, 2015
  40. Junio C HamanoSep 22, 2015
  41. 08/13 submodules: allow parallel fetching, add tests and documentationStefan Beller, Sep 21, 2015
  42. 09/13 submodule config: keep update strategy aroundStefan Beller, Sep 21, 2015
  43. Eric SunshineSep 22, 2015
  44. Stefan BellerSep 22, 2015
  45. 10/13 git submodule update: cmd_update_recursiveStefan Beller, Sep 21, 2015
  46. 11/13 git submodule update: cmd_update_cloneStefan Beller, Sep 21, 2015
  47. 12/13 git submodule update: cmd_update_fetchStefan Beller, Sep 21, 2015
  48. 13/13 Rewrite submodule update in CStefan Beller, Sep 21, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.