bugmake - Bugs: bug #33138, .PARLLELSYNC enhancement with patch

 
 

bug #33138: .PARLLELSYNC enhancement with patch

Submitter:  David Boyce <boyski>
Submitted:  Thu 21 Apr 2011 04:17:07 PM UTC
   
 
Severity:  3 - Normal Item Group:  Enhancement
Status:  Fixed Privacy:  Public
Assigned to:  psmith Open/Closed:  Closed
Component Version:  3.82 Operating System:  POSIX-Based
Fixed Release:  4.0 Triage Status:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 16 Apr 2013 05:54:13 AM UTC, comment #12: 

I've applied the patch from Frank.  However I also reworked it in various ways: I renamed the option to -O / --output-sync.  I changed the option argument from a digit (1, 2) to a string (target, make) to denote per-target output vs. per-make output.  I changed the behavior so that the entire recipe is printed together, rather than each individual job being printed together, so that even without .ONESHELL you still have the whole recipe as a single output.  I can't justify to myself the need for individual job output but if someone really wants it please provide a reason.

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.  Hopefully we can fix that.

I reworked the unit test somewhat to try to make it faster.  It's still too slow, because we have to add sleeps in order to get the directory enter/exit text to come out right.  But it's better.

There is an open issue with the enter/exit messages.  I'm not sure I'm happy with how that's working right now.  I need to look into it further.

Paul D. Smith <psmith>
Group administrator
Mon 08 Apr 2013 03:11:58 AM UTC, comment #11: 

By a strange coincidence I just found a bug today:

pump_from_tmp_fd() would only copy one buffer (8k) full of data, because the check "nleft <= 0" was after the loop which decremented nleft to 0 while writing. Moving it before the loop fixes the bug (patch attached).

