git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Cc: Philip Oakley <philipoakley@iee.org>,
	Kevin Willford <kcwillford@gmail.com>,
	Kevin Willford <kewillf@microsoft.com>, Jeff King <peff@peff.net>
Subject: [PATCH] progress: simplify "delayed" progress API
Date: Sat, 19 Aug 2017 10:39:41 -0700	[thread overview]
Message-ID: <xmqqy3qf8nj6.fsf_-_@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170814222947.edvuz7b2hxuwcsqj@sigill.intra.peff.net> (Jeff King's message of "Mon, 14 Aug 2017 18:29:47 -0400")

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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * 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 = &found_guilty_entry;
 	sb.found_guilty_entry_data = &pi;
 	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(&sb, 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, &opts);
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(&revs, 1, expire, progress);
 	stop_progress(&progress);
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 95d84d5cda..dfad8e847a 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -364,7 +364,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		revs.limited = 1;
 
 	if (show_progress)
-		progress = start_progress_delay(show_progress, 0, 0, 2);
+		progress = start_delayed_progress(show_progress, 0);
 
 	if (use_bitmap_index && !revs.prune) {
 		if (revs.count && !revs.left_right && !revs.cherry_mark) {
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 786f389498..0d8c3d2ee4 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -532,9 +532,9 @@ void diffcore_rename(struct diff_options *options)
 	}
 
 	if (options->show_rename_progress) {
-		progress = start_progress_delay(
+		progress = start_delayed_progress(
 				_("Performing inexact rename detection"),
-				rename_dst_nr * rename_src_nr, 50, 1);
+				rename_dst_nr * rename_src_nr);
 	}
 
 	mx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx));
diff --git a/progress.c b/progress.c
index 73e36d4a42..289678d43d 100644
--- a/progress.c
+++ b/progress.c
@@ -34,7 +34,7 @@ struct progress {
 	unsigned total;
 	unsigned last_percent;
 	unsigned delay;
-	unsigned delayed_percent_treshold;
+	unsigned delayed_percent_threshold;
 	struct throughput *throughput;
 	uint64_t start_ns;
 };
@@ -88,7 +88,7 @@ static int display(struct progress *progress, unsigned n, const char *done)
 			return 0;
 		if (progress->total) {
 			unsigned percent = n * 100 / progress->total;
-			if (percent > progress->delayed_percent_treshold) {
+			if (percent > progress->delayed_percent_threshold) {
 				/* inhibit this progress report entirely */
 				clear_progress_signal();
 				progress->delay = -1;
@@ -205,8 +205,8 @@ int display_progress(struct progress *progress, unsigned n)
 	return progress ? display(progress, n, NULL) : 0;
 }
 
-struct progress *start_progress_delay(const char *title, unsigned total,
-				       unsigned percent_treshold, unsigned delay)
+static struct progress *start_progress_delay(const char *title, unsigned total,
+					     unsigned percent_threshold, unsigned delay)
 {
 	struct progress *progress = malloc(sizeof(*progress));
 	if (!progress) {
@@ -219,7 +219,7 @@ struct progress *start_progress_delay(const char *title, unsigned total,
 	progress->total = total;
 	progress->last_value = -1;
 	progress->last_percent = -1;
-	progress->delayed_percent_treshold = percent_treshold;
+	progress->delayed_percent_threshold = percent_threshold;
 	progress->delay = delay;
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
@@ -227,6 +227,11 @@ struct progress *start_progress_delay(const char *title, unsigned total,
 	return progress;
 }
 
+struct progress *start_delayed_progress(const char *title, unsigned total)
+{
+	return start_progress_delay(title, total, 0, 2);
+}
+
 struct progress *start_progress(const char *title, unsigned total)
 {
 	return start_progress_delay(title, total, 0, 0);
diff --git a/progress.h b/progress.h
index 611e4c4d42..6392b63371 100644
--- a/progress.h
+++ b/progress.h
@@ -6,8 +6,7 @@ struct progress;
 void display_throughput(struct progress *progress, off_t total);
 int display_progress(struct progress *progress, unsigned n);
 struct progress *start_progress(const char *title, unsigned total);
-struct progress *start_progress_delay(const char *title, unsigned total,
-				       unsigned percent_treshold, unsigned delay);
+struct progress *start_delayed_progress(const char *title, unsigned total);
 void stop_progress(struct progress **progress);
 void stop_progress_msg(struct progress **progress, const char *msg);
 
diff --git a/unpack-trees.c b/unpack-trees.c
index dd535bc849..e5ae7fe183 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -343,8 +343,7 @@ static struct progress *get_progress(struct unpack_trees_options *o)
 			total++;
 	}
 
-	return start_progress_delay(_("Checking out files"),
-				    total, 50, 1);
+	return start_delayed_progress(_("Checking out files"), total);
 }
 
 static int check_updates(struct unpack_trees_options *o)
-- 
2.14.1-396-gef255185d2


  parent reply	other threads:[~2017-08-19 17:39 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-31 15:04 [PATCH 0/2] Add progress to format-patch and rebase Kevin Willford
2017-05-31 15:04 ` [PATCH 1/2] format-patch: have progress option while generating patches Kevin Willford
2017-05-31 18:40   ` Stefan Beller
2017-05-31 19:31     ` Kevin Willford
2017-05-31 22:01   ` Jeff King
2017-06-01  4:10     ` Junio C Hamano
2017-06-01 11:15     ` Johannes Schindelin
2017-06-01 15:54       ` Jeff King
2017-05-31 15:04 ` [PATCH 2/2] rebase: turn on progress option by default for format-patch Kevin Willford
2017-05-31 19:08   ` Stefan Beller
2017-05-31 19:46     ` Kevin Willford
2017-05-31 20:27       ` Stefan Beller
2017-06-01 11:11     ` Johannes Schindelin
2017-05-31 22:11   ` Jeff King
2017-06-03 23:45     ` Junio C Hamano
2017-08-10 18:32 ` [PATCH v2 0/2] Add progress for format-patch and rebase Kevin Willford
2017-08-10 22:48   ` Junio C Hamano
2017-08-10 23:17     ` Jeff King
2017-08-10 18:32 ` [PATCH v2 1/2] format-patch: have progress option while generating patches Kevin Willford
2017-08-10 23:20   ` Jeff King
2017-08-11 22:18     ` Junio C Hamano
2017-08-12  8:06       ` Philip Oakley
2017-08-13  4:39         ` Jeff King
2017-08-14 16:45           ` Junio C Hamano
2017-08-14 18:35             ` Junio C Hamano
2017-08-14 22:29               ` Jeff King
2017-08-14 22:42                 ` Junio C Hamano
2017-08-14 23:08                   ` Jeff King
2017-08-14 23:23                     ` Junio C Hamano
2017-08-19 17:39                 ` Junio C Hamano [this message]
2017-08-19 20:58                   ` [PATCH] progress: simplify "delayed" progress API Junio C Hamano
2017-08-20  7:43                   ` Jeff King
2017-08-10 18:32 ` [PATCH v2 2/2] rebase: turn on progress option by default for format-patch Kevin Willford
2017-08-11 22:22   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqy3qf8nj6.fsf_-_@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=kcwillford@gmail.com \
    --cc=kewillf@microsoft.com \
    --cc=peff@peff.net \
    --cc=philipoakley@iee.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).