Re: [PATCH] progress: simplify "delayed" progress API

[Jeff King]

On Sat, Aug 19, 2017 at 10:39:41AM -0700, Junio C Hamano wrote:

> We used to expose the full power of the delayed progress API to the
> callers, so that they can specify, not just the message to show and
> expected total amount of work that is used to compute the percentage
> of work performed so far, the percent-threshold parameter P and the
> delay-seconds parameter N.  The progress meter starts to show at N
> seconds into the operation only if the amount of work completed
> exceeds P.
> 
> Most callers used either (0%, 2s) or (50%, 1s) as (P, N), but there
> are oddballs that chose more random-looking values like 95%.
> 
> For a smoother workload, (50%, 1s) would allow us to start showing
> the progress meter earlier than (0%, 2s), while keeping the chance
> of not showing progress meter for long running operation the same as
> the latter.  For a task that would take 2s or more to complete, it
> is likely that less than half of it would complete within the first
> second, if the workload is smooth.  But for a spiky workload whose
> earlier part is easier, such a setting is likely to fail to show the
> progress meter entirely and (0%, 2s) is more appropriate.
> 
> But that is merely a theory.  Realistically, it is of dubious value
> to ask each codepath to carefully consider smoothness of their
> workload and specify their own setting by passing two extra
> parameters.  Let's simplify the API by dropping both parameters and
> have everybody use (0%, 2s).

Nicely explained (modulo the reversal you noticed in the first
paragraph). The patch looks good to me.

> Oh, by the way, the percent-threshold parameter and the structure
> member were consistently misspelled, which also is now fixed ;-)

Heh, that was bugging me, too. :)

>  * So before I forget all about this topic, here is a patch to
>actually do this.
> 
>> So I'd vote to drop that parameter entirely. And if 1 second seems
>> noticeably snappier, then we should probably just move everything to a 1
>> second delay (I don't have a strong feeling either way).
> 
>I was also tempted to do this, but decided to keep it closer to
>the original for majority of callers by leaving the delay at 2s.
>With this patch, such a change as a follow-up is made a lot
>easier (somebody may want to even make a configuration out of it,
>but that would not be me).

Yeah, I agree that it can come later on top (if at all).

Thanks for finishing this off.

-Peff

Re: [PATCH] progress: simplify "delayed" progress API

[Junio C Hamano]

Junio C Hamano  writes:

> We used to expose the full power of the delayed progress API to the
> callers, so that they can specify, not just the message to show and
> expected total amount of work that is used to compute the percentage
> of work performed so far, the percent-threshold parameter P and the
> delay-seconds parameter N.  The progress meter starts to show at N
> seconds into the operation only if the amount of work completed
> exceeds P.

Oops, we inspect the doneness at N seconds and show only if we have
not yet completed P percent of the total work.

Perhaps

... at N seconds into the operation only if we have not
completed P per-cent of the total work.

[PATCH] progress: simplify "delayed" progress API

[Junio C Hamano]

We used to expose the full power of the delayed progress API to the
callers, so that they can specify, not just the message to show and
expected total amount of work that is used to compute the percentage
of work performed so far, the percent-threshold parameter P and the
delay-seconds parameter N.  The progress meter starts to show at N
seconds into the operation only if the amount of work completed
exceeds P.

Most callers used either (0%, 2s) or (50%, 1s) as (P, N), but there
are oddballs that chose more random-looking values like 95%.

For a smoother workload, (50%, 1s) would allow us to start showing
the progress meter earlier than (0%, 2s), while keeping the chance
of not showing progress meter for long running operation the same as
the latter.  For a task that would take 2s or more to complete, it
is likely that less than half of it would complete within the first
second, if the workload is smooth.  But for a spiky workload whose
earlier part is easier, such a setting is likely to fail to show the
progress meter entirely and (0%, 2s) is more appropriate.

But that is merely a theory.  Realistically, it is of dubious value
to ask each codepath to carefully consider smoothness of their
workload and specify their own setting by passing two extra
parameters.  Let's simplify the API by dropping both parameters and
have everybody use (0%, 2s).

Oh, by the way, the percent-threshold parameter and the structure
member were consistently misspelled, which also is now fixed ;-)

Helped-by: Jeff King 
Signed-off-by: Junio C Hamano 
---

 * So before I forget all about this topic, here is a patch to
   actually do this.

   > So I'd vote to drop that parameter entirely. And if 1 second seems
   > noticeably snappier, then we should probably just move everything to a 1
   > second delay (I don't have a strong feeling either way).

   I was also tempted to do this, but decided to keep it closer to
   the original for majority of callers by leaving the delay at 2s.
   With this patch, such a change as a follow-up is made a lot
   easier (somebody may want to even make a configuration out of it,
   but that would not be me).

 builtin/blame.c|  3 +--
 builtin/fsck.c |  2 +-
 builtin/prune-packed.c |  3 +--
 builtin/prune.c|  2 +-
 builtin/rev-list.c |  2 +-
 diffcore-rename.c  |  4 ++--
 progress.c | 15 ++-
 progress.h |  3 +--
 unpack-trees.c |  3 +--
 9 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index bda1a78726..e0daf17548 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -925,8 +925,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
sb.found_guilty_entry = _guilty_entry;
sb.found_guilty_entry_data = 
if (show_progress)
-   pi.progress = start_progress_delay(_("Blaming lines"),
-  sb.num_lines, 50, 1);
+   pi.progress = start_delayed_progress(_("Blaming lines"), 
sb.num_lines);
 
assign_blame(, opt);
 
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 99dea7adf6..0031439fc4 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -188,7 +188,7 @@ static int traverse_reachable(void)
unsigned int nr = 0;
int result = 0;
if (show_progress)
-   progress = start_progress_delay(_("Checking connectivity"), 0, 
0, 2);
+   progress = start_delayed_progress(_("Checking connectivity"), 
0);
while (pending.nr) {
struct object_array_entry *entry;
struct object *obj;
diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index ac978ad401..8f41f7c20e 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -37,8 +37,7 @@ static int prune_object(const struct object_id *oid, const 
char *path,
 void prune_packed_objects(int opts)
 {
if (opts & PRUNE_PACKED_VERBOSE)
-   progress = start_progress_delay(_("Removing duplicate objects"),
-   256, 95, 2);
+   progress = start_delayed_progress(_("Removing duplicate 
objects"), 256);
 
for_each_loose_file_in_objdir(get_object_directory(),
  prune_object, NULL, prune_subdir, );
diff --git a/builtin/prune.c b/builtin/prune.c
index c378690545..cddabf26a9 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -138,7 +138,7 @@ int cmd_prune(int argc, const char **argv, const char 
*prefix)
if (show_progress == -1)
show_progress = isatty(2);
if (show_progress)
-   progress = start_progress_delay(_("Checking connectivity"), 0, 
0, 2);
+   progress = start_delayed_progress(_("Checking connectivity"), 
0);
 
mark_reachable_objects(, 1, expire, progress);
stop_progress();
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index