From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 30 Nov 2022 10:26:10 GMT
Subject: Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts
Message-ID: <778b25b4-8eb2-6d02-3ba5-a21724ff2f2f@dunelm.org.uk>
In-Reply-To: <221130.86lensiwy0.gmgdl@evledraar.gmail.com>

On 30/11/2022 09:53, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Nov 29 2022, Glen Choo wrote:
> 
>> Calvin Wan <calvinwan@google.com> writes:
>>
>>> @@ -1680,8 +1683,14 @@ static void pp_buffer_stderr(struct parallel_processes *pp,
>>>   	for (size_t i = 0; i < opts->processes; i++) {
>>>   		if (pp->children[i].state == GIT_CP_WORKING &&
>>>   		    pp->pfd[i].revents & (POLLIN | POLLHUP)) {
>>> -			int n = strbuf_read_once(&pp->children[i].err,
>>> -						 pp->children[i].process.err, 0);
>>> +			struct strbuf buf = STRBUF_INIT;
>>> +			int n = strbuf_read_once(&buf, pp->children[i].process.err, 0);
>>> +			strbuf_addbuf(&pp->children[i].err, &buf);
>>> +			if (opts->duplicate_output)

Shouldn't we be checking if n < 0 before we do this?

>>> +				opts->duplicate_output(&buf, &pp->children[i].err,
>>> +					  opts->data,
>>> +					  pp->children[i].data);
>>> +			strbuf_release(&buf);
>>>   			if (n == 0) {
>>>   				close(pp->children[i].process.err);
>>>   				pp->children[i].state = GIT_CP_WAIT_CLEANUP;
>>
>> A common pattern is that optional behavior does not impose additional
>> costs on the non-optional part. Here, we unconditionally do a
>> strbuf_addbuf() even though we don't use the result in the "else" case.
>>
>> So this might be more idiomatically written as:
>>
>>          int n = strbuf_read_once(&pp->children[i].err,
>>          			 pp->children[i].process.err, 0);
>>   +      if (opts->duplicate_output) {
>>   +          struct strbuf buf = STRBUF_INIT;
>>   +          strbuf_addbuf(&buf, &pp->children[i].err);
>>   +        	opts->duplicate_output(&buf, &pp->children[i].err,
>>   +        		  opts->data,
>>   +        		  pp->children[i].data);
>>   +          strbuf_release(&buf);
>>   +      }
>>
> [...]
> And why does "&buf" exist at all? 

I was wondering that as too

>Why can't we just pass
> &pp->children[i].err, and if this callback cares about the last thing we
> read let's pass it an offset, so it can know what came from the
> strbuf_read_once() (I don't know if it actually cares about that
> either...).

Or we could just pass a `const char*`, `size_t` pair.

> That would avoid the copy entirely.

Is the copying quadratic at the moment? - it looks like each call to 
strbuf_read_once() appends to the buffer and we copy the whole buffer 
each time.

Best Wishes

Phillip