(file #27808)

Frank Heckenbach <frank>
Sat 06 Apr 2013 04:03:55 PM UTC, comment #10: 

Thanks. If you have any questions, let me know.

I'm attaching a version of the patch which is unchanged except for adding the usage line for --parallel-sync.

(file #27793)

Frank Heckenbach <frank>
Sat 06 Apr 2013 03:16:21 PM UTC, comment #9: 

I'm looking at adding this feature this weekend, FYI.

Paul D. Smith <psmith>
Group administrator
Fri 08 Feb 2013 03:48:58 PM UTC, comment #8: 

Any update whether any of these patches will get into 3.83 or when 3.83 is expected for that matter?

Anonymous
Sun 06 Jan 2013 04:36:06 AM UTC, comment #7: 


> I agree this seems useful. However, since it would involve changes
> to existing code which are not strictly needed...


Not functional changes, just  a little refactoring, and adding a new reliance on tmpfile() may cause more stress to ports than enhancing the existing use of mkstemp(). But I agree, this patch has reached the point where it needs an up-or-down vote from Paul et al before any such final finish work is done.

David

David Boyce <boyski>
Sun 06 Jan 2013 03:02:18 AM UTC, comment #6: 

PS: Ignore the ugly line breaks in the mail. I posted the comment via Savannah (where the lines appear correctly). Apparently it messed them up when forwarding it via email. :-(

Frank Heckenbach <frank>
Sun 06 Jan 2013 02:57:55 AM UTC, comment #5: 

David Boyce wrote:

> This is the original author. I've become very busy at my day job in the last
> year or two so I've lost track of this. Thanks for picking it up and improving
> it. I haven't had time to look at your new patch yet, and not sure when I
> will, but here are a few responses to your comments:


Thanks for your reply.

> > Target vs flag
>
> I have to agree that a flag is more "correct". It's more convenient to use
> .PARALLELSYNC because getting users to use a flag can be difficult (in fact
> the major insight of Feldman's original make was that a naive user should be
> able to just type "make" and the right thing will happen),


If the "right thing" depends on the setting (like with
".SECONDEXPANSION" or various other flags), I definitely agree with
this reasoning.

However, in this case, it's not a question whether the build works
correctly or not, just how the output is presented to the user.
Therefore, different users may legitimately want different settings,
and this is easier to do with an argument than by modifying the
Makefile. Furthermore, this particular feature is interesting only
with parallel builds which require an option ("-j") anyway. And, as
I mentioned, passing it to recursive makes is also easier this way.

> but a flag is more
> flexible. To the best of my memory (not so good) the reasons I went with a
> special target are:
>
> 1. Given the clever --eval flag Paul added in 3.82 any special target can be
> used as a flag, e.g. "--eval .PARALLELSYNC:" (BTW I tried to argue that --eval
> was worthy of a scarce single-letter alias (-E) on the grounds that it was a
> flag-to-end-all-flags but I don't think that happened).


I wasn't aware of this feature. So it should also work this way. but
if we want to support both ways I implemented (fine and coarse) we'd
need two special targets. So I still prefer the option.

> 2. Paul has some concerns about make "becoming like GNU tar" which apparently
> has too many options. Personally I'm always willing to trade a few more
> minutes of RTFM for more capability, and have no problem with GNU tar, but
> tastes differ.


Same for me. (So far make is quite short of the 52 available
letters. :)

What I could imagine is actually to make it the default when using
"-j" on the assumption that most users would prefer a properly
grouped output, and turn it off with an option. But it may be too
intrusive a change for a new feature, and I don't have a strong
opinion on that. (For me, it's just a one-time setup in my script
that invokes make with the right "-j" option depending on the system
configuration anyway.)

> > tmpfile vs mkstemp
>
> IIRC the reason I used tmpfile() instead of mkstemp() is that mkstemp doesn't
> unlink the file automatically, which leads to a number of risk factors such as
> filling up /tmp and so on.
>
> Possibly the best/most portable approach would be to refactor the existing
> open_tmpfile() function, which returns a FILE *, into an open_tmpfd() which
> returns a descriptor wrapped by open_tmpfile() which just converts the
> descriptor into a FILE * using fdopen(). This is already what happens, it's
> just a matter of splitting existing logic into two functions. Now open_tmpfd()
> is logically just mkstemp-with-porting-hacks and we could enhance it with an
> "unlink" option for this feature.


I agree this seems useful. However, since it would involve changes
to existing code which are not strictly needed for this new feature
(it works as it is), I'd rather wait for the go-ahead from one of
the maintainers before making such a change.

> > make[1]: Leaving directory 'bar'
> > make[1]: Leaving directory 'bar'
>
> The problem with doubling these is that it becomes non-deterministic. What if
> there was a directory structure foo/bar/bar/baz?


Actually the messages contain the absolute directory name (retrieved
by "getcwd (current_directory, GET_PATH_MAX)"); I just cut them in
my mail to avoid clutter and not expose my directories. So I don't
think there's actually a problem.

But I noticed another problem: Messages output by make itself (such
as "Makefile:42: recipe for target 'foo' failed") are not always
properly enclosed with enter/leave messages. If they mention file
names, as here "Makefile", this leads to the same issue.

Therefore I modified message(), error() and fatal() to enclose each
message with enter/leave messages, but only if parallel_sync is
active. (Otherwise, for simple uses it would clutter the output too
much.) The special case of message() with fmt==0 is left unchanged,
so the basic enter/leave messages still appear, even if no other
message are produced (the normal case with "-s" and clean builds),
as a kind of progress indicator.

Of course, when parallel_sync is used, this now results in even more
enter/leave messages, but if we want messages properly associated
with directories and don't want to mess with the messages
themselves, I see no alternative. (Again, this mode would usually be
used with machine-parsers like the Emacs directory tracker where the
number of messages won't matter so much as their reliability.)

> Additionally, would it be possible to implement the enhancement proposed in
> the email discussion? See
> http://lists.gnu.org/archive/html/bug-make/2011-04/msg00041.html for context?
> Or was that already done? I don't remember.


You mean merging stdout and stderr if and only if they were the same
originally? That was implemented already (before my changes).

> Last, Paul has been pretty clear lately that new patches should be accompanied
> by associated regression tests. That might be the remaining hurdle.


OK, I added some tests, basically my test case as posted, with
coarse and fine sync, and a test to check the above issue WRT
make-generated messages. (features/parallel-sync)

I also added spaces before parentheses according to the coding
standards and made sure it builds without warnings if PARALLEL_SYNC
is not used.

Frank


(file #27206)

Frank Heckenbach <frank>
Sat 05 Jan 2013 10:24:04 PM UTC, comment #4: 

This is the original author. I've become very busy at my day job in the last year or two so I've lost track of this. Thanks for picking it up and improving it. I haven't had time to look at your new patch yet, and not sure when I will, but here are a few responses to your comments:

> Target vs flag


I have to agree that a flag is more "correct". It's more convenient to use .PARALLELSYNC because getting users to use a flag can be difficult (in fact the major insight of Feldman's original make was that a naive user should be able to just type "make" and the right thing will happen), but a flag is more flexible. To the best of my memory (not so good) the reasons I went with a special target are:

1. Given the clever --eval flag Paul added in 3.82 any special target can be used as a flag, e.g. "--eval .PARALLELSYNC:" (BTW I tried to argue that --eval was worthy of a scarce single-letter alias (-E) on the grounds that it was a flag-to-end-all-flags but I don't think that happened).

2. Paul has some concerns about make "becoming like GNU tar" which apparently has too many options. Personally I'm always willing to trade a few more minutes of RTFM for more capability, and have no problem with GNU tar, but tastes differ.

Bottom line, I'm more or less agnostic on flag-vs-target.

> tmpfile vs mkstemp


IIRC the reason I used tmpfile() instead of mkstemp() is that mkstemp doesn't unlink the file automatically, which leads to a number of risk factors such as filling up /tmp and so on.

Possibly the best/most portable approach would be to refactor the existing open_tmpfile() function, which returns a FILE , into an open_tmpfd() which returns a descriptor wrapped by open_tmpfile() which just converts the descriptor into a FILE using fdopen(). This is already what happens, it's just a matter of splitting existing logic into two functions. Now open_tmpfd() is logically just mkstemp-with-porting-hacks and we could enhance it with an "unlink" option for this feature.

> make[1]: Leaving directory 'bar'
> make[1]: Leaving directory 'bar'


The problem with doubling these is that it becomes non-deterministic. What if there was a directory structure foo/bar/bar/baz?

Additionally, would it be possible to implement the enhancement proposed in the email discussion? See http://lists.gnu.org/archive/html/bug-make/2011-04/msg00041.html for context? Or was that already done? I don't remember.

Last, Paul has been pretty clear lately that new patches should be accompanied by associated regression tests. That might be the remaining hurdle.

David Boyce <boyski>
Sat 05 Jan 2013 05:21:12 AM UTC, comment #3: 

The current patch doesn't work well with recursive makes. The test
case (sync-recursive-demo.tar.gz) demonstrates this. Its output with
"make -j" is the following:

make -C foo
make -C bar
[2s delay]
make[1]: Entering directory 'foo'
foo: start
foo: end
make[1]: Leaving directory 'foo'
[2s delay]
make[1]: Entering directory 'bar'
bar: start
baz: start
bar: end
baz: end
make[1]: Leaving directory 'bar'

This shows two potential problems:

a) The output of bar and baz is still mixed up. This is clearly a
   bug, as far as the purpose of the patch is concerned.

b) The output of bar is not shown until baz is also finished which
   is just an inconvenience, but may be a significant one if the
   recursive jobs take long (think of several directories with large
   builds coordinated by a small top-level Makefile). While some
   amount of output delay is inevitable when syncing, as discussed
   in the original thread, in this case it might be preferable to
   sync the output of the individual recipes of the recursive makes.

The problems occur because the patch doesn't handle recursive jobs
specially, so they will be synced like any other command, as a
whole. Fortunately, I think the issues are rather easy to solve:

1. Do not activate parallel_sync for the recursive jobs themselves.
   With an external solution using SHELL, it might be tricky to
   detect recursive jobs, but with this internal solution, the
   information is readily available by checking
   "!(flags & COMMANDS_RECURSE)" before the assign_child_tempfiles()
   call.

   If the call is not made, we must also set "outfd = errfd = -1" in
   the child. This doesn't hurt anyway and I'd prefer to do it in
   new_job(), right after the allocation of a new child for clarity
   (as is it now, these fields are left unintialized in this case,
   so their validity depends on a global flag which can be confusing
   and error-prone).

   The other places where parallel_sync is checked do not need any
   changes because they do nothing if outfd/errfd are < 0.

2. Pass the parallel sync option to the sub makes. If it is turned
   into a command-line option, as I suggested in my previous
   comment, that can happen automatically via MAKEFLAGS. This
   involves removing the change in read.c and inserting in an entry
   in struct command_switch switches[].

Since someone might prefer the current behaviour of b), I actually
made parallel_sync a tri-state (none, PARALLEL_SYNC_FINE,
PARALLEL_SYNC_COARSE, which also seems easier to do with a
command-line option than with special targets) and modified the
check in 1. accordingly. Note that in the PARALLEL_SYNC_COARSE case,
the various recipes in each recursive jobs will sync against each
other on their stdout/stderr which is in fact the temp file created
by the higher-level parallel sync. So it still fixes the bug of a).

As a detail, the statement
"parallel_sync = assign_child_tempfiles(...);"
won't work this way with a tri-state. I've modified it to preserve
the value if assign_child_tempfiles() returns non-zero.

With those changes I now get this output with PARALLEL_SYNC_FINE
(which I activate with option "-P"):

make -C foo
make -C bar
make[1]: Entering directory 'foo'
make[1]: Entering directory 'bar'
[1s delay]
bar: start
bar: end
[1s delay]
foo: start
foo: end
make[1]: Leaving directory 'foo'
[2s delay]
baz: start
baz: end
make[1]: Leaving directory 'bar'

With PARALLEL_SYNC_COARSE ("-P2") I get:

make -C foo
make -C bar
[2s delay]
make[1]: Entering directory 'foo'
foo: start
foo: end
make[1]: Leaving directory 'foo'
[2s delay]
make[1]: Entering directory 'bar'
bar: start
bar: end
baz: start
baz: end
make[1]: Leaving directory 'bar'

This shows that a) is fixed, and b) is for PARALLEL_SYNC_FINE as it
should.

