Re: [bug #33138] .PARLLELSYNC enhancement with patch

[Frank Heckenbach]

Eli Zaretskii wrote:

> > Date: Tue, 23 Apr 2013 21:41:22 +0200
> > From: Frank Heckenbach <address@hidden>
> > 
> > Sure, it's excluding much more than necessary, but since
> > the critical section is very short, this shouldn't hurt much. (In
> > other words, if make jobs produce such huge output that copying it
> > around takes a significant amount of time, nobody's ever going to
> > read that output anyway, or someone is post-processing it which will
> > take much longer than copying it, anyway.)
> 
> It doesn't matter whether someone reads the output or not, that output
> gets written and takes its time.  We sometimes invoke tools with
> options we later regret, because we don't anticipate the results.

I was just saying we don't need to optimize for such a strange case.
I'm not saying we should fail in such a case, just that it may take
a little longer if locking is too coarse. Producing the output by
the jobs takes some time anyway, and if the additional delay caused
by locking is too much, they just shouldn't use this option. It's an
option after all.

> > Indeed, as David wrote, under Unix, locking stdout/stderr is most
> > straightforward because it's available without further setup.
> > Incidentally, this way of locking is also as fine-grained as
> > possible (considering recursive makes). However, as I wrote, this
> > fine-grained locking is not really necessary, so it's not worth
> > worrying about replicating it on Windows if this causes trouble.
> 
> It is not easy to know what needs to be replicated and what doesn't,
> because the "fine print" of the implementation is not clear, at least
> to me, and therefore it could easily happen that non-replication of
> some seemingly secondary detail completely breaks the design.

I think it was discussed extensively in the original discussion (in
which you also participated) and here. If anything is still unclear,
feel free to ask. But just to repeat: The only thing really
necessary is that two makes with the same stdout/stderr never run
sync_output() simultaneously. If *any* two sub-makes never run
sync_output() simultaneously, that is also fine.

> > > On Tue, Apr 23, 2013 at 10:50 AM, Eli Zaretskii <address@hidden> wrote:
> > > 
> > > > Please tell me that nothing in this feature relies on
> > > > 'fork', with its copying of handles and other data structures.
> > 
> > All it requires is inheriting the redirected stdout/stderr to child
> > processes. This was already possible under Dos (with the exception
> > that since there was no fork, you had to redirect in the parent
> > process, call the child, then undirect in the parent, IIRC).
> 
> Inheritance is not the problem; _finding_ the inherited object or its
> handle is.  With stdout, its name is global, but not so with a handle
> to some mutex I can create or its name (unless we indeed make the name
> fixed and thus system-global, which I don't like, if only because it's
> not how Make works on Unix).

Now we're talking about different things. I was replying to your
question what it requires about fork etc. (answer: just
stdout/stderr redirection like in the shell). You're worried about
how to pass a reference to a mutex around. Paul just replied to
that.

> > They can have their own stdout, in particular with the
> > "--output-sync=make" option. But that's actually the harmless case:
> > Each sub-make runs with its stdout already redirected to a temp file
> > by the main make. In turn, it redirects the stdout of its children
> > to separate temp files, and when they are done, collects the data to
> > its stdout, i.e. the temp file from the main make. When the sub make
> > is finished, the main make collects its output to the original
> > stdout. So unless I'm mistaken, no locking is actually required in
> > this case.
> 
> But we still acquire the semaphore in this case, don't we?  Perhaps we
> shouldn't.

It wouldn't really make things easier, just add another conditional
which needs to be tested etc.

> > > I still don't know how does Make achieve on Unix the interlocking with
> > > its sub-Make's.  Is it because the lock is inherited as part of fork?
> > 
> > The fd is inherited as part of fork. Each make that needs to takes a
> > lock on the fd.
> 
> On the same fd, or, rather, on an fd connected to the same resource.
> That's what I thought (it isn't spelled out anywhere in the comments,
> and the code that implements that is quite scattered).

It's the same fd (namely, stdout or stderr) via inheritance. Of
course, you can say it's not the same fd, but two different fd's
(i.e., stdout of different processes) referring to the same
resource, but that's splitting hairs (and, again, probably
irrelevant since you're going to implement it differently anyway).

> > > (I wish the design and implementation of this feature were less
> > > Posix-centric...)
> > 
> > The implementation my be (though it's only two functions,
> > acquire_semaphore(), release_semaphore()) that can be completely
> > replaced (note, again, the fact that the stdout or stderr fd also
> > serves for locking, is just an implementation detail and not central
> > to the design).
> 
> I beg to differ.  The implementation relies heavily on the fact that
> the semaphore's name or descriptor is globally known to every program
> in the chain.  Writing a replacement for F_SETLKW using LockFileEx
> took me no more than 5 minutes; it is only when I started to try to
> understand how would that function actually do its job on Windows that
> I discovered a crucial detail: that what was being locked was (at
> least normally) a device, not a file, and that every sub-Make must
> lock the same device/file.

The latter is obvious (if everyone locks their private resource,
that's no locking at all). The former is indeed a problem and if, as
you say, LockFileEx doesn't work on Windows 9X, that might be
another one. So I'd say, just forget about LockFileEx and use a
mutex.

> IOW, the top-level design is indeed quite general, but the low-level
> algorithmic details are not, and therefore just replacing these
> functions will not necessarily do the job.

I still don't see the last conclusion. Sure, you might need to add
code to pass a mutex handle/name around, but other than that, just
replace acquire_semaphore/release_semaphore.

> If we really want to make this reasonably portable (and without that,
> I cannot see how Paul's dream about less ifdef's is going to
> materialize), this code needs IMO to be refactored to make the
> algorithm know less about the implementation details.

Where does it actually know too much about implementation details?

The code that sets up sync_handle? Maybe, but all it does it to
check if stdout/stderr are valid and if so, the same. These are the
other two open issues under Windows, but we need them anyway to
determine whether to use output-sync at all and whether to use
combined_output. So I think this code doesn't need to change, apart
from factoring out "STREAM_OK" (maybe rename, since it's about fd's,
not streams) and "is_same_file". Setting sync_handle might then
become unnecessary under Windows, but the rest of the code doesn't
(in particular, setting combined_output and possibly resetting
output_sync = 0).