From: Taylor Blau <me@ttaylorr.com>
Date: Tue, 09 Dec 2025 01:59:54 GMT
Subject: Re: [PATCH 07/17] midx-write.c: don't use `pack_perm` when assigning `bitmap_pos`
Message-ID: <aTeCmrLnRtYRm/ah@nand.local>
In-Reply-To: <aTcYU_yVYyXL9TXv@pks.im>

On Mon, Dec 08, 2025 at 07:26:27PM +0100, Patrick Steinhardt wrote:
> On Sat, Dec 06, 2025 at 03:31:19PM -0500, Taylor Blau wrote:
> > In midx_pack_order(), we compute for each bitampped pack the first bit
>
> s/bitampped/bitmapped/

Ugh. My "bitamp" typo strikes again, thanks for spotting!

> > to correspond to an object in that pack, along with how many bits were
> > assigned to object(s) in that pack.
> >
> > Initially, each bitmap_nr value is set to zero, and each bitmap_pos
>
> I assume `bitmap_nr` is the number of bits, whereas `bitmap_pos` is the
> position of the first bit?

That's right!

> > However, we enumerate the bitmapped packs in order of `ctx->pack_perm`.
>
> Which is the "permutation between pack-int-ids from the previous
> multi-pack-index to the new one we are writing"'. So it's basically
> tracking which new packs correspond to the old packs.

Ditto.

> So obviously, the permutation will only ever be different in case we've
> got at least one dropped pack, and that only happens when we expire any
> packs. So the explanation matches.
>
> Of course it may be a bit more fragile now if we ever added a caller
> of this function that _does_ expire data. But we don't have any, so that
> enters the territory of overthinking things.

I think that with incremental MIDXs we will never have such a caller
without a mechanism to tombstone objects in existing packs, but
definitely worth calling out.

> > diff --git a/midx-write.c b/midx-write.c
> > index 73d24fabbc6..c30f6a70d37 100644
> > --- a/midx-write.c
> > +++ b/midx-write.c
> > @@ -637,7 +637,7 @@ static uint32_t *midx_pack_order(struct write_midx_context *ctx)
> >  		pack_order[i] = data[i].nr;
> >  	}
> >  	for (i = 0; i < ctx->nr; i++) {
> > -		struct pack_info *pack = &ctx->info[ctx->pack_perm[i]];
> > +		struct pack_info *pack = &ctx->info[i];
> >  		if (pack->bitmap_pos == BITMAP_POS_UNKNOWN)
> >  			pack->bitmap_pos = 0;
> >  	}
>
> The change looks simple enough.

Yeah, I almost wonder if the commit message was more harmful than not.
The main points that I wanted to get across were:

 - Ultimately we want to enumerate a list, and there's no reason to do
   that in a permuted order.

 - Iterating in that permuted order is fine today because the array of
   values in ctx->pack_perm are always addressable indices into
   ctx->info.

 - That won't be the case in the future when we are combining packs from
   MIDX layers that have a non-zero m->num_packs_in_base, so adjusting
   the implementation now prevents us from running into that pitfall in
   such a future.

Let me know if you think that I should adjust the commit message here.
It's hard to know whether something resembling the above is better or
worse than the current version of the commit message from a reviewer's
perspective, so I'm happy to do whatever you think is cleaner ;-).

Thanks,
Taylor