Re: [PATCH 4/5] pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuse

[Taylor Blau <me@ttaylorr.com>]

On Wed, Sep 04, 2024 at 11:54:18AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > @@ -2055,17 +2055,18 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,
> >  			     struct bitmapped_pack *pack,
> >  			     size_t bitmap_pos,
> >  			     uint32_t pack_pos,
> > +			     off_t offset,
> >  			     struct bitmap *reuse,
> >  			     struct pack_window **w_curs)
> >  {
> > -	off_t offset, delta_obj_offset;
> > +	off_t delta_obj_offset;
> >  	enum object_type type;
> >  	unsigned long size;
> >
> >  	if (pack_pos >= pack->p->num_objects)
> >  		return -1; /* not actually in the pack */
> >
> > -	offset = delta_obj_offset = pack_pos_to_offset(pack->p, pack_pos);
>
> We are now passing redundant piece of information "offset" that
> could be derived from two other parameters, which feels a bit icky,

Yeah, I agree. The only spot we use pack_pos in this function is in the
hunk above, and in the check where we reject any deltas whose base has a
greater than or equal to bit position than the delta object's bit
position.

But I think both of those are redundant. The only caller in
reuse_partial_packfile_from_bitmap_1() only passes bit positions that
are set, so we'll never trip the bounds check above. For the latter
check, it would suffice to compare object offsets in the single-pack
case, since the bit positions corresponding to objects in a single pack
are derived from the ordering of their actual offset.

>  - The logic to punt on a delta pointing backwards can be done by
>    comparing the base_offset and offset, instead of comparing the
>    base_pos and pack_pos.

Ah, yes -- exactly ;-). I should have read the email in its entirety
before starting to respond.

> but it may not be worth the effort, unless we are going to do
> similar clean-up globally in all the codepaths that access
> packfiles.

I'm not so sure we can do it in all cases, but at least in the case of
try_partial_reuse(), doing something like this (which compiles and
passes "make test") is fine:

--- 8< ---
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9d9b8c4bfb..fc51f298d5 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2054,7 +2054,6 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,
 static int try_partial_reuse(struct bitmap_index *bitmap_git,
 			     struct bitmapped_pack *pack,
 			     size_t bitmap_pos,
-			     uint32_t pack_pos,
 			     off_t offset,
 			     struct bitmap *reuse,
 			     struct pack_window **w_curs)
@@ -2063,9 +2062,6 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,
 	enum object_type type;
 	unsigned long size;

-	if (pack_pos >= pack->p->num_objects)
-		return -1; /* not actually in the pack */
-
 	delta_obj_offset = offset;
 	type = unpack_object_header(pack->p, w_curs, &offset, &size);
 	if (type < 0)
@@ -2121,8 +2117,13 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,
 			 * OFS_DELTA is the default). But let's double check to
 			 * make sure the pack wasn't written with odd
 			 * parameters.
+			 *
+			 * Since we're working on a single-pack bitmap, we can
+			 * use the object offset as a proxy for the bit
+			 * position, since the bits are ordered by their
+			 * positions within the pack.
 			 */
-			if (base_pos >= pack_pos)
+			if (base_offset >= offset)
 				return 0;
 			base_bitmap_pos = pack->bitmap_pos + base_pos;
 		}
@@ -2218,8 +2219,8 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git
 				ofs = pack_pos_to_offset(pack->p, pack_pos);
 			}

-			if (try_partial_reuse(bitmap_git, pack, bit_pos,
-					      pack_pos, ofs, reuse, &w_curs) < 0) {
+			if (try_partial_reuse(bitmap_git, pack, bit_pos, ofs,
+					      reuse, &w_curs) < 0) {
 				/*
 				 * try_partial_reuse indicated we couldn't reuse
 				 * any bits, so there is no point in trying more
--- >8 ---

Thanks,
Taylor