[2/5] progress: return early when in the background
diff mbox series

Message ID 20190325103844.26749-3-szeder.dev@gmail.com
State New
Headers show
Series
  • Progress display fixes
Related show

Commit Message

SZEDER Gábor March 25, 2019, 10:38 a.m. UTC
When a git process runs in the background, it doesn't display
progress, only the final "done" line [1].  The condition to check that
are a bit too deep in the display() function, and thus it calculates
the progress percentage even when no progress will be displayed
anyway.

Restructure the display() function to return early when we are in the
background, which prevents the unnecessary progress percentae
calculation, and make the function look a bit better by losing one
level of indentation.

[1] 85cb8906f0 (progress: no progress in background, 2015-04-13)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Comments

Ævar Arnfjörð Bjarmason March 25, 2019, 11:08 a.m. UTC | #1
On Mon, Mar 25 2019, SZEDER Gábor wrote:

> When a git process runs in the background, it doesn't display
> progress, only the final "done" line [1].  The condition to check that
> are a bit too deep in the display() function, and thus it calculates
> the progress percentage even when no progress will be displayed
> anyway.
>
> Restructure the display() function to return early when we are in the
> background, which prevents the unnecessary progress percentae
> calculation, and make the function look a bit better by losing one
> level of indentation.
>
> [1] 85cb8906f0 (progress: no progress in background, 2015-04-13)

CC-ing the author of that patch.

> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  progress.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index 02a20e7d58..b57c0dae16 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)
>  		return;
>
>  	progress->last_value = n;
> +
> +	if (!is_foreground_fd(fileno(stderr)) && !done) {
> +		progress_update = 0;
> +		return;
> +	}
> +
>  	tp = (progress->throughput) ? progress->throughput->display.buf : "";
>  	eol = done ? done : "   \r";
>  	if (progress->total) {
>  		unsigned percent = n * 100 / progress->total;
>  		if (percent != progress->last_percent || progress_update) {
>  			progress->last_percent = percent;
> -			if (is_foreground_fd(fileno(stderr)) || done) {
> -				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> -					progress->title, percent,
> -					(uintmax_t)n, (uintmax_t)progress->total,
> -					tp, eol);
> -				fflush(stderr);
> -			}
> +			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> +				progress->title, percent,
> +				(uintmax_t)n, (uintmax_t)progress->total,
> +				tp, eol);
> +			fflush(stderr);
>  			progress_update = 0;
>  			return;
>  		}
>  	} else if (progress_update) {
> -		if (is_foreground_fd(fileno(stderr)) || done) {
> -			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
> -				progress->title, (uintmax_t)n, tp, eol);
> -			fflush(stderr);
> -		}
> +		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
> +			progress->title, (uintmax_t)n, tp, eol);
> +		fflush(stderr);
>  		progress_update = 0;
>  		return;
>  	}

This patch looks good, just notes for potential follow-up:

 * Is the "is_foreground_fd(fileno(stderr))" case worth moving into
   start_progress_delay() & setting a variable? It's a few C lib calls &
   potential syscall (getpid(...)).

 * Is that "|| done" part in the "progress_update" case something that
   needs to happen? I.e. can we entirely skip the "setup signal handler"
   part in start_progress_delay() if we detect that we're not in the
   foreground, and then rely on the stop_progress() call to print the
   "done"?

   Although we set "progress_update = 1" in stop_progress_msg(), so it's
   not *just* the signal handler but also us "faking" it, and we'd still
   need to stash away "progress->last_value = n" in display() in that
   backgrounding case.

   So maybe it's as simple as it's going to get.
