Re: [PATCH v3 02/13] pack-revindex: prepare for incremental MIDX bitmaps

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 28, 2025, 23:39 UTC
Message-ID: <Z8JJG5EX1MUtGvss@nand.local>
In-Reply-To: <Z8GJaFPMQtslEEt3@pks.im>

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

Show 37 quoted lines

> On Tue, Nov 19, 2024 at 05:07:22PM -0500, Taylor Blau wrote:
> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index 4fa9dfc771a..bba9c6a905a 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -170,6 +170,15 @@ static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)
> >  	return read_bitmap(index->map, index->map_size, &index->map_pos);
> >  }
> >
> > +static uint32_t bitmap_non_extended_bits(struct bitmap_index *index)
> > +{
> > +	if (index->midx) {
> > +		struct multi_pack_index *m = index->midx;
> > +		return m->num_objects + m->num_objects_in_base;
> > +	}
> > +	return index->pack->num_objects;
> > +}
> >
> >  static uint32_t bitmap_num_objects(struct bitmap_index *index)
> >  {
> >  	if (index->midx)
>
> Okay, despite counting our own objects, we also need to account for any
> objects that the MIDX layer that we depend on may refer to. I assume
> that this is basically recursive, and that the base itself would also
> account for its next layer, if any.
>
> What is interesting to see after this commit is what callsites remain
> for `bitmap_num_objects()`. Most of them are converted, but some still
> exist:
>
>   - `load_bitmap_header()`, where we use it to determine the size of the
>     hash cache. Makes sense.
>
>   - `pseudo_merge_bitmap_for_commit()`, where we use it to compute the
>     merged bitmap of a specific commit. This one feels weird to me, I
>     would have expected to use `bitmap_non_extended_bits()` here.

Great question. The reason is that this function determines a bitmap which identifies the parents of a given commit, and that bitmap is compared against the set of pseudo-merge commits we know about in a given layer to determine whether or not we have a matching pseudo-merge.

I left a comment to that effect nearby since this is far from obvious (including to me -- I had to take a few minutes to remember how all of this works!).

>   - `filter_bitmap_blob_limit()`, where we seem to filter through the
>     bitmap of the current layer. I _think_ it makes sense to retain.
>
>   - `create_bitmap_mapping()`, which feels like it should be converted?

I *think* that this is OK because we are only remapping one layer at a time, but I'll have to double check. I'd do so now, but I'm trying to respond as much as I can before my week is over ;-).

> It would be nice to document in the commit message why those functions
> don't need to be converted to help guide the reader a bit.

The conversions here are case-by-case, so I lean towards documenting any non-obvious ones inline with a brief comment (like the adjustment I made above for pseudo_merge_bitmap_for_commit()).

Show 8 quoted lines

> > @@ -491,7 +499,8 @@ static int midx_pack_order_cmp(const void *va, const void *vb)
> >  	const struct midx_pack_key *key = va;
> >  	struct multi_pack_index *midx = key->midx;
> >
> > -	uint32_t versus = pack_pos_to_midx(midx, (uint32_t*)vb - (const uint32_t *)midx->revindex_data);
> > +	size_t pos = (uint32_t*)vb - (const uint32_t *)midx->revindex_data;
>
> Micronit: missing space between `uint32_t` and `*`.

Great catch, thanks!

Show 8 quoted lines

> > +	uint32_t versus = pack_pos_to_midx(midx, pos + midx->num_objects_in_base);
> >  	uint32_t versus_pack = nth_midxed_pack_int_id(midx, versus);
> >  	off_t versus_offset;
> >
>
> Okay, the calculation to calculate the position is basically the same,
> but we now also offset the position by the number of objects in
> preceding layers. Makes sense.

Exactly.

Thanks, Taylor