Re: [PATCH 2/2] fetch: fetch submodules in parallel

From: Stefan Beller <sbeller@google.com>
Date: Sep 14, 2015, 16:46 UTC
Message-ID: <CAGZ79kaBvVWT1OPMxUAU9N2oaC5TT5wwWew5jS0k_o5J10sKfA@mail.gmail.com>
In-Reply-To: <xmqqpp1nxxji.fsf@gitster.mtv.corp.google.com>

On Sat, Sep 12, 2015 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>> +                     if (start_command(&children[i]))
>> +                             die(_("Could not start child process"));
>> +                     flags = fcntl(children[i].err, F_GETFL);
>> +                     fcntl(children[i].err, F_SETFL, flags | O_NONBLOCK);
>
> This function in run-command.c looks as if it is a generic helper to
> be called by anybody, but it seems to only care about the standard
> error and not the standard output stream, which means potential
> users that do not dup them together cannot use it.  Is that a big
> downside, or is it sufficient to document the API to say that
> children must do so?  I offhand do not think the latter is
> unreasonable, but that may be only because I haven't thought things
> through.

Yes it ought to become a generic helper eventually.

I tried implementing a buffering solution for both stdout and stderr, but that doesn't really workout well if you consider interleaved output on the pipes as we cannot accurately replay that later on. To do that we would need to store the timing information of the channels, at least the relative order of it like:

  (stdout, First comes text to stdout),
  (stderr, interrupted by text in stderr)
  (stdout, but stdout doesn't bother, blasting more text)
  (stderr, continues to interrupt)

obtaining the information is inherently racy, as all we can do is polling/reading from both stdout/err as fast as possible but without proper synchronization mechanisms we cannot be sure.

I will add documentation explaining why the async output case will only deal with one channel. I chose stderr as that's already available and needed in this use case.

Show 22 quoted lines

>
>> +                     nr_processes++;
>> +                     slots[i] = 1;
>> +             }
>> +
>> +             /* prepare data for select call */
>> +             FD_ZERO(&fdset);
>> +             maxfd = 0;
>> +             for (i = 0; i < n; i++) {
>> +                     if (!slots[i])
>> +                             continue;
>> +                     FD_SET(children[i].err, &fdset);
>> +                     if (children[i].err > maxfd)
>> +                             maxfd = children[i].err;
>> +             }
>> +             timeout.tv_sec = 0;
>> +             timeout.tv_usec = 500000;
>> +
>> +             i = select(maxfd + 1, &fdset, NULL, NULL, &timeout);
>
> I thought we try to use poll() and on systems with only select we
> allow compat/ to emulate in our code.

I did not know that. I'll rewrite the patch to use poll instead.

Show 16 quoted lines

>
>> +             if (i < 0) {
>> +                     if (errno == EINTR)
>> +                             /* A signal was caught; try again */
>> +                             continue;
>> +                     else if (errno == ENOMEM)
>> +                             die_errno("BUG: keeping track of fds is hard");
>> +                     else if (errno == EINVAL)
>> +                             die_errno("BUG: invalid arguments to select");
>> +                     else if (errno == EBADF)
>> +                             die_errno("BUG: keeping track of fds is hard");
>> +                     else
>> +                             die_errno("Unknown error with select");
>
> I doubt that the later part of elseif cascade adds any value.  You
> will see errno printed anyway.

ok.