pack-bitmap-write: ignore BITMAP_FLAG_REUSE

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

Jeff King committed Dec 8, 2020 at 17:04 UTC 449fa5ee06906ca6d109e06b14cb4f8ea60a6c88
4 files changed +16 -87
builtin/pack-objects.c
-1
@@ -1104,7 +1104,6 @@ static void write_pack_file(void)
1104 stop_progress(&progress_state);
1105
1106 bitmap_writer_show_progress(progress);
1107 - bitmap_writer_reuse_bitmaps(&to_pack);
1107 bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1);
1108 bitmap_writer_build(&to_pack);
1109 bitmap_writer_finish(written_list, nr_written,
pack-bitmap-write.c
+5 -45
@@ -30,7 +30,6 @@ struct bitmap_writer {
30 struct ewah_bitmap *tags;
31
32 kh_oid_map_t *bitmaps;
33 - kh_oid_map_t *reused;
33 struct packing_data *to_pack;
34
35 struct bitmapped_commit *selected;
@@ -112,7 +111,7 @@ void bitmap_writer_build_type_index(struct packing_data *to_pack,
111 * Compute the actual bitmaps
112 */
113
115 -static inline void push_bitmapped_commit(struct commit *commit, struct ewah_bitmap *reused)
114 +static inline void push_bitmapped_commit(struct commit *commit)
115 {
116 if (writer.selected_nr >= writer.selected_alloc) {
117 writer.selected_alloc = (writer.selected_alloc + 32) * 2;
@@ -120,7 +119,7 @@ static inline void push_bitmapped_commit(struct commit *commit, struct ewah_bitm
119 }
120
121 writer.selected[writer.selected_nr].commit = commit;
123 - writer.selected[writer.selected_nr].bitmap = reused;
122 + writer.selected[writer.selected_nr].bitmap = NULL;
123 writer.selected[writer.selected_nr].flags = 0;
124
125 writer.selected_nr++;
@@ -372,13 +371,6 @@ static void store_selected(struct bb_commit *ent, struct commit *commit)
371 khiter_t hash_pos;
372 int hash_ret;
373
375 - /*
376 - * the "reuse bitmaps" phase may have stored something here, but
377 - * our new algorithm doesn't use it. Drop it.
378 - */
379 - if (stored->bitmap)
380 - ewah_free(stored->bitmap);
381 -
374 stored->bitmap = bitmap_to_ewah(ent->bitmap);
375
376 hash_pos = kh_put_oid_map(writer.bitmaps, commit->object.oid, &hash_ret);
@@ -480,35 +472,6 @@ static int date_compare(const void *_a, const void *_b)
472 return (long)b->date - (long)a->date;
473 }
474
483 -void bitmap_writer_reuse_bitmaps(struct packing_data *to_pack)
484 -{
485 - struct bitmap_index *bitmap_git;
486 - if (!(bitmap_git = prepare_bitmap_git(to_pack->repo)))
487 - return;
488 -
489 - writer.reused = kh_init_oid_map();
490 - rebuild_existing_bitmaps(bitmap_git, to_pack, writer.reused,
491 - writer.show_progress);
492 - /*
493 - * NEEDSWORK: rebuild_existing_bitmaps() makes writer.reused reference
494 - * some bitmaps in bitmap_git, so we can't free the latter.
495 - */
496 -}
497 -
498 -static struct ewah_bitmap *find_reused_bitmap(const struct object_id *oid)
499 -{
500 - khiter_t hash_pos;
501 -
502 - if (!writer.reused)
503 - return NULL;
504 -
505 - hash_pos = kh_get_oid_map(writer.reused, *oid);
506 - if (hash_pos >= kh_end(writer.reused))
507 - return NULL;
508 -
509 - return kh_value(writer.reused, hash_pos);
510 -}
511 -
475 void bitmap_writer_select_commits(struct commit **indexed_commits,
476 unsigned int indexed_commits_nr,
477 int max_bitmaps)
@@ -522,12 +485,11 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,
485
486 if (indexed_commits_nr < 100) {
487 for (i = 0; i < indexed_commits_nr; ++i)
525 - push_bitmapped_commit(indexed_commits[i], NULL);
488 + push_bitmapped_commit(indexed_commits[i]);
489 return;
490 }
491
492 for (;;) {
530 - struct ewah_bitmap *reused_bitmap = NULL;
493 struct commit *chosen = NULL;
494
495 next = next_commit_index(i);
@@ -542,15 +504,13 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,
504
505 if (next == 0) {
506 chosen = indexed_commits[i];
545 - reused_bitmap = find_reused_bitmap(&chosen->object.oid);
507 } else {
508 chosen = indexed_commits[i + next];
509
510 for (j = 0; j <= next; ++j) {
511 struct commit *cm = indexed_commits[i + j];
512
552 - reused_bitmap = find_reused_bitmap(&cm->object.oid);
553 - if (reused_bitmap || (cm->object.flags & NEEDS_BITMAP) != 0) {
513 + if ((cm->object.flags & NEEDS_BITMAP) != 0) {
514 chosen = cm;
515 break;
516 }
@@ -560,7 +520,7 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,
520 }
521 }
522
563 - push_bitmapped_commit(chosen, reused_bitmap);
523 + push_bitmapped_commit(chosen);
524
525 i += next + 1;
526 display_progress(writer.progress, i);
pack-bitmap.c
+6 -40
@@ -1338,9 +1338,9 @@ void test_bitmap_walk(struct rev_info *revs)
1338 free_bitmap_index(bitmap_git);
1339 }
1340
1341 -static int rebuild_bitmap(uint32_t *reposition,
1342 - struct ewah_bitmap *source,
1343 - struct bitmap *dest)
1341 +int rebuild_bitmap(const uint32_t *reposition,
1342 + struct ewah_bitmap *source,
1343 + struct bitmap *dest)
1344 {
1345 uint32_t pos = 0;
1346 struct ewah_iterator it;
@@ -1369,19 +1369,11 @@ static int rebuild_bitmap(uint32_t *reposition,
1369 return 0;
1370 }
1371
1372 -int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,
1373 - struct packing_data *mapping,
1374 - kh_oid_map_t *reused_bitmaps,
1375 - int show_progress)
1372 +uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,
1373 + struct packing_data *mapping)
1374 {
1375 uint32_t i, num_objects;
1376 uint32_t *reposition;
1379 - struct bitmap *rebuild;
1380 - struct stored_bitmap *stored;
1381 - struct progress *progress = NULL;
1382 -
1383 - khiter_t hash_pos;
1384 - int hash_ret;
1377
1378 num_objects = bitmap_git->pack->num_objects;
1379 reposition = xcalloc(num_objects, sizeof(uint32_t));
@@ -1399,33 +1391,7 @@ int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,
1391 reposition[i] = oe_in_pack_pos(mapping, oe) + 1;
1392 }
1393
1402 - rebuild = bitmap_new();
1403 - i = 0;
1404 -
1405 - if (show_progress)
1406 - progress = start_progress("Reusing bitmaps", 0);
1407 -
1408 - kh_foreach_value(bitmap_git->bitmaps, stored, {
1409 - if (stored->flags & BITMAP_FLAG_REUSE) {
1410 - if (!rebuild_bitmap(reposition,
1411 - lookup_stored_bitmap(stored),
1412 - rebuild)) {
1413 - hash_pos = kh_put_oid_map(reused_bitmaps,
1414 - stored->oid,
1415 - &hash_ret);
1416 - kh_value(reused_bitmaps, hash_pos) =
1417 - bitmap_to_ewah(rebuild);
1418 - }
1419 - bitmap_reset(rebuild);
1420 - display_progress(progress, ++i);
1421 - }
1422 - });
1423 -
1424 - stop_progress(&progress);
1425 -
1426 - free(reposition);
1427 - bitmap_free(rebuild);
1428 - return 0;
1394 + return reposition;
1395 }
1396
1397 void free_bitmap_index(struct bitmap_index *b)
pack-bitmap.h
+5 -1
@@ -73,7 +73,11 @@ void bitmap_writer_set_checksum(unsigned char *sha1);
73 void bitmap_writer_build_type_index(struct packing_data *to_pack,
74 struct pack_idx_entry **index,
75 uint32_t index_nr);
76 -void bitmap_writer_reuse_bitmaps(struct packing_data *to_pack);
76 +uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,
77 + struct packing_data *mapping);
78 +int rebuild_bitmap(const uint32_t *reposition,
79 + struct ewah_bitmap *source,
80 + struct bitmap *dest);
81 void bitmap_writer_select_commits(struct commit **indexed_commits,
82 unsigned int indexed_commits_nr, int max_bitmaps);
83 void bitmap_writer_build(struct packing_data *to_pack);