From: Junio C Hamano <gitster@pobox•com>
To: Stefan Beller <sbeller@google•com>
Cc: git@vger•kernel.org, ramsay@ramsayjones•plus.com,
jacob.keller@gmail•com, peff@peff•net, jrnieder@gmail•com,
johannes.schindelin@gmail•com, Jens.Lehmann@web•de,
ericsunshine@gmail•com
Subject: Re: [PATCH 6/8] run-command: add an asynchronous parallel child processor
Date: Tue, 29 Sep 2015 20:12:31 -0700 [thread overview]
Message-ID: <xmqqa8s4a9cw.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1443482046-25569-7-git-send-email-sbeller@google.com> (Stefan Beller's message of "Mon, 28 Sep 2015 16:14:04 -0700")
Stefan Beller <sbeller@google•com> writes:
> + while (1) {
> + int i;
> + int output_timeout = 100;
> + int spawn_cap = 4;
> +
> + if (!no_more_task) {
> + for (i = 0; i < spawn_cap; i++) {
> + int code;
> + if (pp->nr_processes == pp->max_processes)
> + break;
> +
> + code = pp_start_one(pp);
> + if (!code)
> + continue;
> + if (code < 0) {
> + pp->shutdown = 1;
> + kill_children(pp, SIGTERM);
> + }
> + no_more_task = 1;
> + break;
> + }
> + }
> + if (no_more_task && !pp->nr_processes)
> + break;
I may have comments on other parts of this patch, but I noticed this
a bit hard to read while reading the end result.
Losing the outer "if (!no_more_task)" and replacing the above with
for (no_more_task = 0, i = 0;
!no_more_task && i < spawn_cap;
i++) {
... do things that may or may not set
... no_more_task
}
if (no_more_task && ...)
break;
would make it clear that regardless of spawn_cap, no_more_task is
honored.
Also I think that having the outer "if (!no_more_task)" and not
having "no_more_task = 0" after each iteration is buggy. Even when
next_task() told start_one() that it does not have more tasks for
now, as long as there are running processes, it is entirely plausible
that next call to next_task() can return "now we have some more task
to do".
Although I think it would make it unsightly, if you want to have the
large indentation that protects the spawn_cap loop from getting
entered, the condition would be
if (!pp->shutdown) {
for (... spawn_cap loop ...) {
...
}
}
That structure could make sense. But even then I would probably
write it more like
...
int spawn_cap = 4;
pp = pp_init(...);
while (1) {
int no_more_task = 0;
for (i = 0;
!no_more_task && !pp->shutdown && i < spawn_cap;
i++) {
...
code = start_one();
... set no_more_task to 1 as needed
... set pp->shutdown to 1 as needed
}
if (no_more_task && !pp->nr_processes)
break;
buffer_stderr(...);
output(...);
collect(...);
}
That is, you need to have two independent conditions that tell you
not to spawn any new task:
(1) You called start_one() repeatedly and next_task() said "nothing
more for now", so you know calling start_one() one more time
without changing other conditions (like draining output from
running processes and culling finished ones) will not help.
Letting other parts of the application that uses this scheduler
loop (i.e. drain output, cull finished process, etc.) may
change the situation and you _do_ need to call start_one() when
the next_task() merely said "nothing more for now".
That is what no_more_task controls.
(2) The application said "I want the system to be gracefully shut
down". next_task() may also have said "nothing more for now"
and you may have set no_more_task in response to it, but unlike
(1) above, draining and culling must be done only to shut the
system down, the application does not want new processes to be
added. You do not want to enter the spawn_cap loop when it
happens.
That is what pp->shutdown controls.
next prev parent reply other threads:[~2015-09-30 3:12 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-28 23:13 [PATCH 0/8] fetch submodules in parallel Stefan Beller
2015-09-28 23:13 ` [PATCH 1/8] submodule.c: write "Fetching submodule <foo>" to stderr Stefan Beller
2015-09-28 23:14 ` [PATCH 2/8] xread: poll on non blocking fds Stefan Beller
2015-09-28 23:14 ` [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking Stefan Beller
2015-09-28 23:14 ` [PATCH 4/8] strbuf: add strbuf_read_once to read " Stefan Beller
2015-09-28 23:14 ` [PATCH 5/8] sigchain: add command to pop all common signals Stefan Beller
2015-09-30 5:23 ` Junio C Hamano
2015-09-28 23:14 ` [PATCH 6/8] run-command: add an asynchronous parallel child processor Stefan Beller
2015-09-30 3:12 ` Junio C Hamano [this message]
2015-09-30 18:28 ` Stefan Beller
2015-09-30 18:48 ` Junio C Hamano
2015-09-28 23:14 ` [PATCH 7/8] fetch_populated_submodules: use new parallel job processing Stefan Beller
2015-09-28 23:14 ` [PATCH 8/8] submodules: allow parallel fetching, add tests and documentation Stefan Beller
-- strict thread matches above, loose matches on Subject: below --
2015-12-14 19:37 [PATCH 0/8] Rerolling sb/submodule-parallel-fetch for the time after 2.7 Stefan Beller
2015-12-14 19:37 ` [PATCH 6/8] run-command: add an asynchronous parallel child processor Stefan Beller
2015-12-14 20:39 ` Johannes Sixt
2015-12-14 21:40 ` Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqa8s4a9cw.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=Jens.Lehmann@web$(echo .)de \
--cc=ericsunshine@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jacob.keller@gmail$(echo .)com \
--cc=johannes.schindelin@gmail$(echo .)com \
--cc=jrnieder@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=ramsay@ramsayjones$(echo .)plus.com \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox