[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Paul Smith
Subject: Re: [bug #33138] .PARLLELSYNC enhancement with patch
Date: Tue, 16 Apr 2013 07:47:08 -0400

On Tue, 2013-04-16 at 11:30 +0300, Eli Zaretskii wrote:
> > Date: Tue, 16 Apr 2013 05:54:13 +0000
> > From: "Paul D. Smith" <address@hidden>
> > 
> > I did a little bit of code rearrangement, but I still think this code will 
> > not
> > work on Windows and might possibly not compile on Windows.
> Indeed, it will not.  Some cursory comments below.

I was hoping that if OUTPUT_SYNC is not #defined, the Windows code would
compile OK (although obviously without this feature), until we get it

> > Hopefully we can fix that.
> We shall see...
> Here's what I see in the changes that is not friendly to Windows:
>  . STREAM_OK uses fcntl and F_GETFD which both don't exist on Windows.

This is to test if somehow make's stdout or stderr has been closed (not
just redirected to /dev/null, but closed).

>  . FD_NOT_EMPTY will only work if its argument descriptor is connected
>    to a file, not the console.  is this condition always true?

Yes, because here we're testing the temporary file that we're saving
output in.  Either the fd will be -1 (not redirected to a temp file), or
it will be a temp file.

>  . open_tmpfd will need close examination on Windows, especially since
>    it closes the stream (the issue is whether the file will still be
>    automatically deleted when the dup'ed file descriptor is closed).

Yes, I suspected this would not work well.  On UNIX the file is actually
deleted FIRST, by tmpfile(), because on UNIX a file is not actually
deleted until the last file descriptor using it is closed, even if it's
not visible in the filesystem anymore.  This is a very nice way to get a
truly anonymous temporary file that cannot be accessed by anything other
than the process that created it.

I'm not sure what the semantics of tmpfile() are on Windows.

>  . pump_from_tmp_fd will need to switch to_fd to binary mode, since
>    this is how tmpfile opens the temporary file; we need output mode
>    match the input mode.

That's a good point.

>  . acquire_semaphore uses fcntl and F_SETLKW, which don't exist on
>    Windows.  the commentary to that function is not revealing its
>    purpose, so I'm unsure how to implement its equivalent on Windows.
>    can someone explain why is this needed and what should it do?  the
>    name seems to imply that using fcntl is an implementation detail,
>    as a crude semaphore -- is that right?  similarly for
>    release_semaphore.  (see also the next item.)

Yes, this is the guts of the feature.  It ensures that only one make
process is writing output at a time.  On other systems like Windows a
different method might be more appropriate.  Since the resource we're
locking on is the output, on UNIX we lock the output fd.  This saves us
from having to create a separate lock file, etc.

I'm pretty convinced that it works fine, even if stdout/stderr are
redirected.  For example, a recursive make which is redirected to a
different file will work OK; the locking for the sub-make will happen on
that file which is different than the locking for other make instances,
but that's OK because they're writing to different places anyway.  The
sub-make's output will be internally consistent, and not interfere with
the parent make's output which is what you want.

Windows has LockFileEx() but we'd need to examine the semantics to
verify it will do what we want.

>  . is there any significance to the fact that sync_handle is either
>    stdout or stderr?  is that important for the synchronization to
>    work, or was that just a convenient handle around?  also, the
>    documentation I have indicates that locking works only on files;
>    is it guaranteed that these handles are redirected to files before
>    acquire_semaphore is called?

They are definitely NOT guaranteed to be redirected to files.  The lock
is taken on stdout (if open) before any redirection happens, so normally
it would be taken on stdout going to the console.  On Linux it works
fine.  I'll need to read the standard more closely and maybe do some
testing on other systems.  It's just a convenient handle... but if it
doesn't always work and we have to create our own handle then that's
some extra work as we have to communicate the handle info to the child
make processes.

>  . calculation of combined_output in start_job_command will need to be
>    reimplemented for Windows, since the reliance on st_dev and st_ino
>    makes assumptions that are false on Windows.

Yes, not surprising.

> Other notes:
>  . outputting stdout and stderr separately is IMO a misfeature: it
>    breaks the temporal proximity of messages written to these streams,
>    and this makes it harder to understand failures.

I agree 100%; that's what the combined output test above is supposed to
handle.  If stdout and stderr are going to the same place then we
redirect them to the same temporary file so they will ultimately appear
in the same order as they would have on the terminal.  Only if they are
not going to the same place anyway do we keep them separate.  This seems
to always be the behavior you want.

>  . why is "leaving directory FOO" displayed _before_ releasing the
>    stderr output? it should be after, I think.

The enter/leave output needs investigation for sure.  I think we're
doing too much here.  I would have left it alone but I think some tools
that parse make results expect to see that output to help them figure
out what's going on.

>  . AFAIU, the call to fcntl inside sync_output can be interrupted by a
>    signal, and will return -1.  shouldn't the code do something
>    non-trivial to avoid a total failure in this case?

Hm, well, this is the unlock which you would not expect to be waiting.
But you're right it doesn't hurt to use EINTRLOOP here just to be safe.

>  . suggest to use "acquire_semaphore" and "release_semaphore" in the
>    call to perror in those two functions, as "fcntl" is not specific
>    enough, and also reveals implementation details.

Good point.

>  . likewise, "read()" and "write()" in diagnostics from
>    pump_from_tmp_fd are less specific than they could be; how about
>    "read from temporary file" instead?


>  . btw, why an error in acquire_semaphore and release_semaphore causes
>    Make to bail out?  isn't it better to fall back on the normal
>    operation mode instead?

I'll look at this.  I guess the problem is that if some instances of
make can take the semaphore and some can't, for some reason, should we
continue anyway?

>  . I don't understand why PARLLEL_SYNC is turned on unconditionally in
>    job.h based on POSIX being defined.  it should IMO be in config.h
>    instead, set at configure time.

Yes I agree: that needs to be worked out.  Some configure.ac magic needs
to happen here.  It's not clear how hard it will be to test for all the
things this feature requires.  Some are straightforward but not all.

>  . the new argument to log_working_directory is not documented in the
>    commentary to the function.

All good points.

Thanks for the review; I'll look into this.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]