[v2,19/24] pack-bitmap-write: ignore BITMAP_FLAG_REUSE

Commit Message

Taylor Blau Nov. 17, 2020, 9:48 p.m. UTC

From: Jeff King <[e-mail]>

The on-disk bitmap format has a flag to mark a bitmap to be "reused".
This is a rather curious feature, and works like this:

  - a run of pack-objects would decide to mark the last 80% of the
    bitmaps it generates with the reuse flag

  - the next time we generate bitmaps, we'd see those reuse flags from
    the last run, and mark those commits as special:

      - we'd be more likely to select those commits to get bitmaps in
        the new output

      - when generating the bitmap for a selected commit, we'd reuse the
        old bitmap as-is (rearranging the bits to match the new pack, of
        course)

However, neither of these behaviors particularly makes sense.

Just because a commit happened to be bitmapped last time does not make
it a good candidate for having a bitmap this time. In particular, we may
choose bitmaps based on how recent they are in history, or whether a ref
tip points to them, and those things will change. We're better off
re-considering fresh which commits are good candidates.

Reusing the existing bitmap _is_ a reasonable thing to do to save
computation. But only reusing exact bitmaps is a weak form of this. If
we have an old bitmap for A and now want a new bitmap for its child, we
should be able to compute that only by looking at trees and that are new
to the child. But this code would consider only exact reuse (which is
perhaps why it was eager to select those commits in the first place).

Furthermore, the recent switch to the reverse-edge algorithm for
generating bitmaps dropped this optimization entirely (and yet still
performs better).

So let's do a few cleanups:

 - drop the whole "reusing bitmaps" phase of generating bitmaps. It's
   not helping anything, and is mostly unused code (or worse, code that
   is using CPU but not doing anything useful)

 - drop the use of the on-disk reuse flag to select commits to bitmap

 - stop setting the on-disk reuse flag in bitmaps we generate (since
   nothing respects it anymore)

We will keep a few innards of the reuse code, which will help us
implement a more capable version of the "reuse" optimization:

 - simplify rebuild_existing_bitmaps() into a function that only builds
   the mapping of bits between the old and new orders, but doesn't
   actually convert any bitmaps

 - make rebuild_bitmap() public; we'll call it lazily to convert bitmaps
   as we traverse (using the mapping created above)

Signed-off-by: Jeff King <[e-mail]>
Signed-off-by: Taylor Blau <[e-mail]>
---
 builtin/pack-objects.c |  1 -
 pack-bitmap-write.c    | 50 +++++-------------------------------------
 pack-bitmap.c          | 46 +++++---------------------------------
 pack-bitmap.h          |  6 ++++-
 4 files changed, 16 insertions(+), 87 deletions(-)

Comments

Jonathan Tan Dec. 2, 2020, 7:13 a.m. UTC | #1

> Just because a commit happened to be bitmapped last time does not make
> it a good candidate for having a bitmap this time. In particular, we may
> choose bitmaps based on how recent they are in history, or whether a ref
> tip points to them, and those things will change. We're better off
> re-considering fresh which commits are good candidates.
> 
> Reusing the existing bitmap _is_ a reasonable thing to do to save
> computation. But only reusing exact bitmaps is a weak form of this. If
> we have an old bitmap for A and now want a new bitmap for its child, we
> should be able to compute that only by looking at trees and that are new
> to the child. But this code would consider only exact reuse (which is
> perhaps why it was eager to select those commits in the first place).

Makes sense.

> -int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,
> -			     struct packing_data *mapping,
> -			     kh_oid_map_t *reused_bitmaps,
> -			     int show_progress)
> +uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,
> +				struct packing_data *mapping)

[snip body of function]

Here, a lot of the function is deleted, and only the part that creates
the mapping from old indices to new indices remains - hence, the
renaming of the function. OK.

Overall this looks good. I was wondering if there would be any functions
now unused, but looking at the deleted lines, that doesn't seem to be
the case.

Patch

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 5617c01b5a..2a00358f34 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1104,7 +1104,6 @@  static void write_pack_file(void)
 				stop_progress(&progress_state);
 
 				bitmap_writer_show_progress(progress);
