On Fri, Feb 28, 2025 at 11:01:19AM +0100, Patrick Steinhardt wrote:
> On Tue, Nov 19, 2024 at 05:07:29PM -0500, Taylor Blau wrote:
> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index 41675a69f68..e3fdcf8a01a 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -946,18 +946,21 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_
> > struct ewah_bitmap *bitmap_for_commit(struct bitmap_index *bitmap_git,
> > struct commit *commit)
> > {
> > - khiter_t hash_pos = kh_get_oid_map(bitmap_git->bitmaps,
> > - commit->object.oid);
> > + khiter_t hash_pos;
> > + if (!bitmap_git)
> > + return NULL;
> > +
> > + hash_pos = kh_get_oid_map(bitmap_git->bitmaps, commit->object.oid);
> > if (hash_pos >= kh_end(bitmap_git->bitmaps)) {
> > struct stored_bitmap *bitmap = NULL;
> > if (!bitmap_git->table_lookup)
> > - return NULL;
> > + return bitmap_for_commit(bitmap_git->base, commit);
> >
> > /* this is a fairly hot codepath - no trace2_region please */
> > /* NEEDSWORK: cache misses aren't recorded */
> > bitmap = lazy_bitmap_for_commit(bitmap_git, commit);
> > if (!bitmap)
> > - return NULL;
> > + return bitmap_for_commit(bitmap_git->base, commit);
> > return lookup_stored_bitmap(bitmap);
> > }
> > return lookup_stored_bitmap(kh_value(bitmap_git->bitmaps, hash_pos));
>
> One of the things that worries me a bit is that by recursing, we
> essentially are bound in the depth of MIDX layers as we may otherwise
> bust the stack. Not that I expect us to typically have thousands of
> layers, but if there ever was a bug this may fail in bad ways.
I think it's a valid concern, but in practice I suspect we are unlikely
to run into it. If we have enough MIDX layers to blow the stack, we
probably have much bigger problems to worry about ;-).
> I already asked this for a previous commit, but what is the current
> state regarding compaction of the layers? Do we need to be worried about
> this or do we already know to keep things limited in general?
I mentioned upthread, but briefly: we don't compact MIDX layers today,
but the design is such that it is possible (and planned) to do in the
future (part three of this multi-series thing).
Thanks,
Taylor
