Re: [PATCH 80/83] run-command: make dup_devnull() non static

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 9, 2016, 15:05 UTC
Message-ID: <alpine.DEB.2.20.1605091700060.4092@virtualbox>
In-Reply-To: <CACsJy8DUcLWQ6AwyLE+CF+TxZtqDLS=82peizSf4TrZ3q=qjJg@mail.gmail.com>

Hi Duy,

On Sun, 8 May 2016, Duy Nguyen wrote:

Show 9 quoted lines

> On Sun, May 8, 2016 at 1:33 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > The claim is that this libifies the procedure. But it makes the code
> > really nasty for use as a library: if this is run in a thread (and you
> > know that we are going to have to do this in the near future, for
> > performance reasons), it will completely mess up all the other threads
> > because it messes with the global file descriptors.
> 
> I vote one step at a time, leave multi-thread support for future.

Oh, but I never said that we have to do that now!

All I said was that using this dup() dance instead of truly libifying the functions would slam the door almost shut for future multi-threading. Which is a strong hint in my book that we should *not* do that dup() dance, but fix our code by introducing a silent mode.

> There's a lot more shared state than file descriptors anyway, at least
> there are object db and index access and probably a couple of hidden
> static variables somewhere.

Sure. And do we change those shared states temporarily in our functions? No, we don't. The object db is not made temporarily inaccessible while a certain function runs. The index access is not temporarily disabled while a certain function runs.

And this is what that dup() dance does: it disables *all* output, not only from the current thread.

> And I'm not sure if multi-thread really helps here. Are we really
> CPU-bound? If object inflation causes that (wild guess), can we just
> inflate ahead in some separate process and pass the result back?

Again. I did *not* suggest to introduce multi-threading. I was making a case for *avoiding* that ugly dup() to /dev/null and then dup() it back to the original state. That would just ask for unintended side effects.

Ciao, Dscho