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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2015, 19:11 UTC
Message-ID: <xmqqpp1nxxji.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1442012994-20374-3-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 46 quoted lines

> diff --git a/run-command.c b/run-command.c
> index 28e1d55..b8ff67b 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -852,3 +852,147 @@ int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)
>  	close(cmd->out);
>  	return finish_command(cmd);
>  }
> +
> +int run_processes_async(int n, get_next_task fn, void *data)
> +{
> +	int i, wait_status;
> +	pid_t pid;
> +
> +	/* no more tasks. Also set when aborting early. */
> +	int all_tasks_started = 0;
> +	int nr_processes = 0;
> +	int child_in_foreground = 0;
> +	struct timeval timeout;
> +	struct child_process *children = xcalloc(n, sizeof(*children));
> +	char *slots = xcalloc(n, sizeof(*slots));
> +	struct strbuf *err = xcalloc(n, sizeof(*err));
> +	fd_set fdset;
> +	int maxfd;
> +	struct strbuf finished_children = STRBUF_INIT;
> +	int flags;
> +	for (i = 0; i < n; i++)
> +		strbuf_init(&err[i], 0);
> +
> +	while (!all_tasks_started || nr_processes > 0) {
> +		/* Start new processes. */
> +		while (!all_tasks_started && nr_processes < n) {
> +			for (i = 0; i < n; i++)
> +				if (!slots[i])
> +					break; /* found an empty slot */
> +			if (i == n)
> +				die("BUG: bookkeeping is hard");
> +
> +			if (fn(data, &children[i], &err[i])) {
> +				all_tasks_started = 1;
> +				break;
> +			}
> +			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.

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

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