[PATCH] reduce progress updates in background

Hi,

I've noticed that when a long-running git operation that generates
progress output is suspended and converted to a background process,
the terminal still gets spammed with progress updates (to stderr).

Many years ago I fixed a similar issue in the NetBSD ftp progress
bar code (which I wrote).

I've experimented around with a couple of different solutions, including:
1. suppress all progress output whilst in the background
2. suppress "in progress" updates whilst in the background,
   but display the "done" message even if in the background.

In both cases, warnings were still output to the terminal.

I've attached a patch that implements (2) above.

If the consensus is that all progress messages should be suppressed,
I can provide the (simpler) patch for that.

I've explicitly separated the in_progress_fd() function
so that it's easier to (a) reuse elsewhere where appropriate,
and (b) make any portability changes to the test if necessary.
I also used getpgid(0) versus getpgrp() to avoid portability
issues with the signature in the latter with pre-POSIX.

A minor optimisation could be to pass in struct progress *
and to cache getpgid(0) in a member of struct progress
in start_progress_delay(), since this value shouldn't change
during the life of the process.

regards,
Luke.

On Mon, 13 Apr 2015, Luke Mewburn wrote:

Hi,

I've noticed that when a long-running git operation that generates
progress output is suspended and converted to a background process,
the terminal still gets spammed with progress updates (to stderr).

Many years ago I fixed a similar issue in the NetBSD ftp progress
bar code (which I wrote).

I've experimented around with a couple of different solutions, including:
1. suppress all progress output whilst in the background
2. suppress "in progress" updates whilst in the background,
   but display the "done" message even if in the background.

In both cases, warnings were still output to the terminal.

I've attached a patch that implements (2) above.

If the consensus is that all progress messages should be suppressed,
I can provide the (simpler) patch for that.

I've explicitly separated the in_progress_fd() function
so that it's easier to (a) reuse elsewhere where appropriate,
and (b) make any portability changes to the test if necessary.
I also used getpgid(0) versus getpgrp() to avoid portability
issues with the signature in the latter with pre-POSIX.

A minor optimisation could be to pass in struct progress *
and to cache getpgid(0) in a member of struct progress
in start_progress_delay(), since this value shouldn't change
during the life of the process.

What if you suspend the task and push it into the background? Would be 
nice to inhibit progress display in that case, and resume it if the task 
returns to the foreground.

Also the display() function may be called quite a lot without 
necessarily resulting in a display output. Therefore I'd suggest adding 
in_progress_fd() to the if condition right before the printf() instead.


Nicolas

On Mon, Apr 13, 2015 at 10:11:09AM -0400, Nicolas Pitre wrote:
  | What if you suspend the task and push it into the background? Would be 
  | nice to inhibit progress display in that case, and resume it if the task 
  | returns to the foreground.

That's what happens; the suppression only occurs if the process is
currently background.  If I start a long-running operation (such as "git
fsck"), the progress is displayed. I then suspend & background, and the
progress is suppressed.  If I resume the process in the foreground, the
progress starts to display again at the appropriate point.

In the proposed patch, the stop_progress display for a given progress
(i.e. the one that ends in ", done.") is displayed even if in the
background so that there's some indication of progress. E.g.
  Checking object directories: 100% (256/256), done.
  Checking objects: 100% (184664/184664), done.
  Checking connectivity: 184667, done.
This is the test 'if (is_foreground || done)'.

I'm not 100% happy with my choice here, and the simpler behaviour
of "suppress all background progress output" can be achieved by
removing '|| done' from those two tests.

That still doesn't suppress _all_ output whilst in the background.
In order to do that, a larger refactor of various warning methods
would be required. I would argue that's a separate orthoganal fix.


  | Also the display() function may be called quite a lot without 
  | necessarily resulting in a display output. Therefore I'd suggest adding 
  | in_progress_fd() to the if condition right before the printf() instead.

That's an easy enough change to make (although I speculate that the
testing of the foreground status is not that big a performance issue,
especially compared the the existing performance "overhead" of printing
the progress to stderr then forcing a flush :)


