Re: [PATCH v3 12/13] pack-bitmap.c: use `ewah_or_iterator` for type bitmap iterators
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Mar 1, 2025, 00:28 UTC
- Message-ID
- <Z8JUwZFZ3yAv+AV2@nand.local>
- In-Reply-To
- <Z8GJhzdUtH5IB1Pr@pks.im>
On Fri, Feb 28, 2025 at 11:01:43AM +0100, Patrick Steinhardt wrote:
Show 47 quoted lines
> 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