Re: [PATCHv4 06/14] run-command: add an asynchronous parallel child processor

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> +static void pp_buffer_stderr(struct parallel_processes *pp)
> +{
> +	int i;
> +
> +	while ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {
> +		if (errno == EINTR)
> +			continue;
> +		pp_cleanup(pp);
> +		die_errno("poll");
> +	}
> +
> +	/* Buffer output from all pipes. */
> +	for (i = 0; i < pp->max_processes; i++) {
> +		if (pp->children[i].in_use &&
> +		    pp->pfd[i].revents & POLLIN)
> +			if (strbuf_read_once(&pp->children[i].err,
> +					     pp->children[i].process.err, 0) < 0)
> +				if (errno != EAGAIN)
> +					die_errno("read");
> +	}
> +}

I think it is a good thing that the caller is passing the whole pp
to this function.  One thing you may want to consider is to adjust
the poll(2) timeout longer when the process slots are full.

There is nothing you can gain by returning early due to timeout
without doing anything from this function when you know you cannot
start a new process (here, I am assuming that your poll(2) would be
unblocked for a disconnect when one of the processes exits, letting
you return and letting the caller call collect_finished(), which in
turn would allow us to make some progress).

On the other hand, during the early ramp-up period, you may want to
use a shorter poll(2) timeout to give the caller a chance to spawn
more processes sooner.  But that falls into performance tuning that
can and should be left to a later follow-up patch after we get the
basic machinery right.