However, it also shows another problem:

c) In the "PARALLEL_SYNC_FINE" case, the "Entering/Leaving
   directory" messages do not properly relate to the messages. When
   one wants to use them to interpret the messages (such as with the
   "directory change tracking" changes to Emacs's compilation
   commands), this is misleading.

To fix that, I surround each recipe's output with enter/leave
messages in sync_output(). Notes:

- I had to add a 2nd parameter "force" to log_working_directory(),
  otherwise the new messages wouldn't appear since make thinks it
  has already shown them (which it has, but perhaps not recently
  enough). Since I didn't want to mess with the other message
  reporting places, this seems the least intrusive way. Of course,
  this can lead to redundant messages (as seen in the test output
  below), but since those messages in this case are mostly meant to
  be read by machines (e.g. Emacs), it shouldn't hurt too much. It's
  more important that the messages are correct.

- I check if the temporary files are empty before copying them,
  surrounded by the messages. Since usually (at least for me) most
  individual recipes produce no output at all, this avoids many of
  those redundant messages. Also (independent from the new
  messages), it avoids acquiring the semaphore if there is nothing
  to write so it might (slighty) increase throughput. (To do this
  properly, I had to move the close() call from pump_from_tmp_fd()
  to sync_output().)

- The change shows an asymmetry between stdout and stderr. The
  messages only apply to the former since log_working_directory()
  writes them to stdout. But if stdout and stderr are merged (the
  usual case), this is alright, since the copying of stdout handles
  the merged temp file. If they are not merged, there's nothing we
  can do easily, since stderr does not get enter/leave messages
  anyway.

