| Message ID | 20190325103844.26749-3-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Progress display fixes | expand |
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.
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.
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
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.
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; }
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(-)