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

[Taylor Blau <me@ttaylorr.com>]

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:
> > 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