Re: [PATCH v2 20/20] pack-revindex.c: avoid direct revindex access in 'offset_to_pack_pos()'

From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, dstolee@microsoft.com, jrnieder@gmail.com,
	peff@peff.net
Subject: Re: [PATCH v2 20/20] pack-revindex.c: avoid direct revindex access in 'offset_to_pack_pos()'
Date: Wed, 13 Jan 2021 22:42:29 -0800	[thread overview]
Message-ID: <xmqqmtxcyq7e.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <8400ff6c9615b4c999b198c46b2e673ec0f2b14f.1610576604.git.me@ttaylorr.com> (Taylor Blau's message of "Wed, 13 Jan 2021 17:25:10 -0500")

Taylor Blau <me@ttaylorr.com> writes:

> To prepare for on-disk reverse indexes, remove a spot in
> 'offset_to_pack_pos()' that looks at the 'revindex' array in 'struct
> packed_git'.

Hmph, I somehow would have expected that this clean-up would be done
before step [18/20], but that does not matter in the end.  The end
result looks fairly clean.

I wonder if the call overhead to pack_pos_to_offset(), relative to
the direct indexing of an in-core array revindex[] followed by an
access to a member .offset that we used to do, makes a measurable
difference in this tight loop, though.

> diff --git a/pack-revindex.c b/pack-revindex.c
> index a508d5f0a4..5e69bc7372 100644
> --- a/pack-revindex.c
> +++ b/pack-revindex.c
> @@ -177,21 +177,21 @@ int load_pack_revindex(struct packed_git *p)
>  int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)
>  {
>  	unsigned lo, hi;
> -	const struct revindex_entry *revindex;
>  
>  	if (load_pack_revindex(p) < 0)
>  		return -1;
>  
>  	lo = 0;
>  	hi = p->num_objects + 1;
> -	revindex = p->revindex;
>  
>  	do {
>  		const unsigned mi = lo + (hi - lo) / 2;
> -		if (revindex[mi].offset == ofs) {
> +		off_t got = pack_pos_to_offset(p, mi);
> +
> +		if (got == ofs) {
>  			*pos = mi;
>  			return 0;
> -		} else if (ofs < revindex[mi].offset)
> +		} else if (ofs < got)
>  			hi = mi;
>  		else
>  			lo = mi + 1;