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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 23, 2015, 14:59 UTC
Message-ID: <xmqq6131yyef.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqa8sdzl6a.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 31 quoted lines

> 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.
> ...
> 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.

Just to make sure there is no misunderstanding, just like I prefer "start one" over "start as many as possible" in order to give scheduling decision to the calling loop, I would expect that such a follow-up performance tuning would be done by adding another field to pp tht lets the caller of this function, the overall scheduling loop, to tweak the timeout used, without this function making the decision locallly.

Thanks.