I now get with "-P":

make -C foo
make -C bar
make[1]: Entering directory 'foo'
make[1]: Entering directory 'bar'
[1s delay]
make[1]: Entering directory 'bar'
bar: start
bar: end
make[1]: Leaving directory 'bar'
[1s delay]
make[1]: Entering directory 'foo'
foo: start
foo: end
make[1]: Leaving directory 'foo'
make[1]: Leaving directory 'foo'
[2s delay]
make[1]: Entering directory 'bar'
baz: start
baz: end
make[1]: Leaving directory 'bar'
make[1]: Leaving directory 'bar'

and with "-P2":

make -C foo
make -C bar
[2s delay]
make[1]: Entering directory 'foo'
make[1]: Entering directory 'foo'
foo: start
foo: end
make[1]: Leaving directory 'foo'
make[1]: Leaving directory 'foo'
[2s delay]
make[1]: Entering directory 'bar'
make[1]: Entering directory 'bar'
bar: start
bar: end
make[1]: Leaving directory 'bar'
make[1]: Entering directory 'bar'
baz: start
baz: end
make[1]: Leaving directory 'bar'
make[1]: Leaving directory 'bar'

This looks good to me, so I hope the patch is now ready for
inclusion if no new issues are found.

I've added a modified patch (make-sync-recursive.patch) including
the above-mentioned changes and corresponding documentation changes.
It also fixes the two bugs mentioned in my previous comment, though
it does not change anything WRT the first two points mentioned there
which are not bugs, rather matters of preference.