-				bitmap_writer_reuse_bitmaps(&to_pack);
 				bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1);
 				bitmap_writer_build(&to_pack);
 				bitmap_writer_finish(written_list, nr_written,
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 7b4fc0f304..1995f75818 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -30,7 +30,6 @@  struct bitmap_writer {
 	struct ewah_bitmap *tags;
 
 	kh_oid_map_t *bitmaps;
-	kh_oid_map_t *reused;
 	struct packing_data *to_pack;
 
 	struct bitmapped_commit *selected;
@@ -112,7 +111,7 @@  void bitmap_writer_build_type_index(struct packing_data *to_pack,
  * Compute the actual bitmaps
  */
 
-static inline void push_bitmapped_commit(struct commit *commit, struct ewah_bitmap *reused)
+static inline void push_bitmapped_commit(struct commit *commit)
 {
 	if (writer.selected_nr >= writer.selected_alloc) {
 		writer.selected_alloc = (writer.selected_alloc + 32) * 2;
@@ -120,7 +119,7 @@  static inline void push_bitmapped_commit(struct commit *commit, struct ewah_bitm
 	}
 
 	writer.selected[writer.selected_nr].commit = commit;
-	writer.selected[writer.selected_nr].bitmap = reused;
+	writer.selected[writer.selected_nr].bitmap = NULL;
 	writer.selected[writer.selected_nr].flags = 0;
 
 	writer.selected_nr++;
@@ -372,13 +371,6 @@  static void store_selected(struct bb_commit *ent, struct commit *commit)
 	khiter_t hash_pos;
 	int hash_ret;
 
-	/*
-	 * the "reuse bitmaps" phase may have stored something here, but
-	 * our new algorithm doesn't use it. Drop it.
-	 */
-	if (stored->bitmap)
-		ewah_free(stored->bitmap);
-
 	stored->bitmap = bitmap_to_ewah(ent->bitmap);
 
 	hash_pos = kh_put_oid_map(writer.bitmaps, commit->object.oid, &hash_ret);
@@ -477,35 +469,6 @@  static int date_compare(const void *_a, const void *_b)
 	return (long)b->date - (long)a->date;
 }
 
-void bitmap_writer_reuse_bitmaps(struct packing_data *to_pack)
-{
-	struct bitmap_index *bitmap_git;
-	if (!(bitmap_git = prepare_bitmap_git(to_pack->repo)))
-		return;
-
-	writer.reused = kh_init_oid_map();
-	rebuild_existing_bitmaps(bitmap_git, to_pack, writer.reused,
-				 writer.show_progress);
-	/*
-	 * NEEDSWORK: rebuild_existing_bitmaps() makes writer.reused reference
-	 * some bitmaps in bitmap_git, so we can't free the latter.
-	 */
-}
-
-static struct ewah_bitmap *find_reused_bitmap(const struct object_id *oid)
-{
-	khiter_t hash_pos;
-
-	if (!writer.reused)
-		return NULL;
-
-	hash_pos = kh_get_oid_map(writer.reused, *oid);
-	if (hash_pos >= kh_end(writer.reused))
-		return NULL;
-
-	return kh_value(writer.reused, hash_pos);
-}
-
 void bitmap_writer_select_commits(struct commit **indexed_commits,
 				  unsigned int indexed_commits_nr,
 				  int max_bitmaps)
@@ -519,12 +482,11 @@  void bitmap_writer_select_commits(struct commit **indexed_commits,
 
 	if (indexed_commits_nr < 100) {
 		for (i = 0; i < indexed_commits_nr; ++i)
-			push_bitmapped_commit(indexed_commits[i], NULL);
+			push_bitmapped_commit(indexed_commits[i]);
 		return;
 	}
 
 	for (;;) {
-		struct ewah_bitmap *reused_bitmap = NULL;
 		struct commit *chosen = NULL;
 
 		next = next_commit_index(i);
@@ -539,15 +501,13 @@  void bitmap_writer_select_commits(struct commit **indexed_commits,
 
 		if (next == 0) {
 			chosen = indexed_commits[i];
-			reused_bitmap = find_reused_bitmap(&chosen->object.oid);
 		} else {
 			chosen = indexed_commits[i + next];
 
 			for (j = 0; j <= next; ++j) {
 				struct commit *cm = indexed_commits[i + j];
 
-				reused_bitmap = find_reused_bitmap(&cm->object.oid);
-				if (reused_bitmap || (cm->object.flags & NEEDS_BITMAP) != 0) {
+				if ((cm->object.flags & NEEDS_BITMAP) != 0) {
 					chosen = cm;
 					break;
 				}
@@ -557,7 +517,7 @@  void bitmap_writer_select_commits(struct commit **indexed_commits,
 			}
 		}
 
-		push_bitmapped_commit(chosen, reused_bitmap);
+		push_bitmapped_commit(chosen);
 
 		i += next + 1;
 		display_progress(writer.progress, i);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 60c781d100..d1368b69bb 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1338,9 +1338,9 @@  void test_bitmap_walk(struct rev_info *revs)
 	free_bitmap_index(bitmap_git);
 }
 
-static int rebuild_bitmap(uint32_t *reposition,
-			  struct ewah_bitmap *source,
-			  struct bitmap *dest)
+int rebuild_bitmap(const uint32_t *reposition,
+		   struct ewah_bitmap *source,
+		   struct bitmap *dest)
 {
 	uint32_t pos = 0;
 	struct ewah_iterator it;
@@ -1369,19 +1369,11 @@  static int rebuild_bitmap(uint32_t *reposition,
 	return 0;
 }
 
-int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,
-			     struct packing_data *mapping,
-			     kh_oid_map_t *reused_bitmaps,
-			     int show_progress)
+uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,
+				struct packing_data *mapping)
 {
 	uint32_t i, num_objects;
 	uint32_t *reposition;
-	struct bitmap *rebuild;
-	struct stored_bitmap *stored;
-	struct progress *progress = NULL;
-
-	khiter_t hash_pos;
-	int hash_ret;
 
 	num_objects = bitmap_git->pack->num_objects;
 	reposition = xcalloc(num_objects, sizeof(uint32_t));
@@ -1399,33 +1391,7 @@  int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,
 			reposition[i] = oe_in_pack_pos(mapping, oe) + 1;
 	}
 
-	rebuild = bitmap_new();
-	i = 0;
-
-	if (show_progress)
-		progress = start_progress("Reusing bitmaps", 0);
-
-	kh_foreach_value(bitmap_git->bitmaps, stored, {
-		if (stored->flags & BITMAP_FLAG_REUSE) {
-			if (!rebuild_bitmap(reposition,
-					    lookup_stored_bitmap(stored),
-					    rebuild)) {
-				hash_pos = kh_put_oid_map(reused_bitmaps,
-							  stored->oid,
-							  &hash_ret);
-				kh_value(reused_bitmaps, hash_pos) =
-					bitmap_to_ewah(rebuild);
-			}
-			bitmap_reset(rebuild);
-			display_progress(progress, ++i);
-		}
-	});
-
-	stop_progress(&progress);
-
-	free(reposition);
-	bitmap_free(rebuild);
-	return 0;
+	return reposition;
 }
 
 void free_bitmap_index(struct bitmap_index *b)
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 1203120c43..afa4115136 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -73,7 +73,11 @@  void bitmap_writer_set_checksum(unsigned char *sha1);
 void bitmap_writer_build_type_index(struct packing_data *to_pack,
 				    struct pack_idx_entry **index,
 				    uint32_t index_nr);
-void bitmap_writer_reuse_bitmaps(struct packing_data *to_pack);
+uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,
+				struct packing_data *mapping);
+int rebuild_bitmap(const uint32_t *reposition,
+		   struct ewah_bitmap *source,
+		   struct bitmap *dest);
 void bitmap_writer_select_commits(struct commit **indexed_commits,
 		unsigned int indexed_commits_nr, int max_bitmaps);
 void bitmap_writer_build(struct packing_data *to_pack);