Re: [PATCHv5] Another squash on run-command: add an asynchronous parallel child processor

Stefan Beller <sbeller@google.com> writes:

I dunno. As I said, while a small part of that commit is necessary to be squashed into [6/14] (i.e. the "failed to start" bugfix and redefinition of the return value of start_one()), the remainder of the commit is primarily to illustrate possible future enhancements that the basic structure of your code must support, so that we can get the fundamentals right. Each of the actual "possible future enhancements" may or may not be a good idea and there is nothing that back it up. In such a vacuum, I'd prefer to leave them simple and avoid starting performance tuning prematurely.

One thing that is sort-of backed up already by your "cap to 4" experiment is that some sort of slow-start ramping up is far better than letting thundering herd stampede, so I am OK if we kept that SPAWN_CAP part of the commit.

But even then, we do not know if tying that cap to online_cpu() is a good idea. Neither of us have a good argument backed by data on it.

It is tempting to imagine, when you have N cores on an otherwise idle box, the setting of SPAWN_CAP shouldn't make much difference to how well the first child process makes its initial progress as long as it does not exceed the number of idle cores N-1 you have at hand.

But that assumes that the task is CPU bound and you have infinite memory bandwidth. Once the task needs a lot of disk bandwidth to make its initial progress, which certainly is the case for fetch, the first child that is spawned together with (SPAWN_CAP-1) other processes would be competing for the shared resource, and having more online_cpus() would not help you.

If we are not doing analysis that takes into such factors (and it is way too premature for us to be tuning), even "online_cpu() - 1" is unnecessarily too complex than a hardcoded small number (say, "2", or even "1").

The same thing can be said for the output_timeout selection. "Do not get stuck for too long until we have fully ramped up. Do not spin too frequently when there is no more room for a new child" was something I came up out of thin air as an example of something we might want to do, and I did write such a code in that commit, but that was primarily done so that you can clearly see that a better design would be to allow the caller, i.e. the scheduling loop, specify output_timeout to buffer_stderr(), and to keep the latter a "dumb" helper that can be controlled by a more smart caller (as opposed to hiding such a logic in buffer_stderr() and have a "dumb" driver call it). The actual output_timeout computation logic is not well thought out---it may even turn out to be that we are better off if we lengthened the timeout before we have fully ramped up, to encourage the first process to produce some output before we give chance to other new processes to be spawned in the later round.

So for that change, while I think adding that parameter to buffer_stderr() is something we would want to keep, I'd prefer to keep the caller simpler by always passing a hardcoded 100 in the initial version, before we start tuning. And I do not think we want to start tuning before building a solid foundation to tune.

In short, if I were amending that SQUASH??? commit, I'd probably be making it do less, not more, than what it does, something along the line of the attached.

 run-command.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)