(file #27203, file #27204)

Frank Heckenbach <frank>
Sat 29 Dec 2012 12:58:29 AM UTC, comment #2: 

Some remarks on the current patch:

assign_child_tempfiles():

+  /* Check status of stdout and stderr before hooking up temp files. */
+  o_ok = STREAM_OK(stdout);
+  e_ok = STREAM_OK(stderr);

Why do you check these for each recipe and not just once like you do for combined_output? -- Actually, you do check STREAM_OK already in the combined_output checks, so can't you store the results? Sure, it saves just 2 system calls, but it seems to me these things belong together and thus should be treated the same, if only to make the code clearer. In the end, all these checks combined yield four possibilities, stdout only, stderr only, both separate, both combined. It seems confusing to pass one part of the information as a parameter and recheck the rest.

+  /* The tmpfile() function returns a FILE pointer but those can be in
+     limited supply, so we immediately dup its file descriptor and keep
+     only that, closing the FILE pointer.  */

On POSIX, you could perhaps use mkstemp() which returns an FD directly.

pump_from_tmp_fd():

+  ssize_t nleft, nwrite;
+  char buffer[8192];
+       while (nleft > 0)
+       {
+         EINTRLOOP(nwrite, write(to_fd, buffer, nleft));
+         [...]
+         nleft -= nwrite;
+       }

If the loop runs more than once, it will write the first part of the buffer repeatedly. You need an offset that is increased by nwrite.

acquire_semaphore():

+  fl.l_pid = getpid();
+  fl.l_start = fl.l_pid; /* lock just one byte according to pid */
+  fl.l_len = 1;

Why do you lock a different byte for each PID? The purpose of a semaphore is mutual exclusion, so I think you should lock the same byte every time.

Furthermore, you don't need to set l_pid to the current PID. It's an output field used by F_GETLK to return the PID of the process holding the lock.

General:

I'm not too happy with it being triggered by a special target (.PARALLELSYNC). Why should the Makefile dictate how the user sees the output? Perhaps some users want immediate feedback (as mentioned by Eli Zaretskii in the original discussion) while others prefer delayed, but properly grouped output. So why not use a command-line option that each user can set to their preferences, just like they can with "-j" itself?

Frank Heckenbach <frank>
Fri 22 Apr 2011 02:01:53 PM UTC, comment #1: 

Latest patch attached. This works well in my tests on Solaris and Linux. I know of no reason it wouldn't work the same on any POSIX platform, and it's ifdef-ed so as to not break the build on any non-POSIX platform. A start on documentation is included.

(file #23292)

David Boyce <boyski>
Thu 21 Apr 2011 04:17:07 PM UTC, original submission:  

A patch will be provided to support a new special target, tentatively called .PARALLELSYNC, which keeps the output of parallel jobs discrete. The background is amply described in a long thread starting at http://lists.gnu.org/archive/html/bug-make/2011-04/msg00018.html

My patch will only address POSIX systems but with luck this can be ported to the full set of platforms which also support -j

David Boyce <boyski>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #27808:  make-sync-recursive-4.patch added by frank (20KiB - text/x-diff)
file #27793:  make-sync-recursive-3.patch added by frank (20KiB - text/x-diff)
file #27206:  make-sync-recursive-2.patch added by frank (19KiB - text/x-diff)
file #27203:  sync-recursive-demo.tar.gz added by frank (375B - application/gzip - Demo and fix for use with recursive makes)
file #27204:  make-sync-recursive.patch added by frank (15KiB - text/x-diff - Demo and fix for use with recursive makes)
file #23292:  make-sync.patch added by boyski (11KiB - application/octet-stream - Patch against make 3.82.90 circa 4/21/2011)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by psmith (Posted a comment)
  • -email is unavailable- added by frank (Posted a comment)
  • -email is unavailable- added by boyski (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-04-16 psmith StatusNone Fixed
        Assigned toNone psmith
        Open/ClosedOpen Closed
        Component Version4.0 3.82
        Fixed ReleaseNone 4.0
    2013-04-08 frank Attached File- Added make-sync-recursive-4.patch, #27808
    2013-04-06 frank Attached File- Added make-sync-recursive-3.patch, #27793
    2013-01-06 frank Attached File- Added make-sync-recursive-2.patch, #27206
    2013-01-05 frank Attached File- Added sync-recursive-demo.tar.gz, #27203
        Attached File- Added make-sync-recursive.patch, #27204
    2011-04-22 boyski Attached File- Added make-sync.patch, #23292

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code