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

Stefan Beller <sbeller@google.com> writes:

I think we are almost there, but there were still a few more "huh?" in this patch.

This claim is dubious.

The last four lines of this function marks the i-th slot as in_use, and increments nr_processes, so that exit codepath clearly is about "a process was started" and should be returning 1, but the code returns 0, which looks incorrect.

And this one, when get_next_task() says "nothing more to do", is clearly "we returned without starting anything", so according to the comment it should be returning 0, but the code returns 1, which looks incorrect.

What should happen if start_failure returns without dying? Shouldn't this function return something, without doing the remainder of it? i.e.

	if (start_command(...)) {
		pp->start_failur(...);
                return SOMETHING;
	}

According to the comment at the beginning, that SOMETHING ought to be 0, because we did not spawn anything, i.e. we did not increment nr_processes.

But don't make that change yet; I do not think it is a great interface to say "did we or did we not add a new process?" anyway (see below).

And this is "we spawned" that ought to have returned 1.

Perhaps the comment at the beginning is wrong, as the code consistently does the opposite of what the comment says.

But it does not really matter, as I do not think this should be returning "did we or did we not start a new process?" (see below).

This inner loop is why I think "did we or did we not spawn a new process?" is not a great interface.

The reason why it is not a great interface is because there are two possible reasons why pp_start_one() does not spawn a new process, and this caller wants to behave differently depending on why it did not spawn a new process. They are:

 * get_next_task() truly ran out of things to do.

 * get_next_task() gave us a task, but it did not start, and
   start_failure was set not to die (e.g. the function can be used
   to tell the next_task machinery that it needs to return a
   replacement task for the one that failed to run.  That way, upon
   next call to get_next_task, a replacement task can run instead of
   the old one that failed).

For the former, we want to stop looping, for the latter, we definitely do want to keep looping, as we want to make another call to get_next_task() to grab the replacement task for the one that just failed.

So I think it makes more sense to define the meaning of the return value from pp_start_one() differently from the way this patch defines. "Return 0 when we truly ran out of things to do, otherwise return non-zero", for example, would make more sense. The return value does not tell you if the call resulted in one more process, but that is not any loss, as you can look at pp.nr_processes yourself if you really cared.

With that, the above caller could be updated, with optional gradual ramp_up, like so:

	#define RAMP_UP_LIMIT 2

	while (1) {
		int ramp_up;
		int no_more_task;

		for (no_more_task = 0, ramp_up = RAMP_UP_LIMIT;
                     !no_more_task && ramp_up && pp.nr_processes < pp.max_processes;
		     ramp_up--)
			if (!pp_start_one(&pp))
				no_more_task = 1;

		if (!pp.nr_processes && no_more_task)
			break;

If you prefer to swamp the system with a thundering herd at the beginning, you can define RAMP_UP_LIMIT to really a high value instead, e.g. "#define RAMPUP_LIMIT pp.max_processes". I however would not recommend it because doing so would hurt the perceived latency at the beginning.

After the system goes into a steady state, how you set RAMP_UP_LIMIT would not make that much difference, as your slots should be almost always full and you will be replenishing an open slot with a single task as each running task finishes, and you would not be running more than one pp_start_one() at a time anyway.