Should I submit a revised patch with
(1) call in_progress_fd() just before the fprintf() as requested, and
(2) suppress all display output including the "done" call.
?


regards,
Luke.

On Tue, 14 Apr 2015, Luke Mewburn wrote:

On Mon, Apr 13, 2015 at 10:11:09AM -0400, Nicolas Pitre wrote:
  | What if you suspend the task and push it into the background? Would be 
  | nice to inhibit progress display in that case, and resume it if the task 
  | returns to the foreground.

That's what happens; the suppression only occurs if the process is
currently background.  If I start a long-running operation (such as "git
fsck"), the progress is displayed. I then suspend & background, and the
progress is suppressed.  If I resume the process in the foreground, the
progress starts to display again at the appropriate point.

I agree. I was just comenting on your suggestion about caching the 
in_progress_fd() result which would prevent that.

In the proposed patch, the stop_progress display for a given progress
(i.e. the one that ends in ", done.") is displayed even if in the
background so that there's some indication of progress. E.g.
  Checking object directories: 100% (256/256), done.
  Checking objects: 100% (184664/184664), done.
  Checking connectivity: 184667, done.
This is the test 'if (is_foreground || done)'.

Yes.  And I think this is nice.

  | Also the display() function may be called quite a lot without 
  | necessarily resulting in a display output. Therefore I'd suggest adding 
  | in_progress_fd() to the if condition right before the printf() instead.

That's an easy enough change to make (although I speculate that the
testing of the foreground status is not that big a performance issue,
especially compared the the existing performance "overhead" of printing
the progress to stderr then forcing a flush :)

Sure.  But what I'm saying is that progress() may be called a thousand 
times and only one or two of those calls will result in an actual 
print-out. So it is best to test the foreground status only at that 
point.

Should I submit a revised patch with
(1) call in_progress_fd() just before the fprintf() as requested, and
(2) suppress all display output including the "done" call.
?

I'd suggest (1) but not (2).


Nicolas

On Mon, Apr 13, 2015 at 11:48:50PM +1000, Luke Mewburn wrote:

Hi,

I've noticed that when a long-running git operation that generates
progress output is suspended and converted to a background process,
the terminal still gets spammed with progress updates (to stderr).

I've explicitly separated the in_progress_fd() function
so that it's easier to (a) reuse elsewhere where appropriate,
and (b) make any portability changes to the test if necessary.
I also used getpgid(0) versus getpgrp() to avoid portability
issues with the signature in the latter with pre-POSIX.

I like this patch.  It's simple and seems like a sensible change, and I
appreciated the opportunity to learn about tcgetpgrp(3).  The Windows
folks will probably need to stub that function out, but they're no worse
off than they were before.

I do agree with Nicolas that optimizing the code to avoid calling
in_progress_fd as much as possible is a good idea, since system calls
can be expensive on some systems.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

Hi Brian,

On 2015-04-14 05:12, brian m. carlson wrote:

On Mon, Apr 13, 2015 at 11:48:50PM +1000, Luke Mewburn wrote:

I appreciated the opportunity to learn about tcgetpgrp(3).  The Windows
folks will probably need to stub that function out, but they're no worse
off than they were before.

Thanks for thinking of us!

Ciao,
Dscho

Updated patch where is_foreground_fd() is only called in display()
just before the output is to be displayed.

