Re: [PATCH 4/5] pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuse
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Sep 4, 2024, 19:28 UTC
- Message-ID
- <Zti0xrzCtpWScPjz@nand.local>
- In-Reply-To
- <xmqq1q1z9rmt.fsf@gitster.g>
On Wed, Sep 04, 2024 at 11:54:18AM -0700, Junio C Hamano wrote:
Show 22 quoted lines
> 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