SZEDER Gábor March 25, 2019, 11:39 a.m. UTC | #2
On Mon, Mar 25, 2019 at 12:08:47PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Mar 25 2019, SZEDER Gábor wrote:
> 
> > When a git process runs in the background, it doesn't display
> > progress, only the final "done" line [1].  The condition to check that
> > are a bit too deep in the display() function, and thus it calculates
> > the progress percentage even when no progress will be displayed
> > anyway.
> >
> > Restructure the display() function to return early when we are in the
> > background, which prevents the unnecessary progress percentae
> > calculation, and make the function look a bit better by losing one
> > level of indentation.
> >
> > [1] 85cb8906f0 (progress: no progress in background, 2015-04-13)
> 
> CC-ing the author of that patch.
> 
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > ---
> >  progress.c | 26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/progress.c b/progress.c
> > index 02a20e7d58..b57c0dae16 100644
> > --- a/progress.c
> > +++ b/progress.c
> > @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)
> >  		return;
> >
> >  	progress->last_value = n;
> > +
> > +	if (!is_foreground_fd(fileno(stderr)) && !done) {
> > +		progress_update = 0;
> > +		return;
> > +	}
> > +
> >  	tp = (progress->throughput) ? progress->throughput->display.buf : "";
> >  	eol = done ? done : "   \r";
> >  	if (progress->total) {
> >  		unsigned percent = n * 100 / progress->total;
> >  		if (percent != progress->last_percent || progress_update) {
> >  			progress->last_percent = percent;
> > -			if (is_foreground_fd(fileno(stderr)) || done) {
> > -				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> > -					progress->title, percent,
> > -					(uintmax_t)n, (uintmax_t)progress->total,
> > -					tp, eol);
> > -				fflush(stderr);
> > -			}
> > +			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> > +				progress->title, percent,
> > +				(uintmax_t)n, (uintmax_t)progress->total,
> > +				tp, eol);
> > +			fflush(stderr);
> >  			progress_update = 0;
> >  			return;
> >  		}
> >  	} else if (progress_update) {
> > -		if (is_foreground_fd(fileno(stderr)) || done) {
> > -			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
> > -				progress->title, (uintmax_t)n, tp, eol);
> > -			fflush(stderr);
> > -		}
> > +		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
> > +			progress->title, (uintmax_t)n, tp, eol);
> > +		fflush(stderr);
> >  		progress_update = 0;
> >  		return;
> >  	}
> 
> This patch looks good, just notes for potential follow-up:
> 
>  * Is the "is_foreground_fd(fileno(stderr))" case worth moving into
>    start_progress_delay() & setting a variable? It's a few C lib calls &
>    potential syscall (getpid(...)).

It depends on whether you consider the following case worth caring
about:

  $ git long-cmd
  <shows progress>
  Ctrl-Z!
  $ bg
  <silent>
  $ fg
  <shows progress>

Or:

  $ git long-cmd &
  <silent>
  $ fg
  <shows progress>

By moving the is_foreground_fd() check to start_progress_delay() and
caching its result, in the first case we would print progress even
after the process is sent to the background, while in the second we
wouldn't print progress even after the initially backgrounded process
is brought to the foreground.

I think the current behavior makes sense (though I'm not quite sure
about printing the final "done" line, as I think I would be annoyed if
it were printed from the background process while I was typing a
longer command... but I don't run git commands in the background in
the first place)

>  * Is that "|| done" part in the "progress_update" case something that
>    needs to happen? I.e. can we entirely skip the "setup signal handler"
>    part in start_progress_delay() if we detect that we're not in the
>    foreground, and then rely on the stop_progress() call to print the
>    "done"?

This, too, depends on how (or whether at all) we want to handle the
user sending the process to the background and bringing it back.

>    Although we set "progress_update = 1" in stop_progress_msg(), so it's
>    not *just* the signal handler but also us "faking" it, and we'd still
>    need to stash away "progress->last_value = n" in display() in that
>    backgrounding case.
> 
>    So maybe it's as simple as it's going to get.
Jeff King March 26, 2019, 5:38 a.m. UTC | #3
On Mon, Mar 25, 2019 at 11:38:41AM +0100, SZEDER Gábor wrote:

> diff --git a/progress.c b/progress.c
> index 02a20e7d58..b57c0dae16 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)
>  		return;
>  
>  	progress->last_value = n;
> +
> +	if (!is_foreground_fd(fileno(stderr)) && !done) {
> +		progress_update = 0;
> +		return;
> +	}
> +

