Re: [PATCH v3 12/13] pack-bitmap.c: use `ewah_or_iterator` for type bitmap iterators

On Fri, Feb 28, 2025 at 11:01:43AM +0100, Patrick Steinhardt wrote:

On Tue, Nov 19, 2024 at 05:07:53PM -0500, Taylor Blau wrote:

quoted

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 348488e2d9e..83696d834f6 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c

@@ -1622,25 +1622,29 @@ static void show_extended_objects(struct bitmap_index *bitmap_git,
 	}
 }

-static void init_type_iterator(struct ewah_iterator *it,
+static void init_type_iterator(struct ewah_or_iterator *it,
 			       struct bitmap_index *bitmap_git,
 			       enum object_type type)
 {
 	switch (type) {
 	case OBJ_COMMIT:
-		ewah_iterator_init(it, bitmap_git->commits);
+		ewah_or_iterator_init(it, bitmap_git->commits_all,
+				      bitmap_git->base_nr);
 		break;

 	case OBJ_TREE:
-		ewah_iterator_init(it, bitmap_git->trees);
+		ewah_or_iterator_init(it, bitmap_git->trees_all,
+				      bitmap_git->base_nr);
 		break;

 	case OBJ_BLOB:
-		ewah_iterator_init(it, bitmap_git->blobs);
+		ewah_or_iterator_init(it, bitmap_git->blobs_all,
+				      bitmap_git->base_nr);
 		break;

 	case OBJ_TAG:
-		ewah_iterator_init(it, bitmap_git->tags);
+		ewah_or_iterator_init(it, bitmap_git->tags_all,
+				      bitmap_git->base_nr);
 		break;

 	default:

One thing I wonder here is whether we want to continue using the
non-layered iterator in case we don't have an MIDX. But I guess it makes
it easier to not do it like that because we have less conditionals, and
overall the or'd logic shouldn't perform significantly workse anyway. So
as long as we always initialize `base_nr` and the relevant `*_all`
fields even in the non-MIDX case we're fine.

I had a similar thought when writing this and agree with you. We do set
up the _all iterators properly at the top of a chain (see the "if
(!recursing)" conditional inside of load_bitmap()).

Since we call that function for single-pack bitmaps as well (naturally
we are not recursing in that case), we'll call load_all_type_bitmaps()
in that case too.

Thanks,
Taylor