On Mon, Apr 13, 2015 at 11:01:04AM -0400, Nicolas Pitre wrote:
  | > That's what happens; the suppression only occurs if the process is
  | > currently background.  If I start a long-running operation (such as "git
  | > fsck"), the progress is displayed. I then suspend & background, and the
  | > progress is suppressed.  If I resume the process in the foreground, the
  | > progress starts to display again at the appropriate point.
  | 
  | I agree. I was just comenting on your suggestion about caching the 
  | in_progress_fd() result which would prevent that.

Ahh.  My suggestion about is_foreground_fd() result caching within
struct progress was only about caching the getpgid(0) portion of the
test (as that's not expected to change for the life of the process), and
not the tcgetpgrp(fd) portion.  I.e, add 'int curpgid' to struct
progress, set that to getpgid(0) in start_progress_display(), and
compare tcgetpgrp(fd) against progress->curpgid.

In any case, I think it's a micro optimisation not worth worrying about
at this point, given is_foreground_fd() is only called each time the
output would change, per your feedback.

regards,
Luke.

On Tue, 14 Apr 2015, Luke Mewburn wrote:

Updated patch where is_foreground_fd() is only called in display()
just before the output is to be displayed.

Acked-by: Nicolas Pitre <nico@fluxnic.net>

Windows does not have process groups. It is, therefore, the simplest
to pretend that each process is in its own process group.

While here, move the getppid() stub from its old location (between
two sync related functions) next to the two new functions.

Signed-off-by: Johannes Sixt <redacted>
---
 compat/mingw.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 7b523cf..a552026 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h

@@ -95,8 +95,6 @@ static inline unsigned int alarm(unsigned int seconds)
 { return 0; }
 static inline int fsync(int fd)
 { return _commit(fd); }
-static inline pid_t getppid(void)
-{ return 1; }
 static inline void sync(void)
 {}
 static inline uid_t getuid(void)

@@ -118,6 +116,12 @@ static inline int sigaddset(sigset_t *set, int signum)
 #define SIG_UNBLOCK 0
 static inline int sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
 { return 0; }
+static inline pid_t getppid(void)
+{ return 1; }
+static inline pid_t getpgid(pid_t pid)
+{ return pid == 0 ? getpid() : pid; }
+static inline pid_t tcgetpgrp(int fd)
+{ return getpid(); }
 
 /*
  * simple adaptors

-- 
2.3.2.245.gb5bf9d3

On Wed, Apr 15, 2015 at 8:29 PM, Johannes Sixt [off-list ref] wrote:

Windows does not have process groups. It is, therefore, the simplest
to pretend that each process is in its own process group.

Windows does have some concept of process groups, but probably not
quite what you want:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms682083%28v=vs.85%29.aspx

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Hi kusma,

On 2015-04-15 21:43, Erik Faye-Lund wrote:

On Wed, Apr 15, 2015 at 8:29 PM, Johannes Sixt [off-list ref] wrote:

quoted

Windows does not have process groups. It is, therefore, the simplest
to pretend that each process is in its own process group.

Windows does have some concept of process groups, but probably not
quite what you want:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms682083%28v=vs.85%29.aspx

Yes, and we actually need that in Git for Windows anyway because shooting down a process does not kill its child processes:

https://github.com/git-for-windows/msys2-runtime/commit/15f209511985092588b171703e5046eba937b47b#diff-8753cda163376cee6c80aab11eb8701fR402

However, using this code for `getppid()` would be serious overkill (not to mention an unbearable performance hit because you have to enumerate *all* processes to get that information).

Ciao,
Dscho

On Wed, Apr 15, 2015 at 08:29:30PM +0200, Johannes Sixt wrote:
  | Windows does not have process groups. It is, therefore, the simplest
  | to pretend that each process is in its own process group.
  | 
  |  [...]
  | 
  | diff --git a/compat/mingw.h b/compat/mingw.h
  | index 7b523cf..a552026 100644
  | @@ -118,6 +116,12 @@ static inline int sigaddset(sigset_t *set, int signum)
  |  #define SIG_UNBLOCK 0
  |  static inline int sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
  |  { return 0; }
  | +static inline pid_t getppid(void)
  | +{ return 1; }
  | +static inline pid_t getpgid(pid_t pid)
  | +{ return pid == 0 ? getpid() : pid; }
  | +static inline pid_t tcgetpgrp(int fd)
  | +{ return getpid(); }


This appears to be similar to the approach that tcsh uses too;
return the current process ID for the process group ID.
See https://github.com/tcsh-org/tcsh/blob/master/win32/ntport.h
for tcsh's implementation of getpgrp() (a variation of getpgid())
and tcgetpgrp().


regards,
Luke.

On Thursday, April 16, 2015 at 2:45:11 PM UTC+2, Johannes Schindelin wrote:

Hi kusma, 

On 2015-04-15 21:43, Erik Faye-Lund wrote: 

quoted

On Wed, Apr 15, 2015 at 8:29 PM, Johannes Sixt <j...@kdbg.org 

<javascript:>> wrote: 

quoted

quoted

Windows does not have process groups. It is, therefore, the simplest 
to pretend that each process is in its own process group. 

Windows does have some concept of process groups, but probably not 
quite what you want: 

https://msdn.microsoft.com/en-us/library/windows/desktop/ms682083%28v=vs.85%29.aspx 

Yes, and we actually need that in Git for Windows anyway because shooting 
down a process does not kill its child processes: 


https://github.com/git-for-windows/msys2-runtime/commit/15f209511985092588b171703e5046eba937b47b#diff-8753cda163376cee6c80aab11eb8701fR402 

However, using this code for `getppid()` would be serious overkill (not to 
mention an unbearable performance hit because you have to enumerate *all* 
processes to get that information). 

is the windows "JobObject" similar to processgroup? at least killing the 
parent process in a jobobject will kill all childs as well:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms681949%28v=vs.85%29.aspx 

best,
rupert
 

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

On Thursday, April 23, 2015 at 9:25:49 PM UTC+2, rupert thurner wrote:

On Thursday, April 16, 2015 at 2:45:11 PM UTC+2, Johannes Schindelin wrote:

quoted

Hi kusma, 

On 2015-04-15 21:43, Erik Faye-Lund wrote: 

quoted

On Wed, Apr 15, 2015 at 8:29 PM, Johannes Sixt [off-list ref] wrote: 

quoted

Windows does not have process groups. It is, therefore, the simplest 
to pretend that each process is in its own process group. 

Windows does have some concept of process groups, but probably not 
quite what you want: 

https://msdn.microsoft.com/en-us/library/windows/desktop/ms682083%28v=vs.85%29.aspx 

Yes, and we actually need that in Git for Windows anyway because shooting 
down a process does not kill its child processes: 


https://github.com/git-for-windows/msys2-runtime/commit/15f209511985092588b171703e5046eba937b47b#diff-8753cda163376cee6c80aab11eb8701fR402 

However, using this code for `getppid()` would be serious overkill (not 
to mention an unbearable performance hit because you have to enumerate 
*all* processes to get that information). 

is the windows "JobObject" similar to processgroup? at least killing the 
parent process in a jobobject will kill all childs as well:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms681949%28v=vs.85%29.aspx
 

here some discussion of windows console process group, 
CREATE_NEW_PROCESS_GROUP, GenerateConsoleCtrlEvent, and job object from the 
last century:
https://www.microsoft.com/msj/0698/win320698.aspx

rupert

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Hi Rupert,

On 2015-04-23 21:25, rupert thurner wrote:

On Thursday, April 16, 2015 at 2:45:11 PM UTC+2, Johannes Schindelin wrote:

quoted

However, using this code for `getppid()` would be serious overkill (not to
mention an unbearable performance hit because you have to enumerate *all*
processes to get that information).

is the windows "JobObject" similar to processgroup? at least killing the 
parent process in a jobobject will kill all childs as well:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms681949%28v=vs.85%29.aspx

Reading this page carefully reveals that you have to construct JobObjects explicitly. So you cannot get from a process ID to a JobObject; there is probably none. Or there is one. Or many.

Ciao,
Johannes