Moving it here causes a measurable slowdown for:

  git rev-list --progress=foo --objects --all

This function gets called for every single increment of the progress
counter. Whereas in its old location:

>  	tp = (progress->throughput) ? progress->throughput->display.buf : "";
>  	eol = done ? done : "   \r";
>  	if (progress->total) {
>  		unsigned percent = n * 100 / progress->total;
>  		if (percent != progress->last_percent || progress_update) {
>  			progress->last_percent = percent;
> -			if (is_foreground_fd(fileno(stderr)) || done) {
> -				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
> -					progress->title, percent,
> -					(uintmax_t)n, (uintmax_t)progress->total,
> -					tp, eol);
> -				fflush(stderr);
> -			}

It was only triggered when we accumulated enough increments to print. So
we save a few instructions in the backgrounded case, but it costs us a
lot of extra syscalls in every other case.

According to "strace -c", the number of ioctls for that rev-list on
git.git went from 6 to 373,461. But more importantly, my best-of-five
timings went from 3.340s from 3.407s. That's only 2%, but it would be
nice not to pay it if we don't need to.

-Peff
Luke Mewburn March 26, 2019, 6:28 a.m. UTC | #4
On 19-03-25 12:39, SZEDER Gábor wrote:
  | On Mon, Mar 25, 2019 at 12:08:47PM +0100, Ævar Arnfjörð Bjarmason wrote:
  | > 
  | > On Mon, Mar 25 2019, SZEDER Gábor wrote:
  | > 
  | > > When a git process runs in the background, it doesn't display
  | > > progress, only the final "done" line [1].  The condition to check that
  | > > are a bit too deep in the display() function, and thus it calculates
  | > > the progress percentage even when no progress will be displayed
  | > > anyway.
  | > >
  | > > Restructure the display() function to return early when we are in the
  | > > background, which prevents the unnecessary progress percentae
  | > > calculation, and make the function look a bit better by losing one
  | > > level of indentation.
  | > >
  | > > [1] 85cb8906f0 (progress: no progress in background, 2015-04-13)
  | > 
  | > CC-ing the author of that patch.
  | > 
  | > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
  | > > ---
  | > >  progress.c | 26 ++++++++++++++------------
  | > >  1 file changed, 14 insertions(+), 12 deletions(-)
  | > >
  | > > diff --git a/progress.c b/progress.c
  | > > index 02a20e7d58..b57c0dae16 100644
  | > > --- a/progress.c
  | > > +++ b/progress.c
  | > > @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done)
  | > >  		return;
  | > >
  | > >  	progress->last_value = n;
  | > > +
  | > > +	if (!is_foreground_fd(fileno(stderr)) && !done) {
  | > > +		progress_update = 0;
  | > > +		return;
  | > > +	}
  | > > +
  | > >  	tp = (progress->throughput) ? progress->throughput->display.buf : "";
  | > >  	eol = done ? done : "   \r";
  | > >  	if (progress->total) {
  | > >  		unsigned percent = n * 100 / progress->total;
  | > >  		if (percent != progress->last_percent || progress_update) {
  | > >  			progress->last_percent = percent;
  | > > -			if (is_foreground_fd(fileno(stderr)) || done) {
  | > > -				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
  | > > -					progress->title, percent,
  | > > -					(uintmax_t)n, (uintmax_t)progress->total,
  | > > -					tp, eol);
  | > > -				fflush(stderr);
  | > > -			}
  | > > +			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
  | > > +				progress->title, percent,
  | > > +				(uintmax_t)n, (uintmax_t)progress->total,
  | > > +				tp, eol);
  | > > +			fflush(stderr);
  | > >  			progress_update = 0;
  | > >  			return;
  | > >  		}
  | > >  	} else if (progress_update) {
  | > > -		if (is_foreground_fd(fileno(stderr)) || done) {
  | > > -			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
  | > > -				progress->title, (uintmax_t)n, tp, eol);
  | > > -			fflush(stderr);
  | > > -		}
  | > > +		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
  | > > +			progress->title, (uintmax_t)n, tp, eol);
  | > > +		fflush(stderr);
  | > >  		progress_update = 0;
  | > >  		return;
  | > >  	}
  | > 
  | > This patch looks good, just notes for potential follow-up:
  | > 
  | >  * Is the "is_foreground_fd(fileno(stderr))" case worth moving into
  | >    start_progress_delay() & setting a variable? It's a few C lib calls &
  | >    potential syscall (getpid(...)).
  | 
  | It depends on whether you consider the following case worth caring
  | about:
  | 
  |   $ git long-cmd
  |   <shows progress>
  |   Ctrl-Z!
  |   $ bg
  |   <silent>
  |   $ fg
  |   <shows progress>
  | 
  | Or:
  | 
  |   $ git long-cmd &
  |   <silent>
  |   $ fg
  |   <shows progress>
  | 
  | By moving the is_foreground_fd() check to start_progress_delay() and
  | caching its result, in the first case we would print progress even
  | after the process is sent to the background, while in the second we
  | wouldn't print progress even after the initially backgrounded process
  | is brought to the foreground.
  | 
  | I think the current behavior makes sense (though I'm not quite sure
  | about printing the final "done" line, as I think I would be annoyed if
  | it were printed from the background process while I was typing a
  | longer command... but I don't run git commands in the background in
  | the first place)

