threads / patch / 36167

patch, 2 partspack-bitmap progress meters

Subject: [PATCH 0/2] pack-bitmap progress meters

## tl;dr

3 messages between Mar 15, 2014 and Mar 15, 2014. Diffs are folded; open one to read it.

replies: 2people: 1as markdown or json

Jeff King· Mar 15, 2014, 02:25 UTC · lore

Here are patches to make the pack-objects progress meters behave the same both with and without pack reuse. The first one is basically the patch I posted earlier.

The second one drops the "Reusing existing pack", and just rolls those numbers into "Counting objects". I have mixed feelings on it. _I_ find it interesting to know whether the pack was reused. But then I am often debugging bitmaps and packfiles. :) From a regular user's perspective, it is an implementation detail that may not be so interesting (git should just magically be faster, and they do not have to care why).

So I dunno. Opinions welcome.
  [1/2]: pack-objects: show progress for reused packfiles
  [2/2]: pack-objects: show reused packfile objects in "Counting objects"
-Peff
Jeff King· Mar 15, 2014, 02:26 UTC · re: Jeff King · lore

[PATCH 1/2] pack-objects: show progress for reused packfiles

When the "--all-progress" option is in effect, pack-objects shows a progress report for the "writing" phase. If the repository has bitmaps and we are reusing a packfile, the user sees no progress update until the whole packfile is sent. Since this is typically the bulk of what is being written, it can look like git hangs during this phase, even though the transfer is proceeding.

This generally only happens with "git push" from a repository with bitmaps. We do not use "--all-progress" for fetch (since the result is going to index-pack on the client, which takes care of progress reporting). And for regular repacks to disk, we do not reuse packfiles.

We already have the progress meter setup during write_reused_pack; we just need to call display_progress whiel we are writing out the pack. The progress meter is attached to our output descriptor, so it automatically handles the throughput measurements.

However, we need to update the object count as we go, since that is what feeds the percentage we show. We aren't actually parsing the packfile as we send it, so we have no idea how many objects we have sent; we only know that at the end of N bytes, we will have sent M objects. So we cheat a little and assume each object is M/N bytes (i.e., the mean of the objects we are sending). While this isn't strictly true, it actually produces a more pleasing progress meter for the user, as it moves smoothly and predictably (and nobody really cares about the object count; they care about the percentage, and the object count is a proxy for that).

One alternative would be to actually show two progress meters: one for the reused pack, and one for the rest of the objects. That would more closely reflect the data we have (the first would be measured in bytes, and the second measured in objects). But it would also be more complex and annoying to the user; rather than seeing one progress meter counting up to 100%, they would finish one meter, then start another one at zero.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/pack-objects.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
Show changes to builtin/pack-objects.c +16 −3
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f2365a4..12aa94c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -708,7 +708,7 @@ static struct object_entry **compute_write_order(void)
 static off_t write_reused_pack(struct sha1file *f)
 {
 	unsigned char buffer[8192];
-	off_t to_write;
+	off_t to_write, total;
 	int fd;
 
 	if (!is_pack_valid(reuse_packfile))
@@ -725,7 +725,7 @@ static off_t write_reused_pack(struct sha1file *f)
 	if (reuse_packfile_offset < 0)
 		reuse_packfile_offset = reuse_packfile->pack_size - 20;
 
-	to_write = reuse_packfile_offset - sizeof(struct pack_header);
+	total = to_write = reuse_packfile_offset - sizeof(struct pack_header);
 
 	while (to_write) {
 		int read_pack = xread(fd, buffer, sizeof(buffer));
@@ -738,10 +738,23 @@ static off_t write_reused_pack(struct sha1file *f)
 
 		sha1write(f, buffer, read_pack);
 		to_write -= read_pack;
+
+		/*
+		 * We don't know the actual number of objects written,
+		 * only how many bytes written, how many bytes total, and
+		 * how many objects total. So we can fake it by pretending all
+		 * objects we are writing are the same size. This gives us a
+		 * smooth progress meter, and at the end it matches the true
+		 * answer.
+		 */
+		written = reuse_packfile_objects *
+				(((double)(total - to_write)) / total);
+		display_progress(progress_state, written);
 	}
 
 	close(fd);
-	written += reuse_packfile_objects;
+	written = reuse_packfile_objects;
+	display_progress(progress_state, written);
 	return reuse_packfile_offset - sizeof(struct pack_header);
 }
 
-- 
1.9.0.417.gc6bea4f
Jeff King· Mar 15, 2014, 02:26 UTC · re: Jeff King · lore

[PATCH 2/2] pack-objects: show reused packfile objects in "Counting objects"

When we are sending a pack for push or fetch, we may reuse a chunk of packfile without even parsing it. The progress meter then looks like this:

  Reusing existing pack: 3440489, done.
  Counting objects: 3, done.

The first line shows that we are reusing a large chunk of objects, and then we further count any objects not included in the reused portion with an actual traversal.

These are all implementation details that the user does not need to care about. Instead, we can show the reused objects in the normal "counting..." progress meter (which will simply go much faster than normal), and then continue to add to it as we traverse.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/pack-objects.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)
Show changes to builtin/pack-objects.c +3 −8
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 12aa94c..4ca3946 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1038,7 +1038,7 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
 			    exclude, name && no_try_delta(name),
 			    index_pos, found_pack, found_offset);
 
-	display_progress(progress_state, to_pack.nr_objects);
+	display_progress(progress_state, nr_result);
 	return 1;
 }
 
@@ -1054,7 +1054,7 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,
 
 	create_object_entry(sha1, type, name_hash, 0, 0, index_pos, pack, offset);
 
-	display_progress(progress_state, to_pack.nr_objects);
+	display_progress(progress_state, nr_result);
 	return 1;
 }
 
@@ -2456,12 +2456,7 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
 			&reuse_packfile_offset)) {
 		assert(reuse_packfile_objects);
 		nr_result += reuse_packfile_objects;
-
-		if (progress) {
-			fprintf(stderr, "Reusing existing pack: %d, done.\n",
-				reuse_packfile_objects);
-			fflush(stderr);
-		}
+		display_progress(progress_state, nr_result);
 	}
 
 	traverse_bitmap_commit_list(&add_object_entry_from_bitmap);
-- 
1.9.0.417.gc6bea4f

← back to recent threads