[Top][All Lists]

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

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

From: Frank Heckenbach
Subject: Re: [bug #33138] .PARLLELSYNC enhancement with patch
Date: Wed, 24 Apr 2013 18:50:15 +0200

Eli Zaretskii wrote:

> That's one way.  Another one is to discuss the design more thoroughly
> before the patches are accepted.

I think it was discussed quite extensively. Also in retrospect, I
don't see how the design could have been much different (see below).

> I tried to turn the discussion that
> way when the issue was first brought up, but my comments were largely
> ignored (if I compare what was suggested then with what was eventually
> committed), and most of the discussions were about the details of the
> Unix implementation anyway.

I just reread the whole discussion, and as far as I can see, most of
your questions were answered. Already in
David wrote: "Yes, it's conceptually a semaphore. In fact a Windows
port might prefer to use real semaphores."

You wrote:

: Yes, but a few words about how is this semaphore supposed to get job
: done, and in fact what kind of "synchronization" will this bring to
: Make, would be appreciated.  I don't think you described the feature
: too much in your original post.

Just like David, I really don't know what more you want described. I
can repeat what I wrote yesterday: The only thing really necessary
is that two makes [with the same stdout/stderr] never run
sync_output() simultaneously. (Note: We're only talking about makes,
i.e. cooperating processes; we never need to synchronize the
processes run from the recipes, apart from recursive makes.)

That's really all there is to it, the rest is implementation details
(and may be totally different on Windows). Perhaps you're thinking
of it as much more complicated than is really is. It's just about
the simplest possible use case of an inter-process mutual exclusion
(all processes related, so they can pass info around; all
cooperating; just one code block to lock).

: Btw, there will be other side effects, at least on non-Posix
: platforms, due to the fact that stuff that was supposed to go to the
: screen is redirected to a file instead.  Some programs sense that and
: behave differently, e.g. with binary non-printable characters or with
: special character sequences.  By redirecting the output to files, then
: displaying it on the screen, the output may look strangely, or have
: some more grave effect, like terminating output prematurely.

I don't see why it would terminate prematurely, but indeed it may
display differently, also on Unix, e.g. "ls --color=tty" may use
colors whwn writing to a terminal and not to a file. So if you use
output-sync and your recipes contains this command, you lose colors.
I'd say you have several options:

- Live with it.

- Change the command (in the ls example, use "--color=always").

- If the effect is more serious than losing colors (you say it "may
  look strangely"), maybe you shouldn't put such a command in a make
  recipe in the first place (at least not in a Makefile that's going
  to be distributed; note that the same problems will happen if the
  user redirects the whole make output to a file which I often do).

- If the Makefile is only used in-house and you definitely want the
  output-device-dependent behaviour, just don't use output-sync.
  It's just an option.

- Provide your own work-around, perhaps write your output to a pty.

What I'm saying is the circumstances where this is a real problem
seem rather exotic, and noone's forcing you to use the option. If we
were talking about changing make's default behaviour, the concern
may be justified, but we aren't.

: We should consider these potential problems, and in any case the
: simple loop that reads from temporary files and writes to
: stdout/stderr may need to become much more complicated, at least
: on DOS/Windows.  (I believe similar, though less serious, problems
: could happen on Unix as well.)

Apart from setting O_BINARY, I don't see why. Can you explain?

WRT O_BINARY, you wrote:

:  . 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.

I was about to add it, but now I wonder if that's really needed:
If the temp file is opened in binary mode, and this temp file is
passed as stdout/stderr to the shell commands, those will write
their output in binary mode (i.e., without CR, AIUI). Later
pump_from_tmp_fd() will read it back in binary and dump it to the
original stdout/stderr in whatever mode it was (usually non-binary),
so the CRs get added there. Or is there any special handling of
stdout/stderr WRT binary mode during inheritance or so?

I'm attaching a small patch to:

- Factor out is_same_file() into a function.

- Turn file_ok() and fd_not_empty() into functions instead of
  macros. (No real need for macros, and functions may be easier to

- Use SEEK_END rather than SEEK_CUR in fd_not_empty(), so it also
  reports true if the fd is at the beginning of a non-empty file
  (such as if the position is not shared between processes
  inheriting the fd; under POSIX it should be, then it makes no
  difference, otherwise it's one thing less to worry about).

- Fix a wrong comment as I reported yesterday.

- Don't discard the output if acquire_semaphore() fails, as I wrote
  last week.

To sum, what's missing for Windows is:

- implement file_ok() and is_same_file() (both take only stdout and
  stderr as arguments, in case this helps)

- imeplement acquire_semaphore()/release_semaphore(), probably using
  a real mutex instead of file locking, and add code to pass down
  the mutex name or handle to sub-makes.

>From my view, all of this has been established several mails ago, so
I really wonder what's the problem now.
--- job.c.orig  2013-04-24 16:30:05.000000000 +0200
+++ job.c       2013-04-24 18:24:18.000000000 +0200
@@ -244,13 +244,37 @@
 unsigned int jobserver_tokens = 0;
 /* Semaphore for use in -j mode with output_sync. */
 int sync_handle = -1;
-#define STREAM_OK(_s)       ((fcntl (fileno (_s), F_GETFD) != -1) || (errno != 
+/* Test whether a file is valid. */
+static int
+file_ok (FILE *f)
+  return fcntl (fileno (f), F_GETFD) != -1 || errno != EBADF;
+/* Test whether a file contains any data. */
+static int
+fd_not_empty (int fd)
+  return fd >= 0 && lseek (fd, 0, SEEK_END) > 0;
+/* Test whether two files are the same. */
+static int
+is_same_file (FILE *f1, FILE *f2)
+  struct stat stbuf_1, stbuf_2;
+  return fstat (fileno (f1), &stbuf_1) == 0 &&
+         fstat (fileno (f2), &stbuf_2) == 0 &&
+         stbuf_1.st_dev == stbuf_2.st_dev &&
+         stbuf_1.st_ino == stbuf_2.st_ino;
-#define FD_NOT_EMPTY(_f)    ((_f) >= 0 && lseek ((_f), 0, SEEK_CUR) > 0)
 #endif /* OUTPUT_SYNC */
 #ifdef WINDOWS32
@@ -561,13 +585,13 @@
   if (c->outfd != -1 && c->errfd != -1)
     return 1;
-  if (STREAM_OK (stdout))
+  if (file_ok (stdout))
       c->outfd = open_tmpfd ();
       CLOSE_ON_EXEC (c->outfd);
-  if (STREAM_OK (stderr))
+  if (file_ok (stderr))
       if (c->outfd >= 0 && combined)
         c->errfd = c->outfd;
@@ -622,7 +646,7 @@
   fl.l_type = F_WRLCK;
   fl.l_whence = SEEK_SET;
-  fl.l_start = 0; /* lock just one byte according to pid */
+  fl.l_start = 0; /* lock just one byte */
   fl.l_len = 1;
   if (fcntl (sync_handle, F_SETLKW, &fl) != -1)
     return &fl;
@@ -648,13 +672,15 @@
 static void
 sync_output (struct child *c)
-  void *sem;
-  int outfd_not_empty = FD_NOT_EMPTY (c->outfd);
-  int errfd_not_empty = FD_NOT_EMPTY (c->errfd);
+  int outfd_not_empty = fd_not_empty (c->outfd);
+  int errfd_not_empty = fd_not_empty (c->errfd);
-  if ((outfd_not_empty || errfd_not_empty) && (sem = acquire_semaphore ()))
+  if (outfd_not_empty || errfd_not_empty)
+      /* Try to acquire the semaphore. If it fails, dump the output
+         unsynchronized; still better than silently discarding it.  */
+      void *sem = acquire_semaphore ();
       /* We've entered the "critical section" during which a lock is held.
          We want to keep it as short as possible.  */
       if (outfd_not_empty)
@@ -667,7 +693,8 @@
         pump_from_tmp_fd (c->errfd, fileno (stderr));
       /* Exit the critical section.  */
-      release_semaphore (sem);
+      if (sem)
+        release_semaphore (sem);
   if (c->outfd >= 0)
@@ -1567,18 +1594,12 @@
               synchronize on. This block is traversed only once. */
           if (sync_handle == -1)
-              if (STREAM_OK (stdout))
+              if (file_ok (stdout))
-                  struct stat stbuf_o, stbuf_e;
                   sync_handle = fileno (stdout);
-                  combined_output =
-                    fstat (fileno (stdout), &stbuf_o) == 0 &&
-                    fstat (fileno (stderr), &stbuf_e) == 0 &&
-                    stbuf_o.st_dev == stbuf_e.st_dev &&
-                    stbuf_o.st_ino == stbuf_e.st_ino;
+                  combined_output = is_same_file (stdout, stderr);
-              else if (STREAM_OK (stderr))
+              else if (file_ok (stderr))
                 sync_handle = fileno (stderr);

reply via email to

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