You've described the current behaviour as I intended it
in the original patch. I.e,:
- Display progress if foreground.
- Suppress output if background.
- Check the foreground/background state each update in case it changed.

I based that on other tools that also dynamically change their
output/progress behaviour whether in the foreground or background.

Regarding the final "done" line; I think that's a matter of
personal preference. I'm not too fussed if that was changed
so that "done" isn't printed if in the background.


  | 
  | >  * Is that "|| done" part in the "progress_update" case something that
  | >    needs to happen? I.e. can we entirely skip the "setup signal handler"
  | >    part in start_progress_delay() if we detect that we're not in the
  | >    foreground, and then rely on the stop_progress() call to print the
  | >    "done"?
  | 
  | This, too, depends on how (or whether at all) we want to handle the
  | user sending the process to the background and bringing it back.
  | 
  | >    Although we set "progress_update = 1" in stop_progress_msg(), so it's
  | >    not *just* the signal handler but also us "faking" it, and we'd still
  | >    need to stash away "progress->last_value = n" in display() in that
  | >    backgrounding case.
  | > 
  | >    So maybe it's as simple as it's going to get.
  | 



regards,
Luke.

Patch
diff mbox series

diff --git a/progress.c b/progress.c
index 02a20e7d58..b57c0dae16 100644
--- a/progress.c
+++ b/progress.c
@@ -86,28 +86,30 @@  static void display(struct progress *progress, uint64_t n, const char *done)
 		return;
 
 	progress->last_value = n;
+
+	if (!is_foreground_fd(fileno(stderr)) && !done) {
+		progress_update = 0;
+		return;
+	}
+
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
 	eol = done ? done : "   \r";
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			if (is_foreground_fd(fileno(stderr)) || done) {
-				fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
-					progress->title, percent,
-					(uintmax_t)n, (uintmax_t)progress->total,
-					tp, eol);
-				fflush(stderr);
-			}
+			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
+				progress->title, percent,
+				(uintmax_t)n, (uintmax_t)progress->total,
+				tp, eol);
+			fflush(stderr);
 			progress_update = 0;
 			return;
 		}
 	} else if (progress_update) {
-		if (is_foreground_fd(fileno(stderr)) || done) {
-			fprintf(stderr, "%s: %"PRIuMAX"%s%s",
-				progress->title, (uintmax_t)n, tp, eol);
-			fflush(stderr);
-		}
+		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
+			progress->title, (uintmax_t)n, tp, eol);
+		fflush(stderr);
 		progress_update = 0;
 		return;
 	}