Re: [PATCH 4/3] midx: inline nth_midxed_pack_entry()

[Jeff King <peff@peff.net>]

On Sat, Sep 11, 2021 at 10:31:34PM +0200, René Scharfe wrote:

> Am 11.09.21 um 19:07 schrieb Jeff King:
> > On Sat, Sep 11, 2021 at 06:08:42PM +0200, René Scharfe wrote:
> >
> >> @@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r,
> >>  	if (!is_pack_valid(p))
> >>  		return 0;
> >>
> >> -	nth_midxed_object_oid(&oid, m, pos);
> >> -	if (oidset_contains(&p->bad_objects, &oid))
> >> +	if (oidset_contains(&p->bad_objects, oid))
> >>  		return 0;
> >
> > So we get to avoid the nth_midxed_object_oid() copy entirely. Very nice.
> >
> > Compared to the code before your series, we still have an extra function
> > call to oidset_contains(), which will (in the common case) notice we
> > have no entries and immediately return. But I think that's getting into
> > pointless micro-optimization.
> 
> Right.  I measure a 0.5% slowdown for git multi-pack-index verify.  An
> inline oidset_size call avoids it.  That's easy enough to add, so let's
> have it!

I don't mind that, but I wonder if we can have our cake and eat it, too.

oidset_contains() is short, too, and could be inlined. Or if we're
worried about the size of the embedded kh_get_oid_set() getting inlined,
we could do something like:

  static inline int oidset_contains(const struct oidset *set, const
				    struct object_id *oid)
  {
	if (!oidset_size(set))
		return 0;
	return oidset_contains_func(set, oid);
  }

That saves callers from having to deal with it, at the expense of a
slightly complicated oidset implementation.

I guess it's an extra integer comparison for callers that _do_ expect to
have a non-empty set. So maybe it is better left to the caller to
decide whether to optimize in this way.

(A totally inline oidset_contains() avoids the extra check, but possibly
at the cost of larger code size).

-Peff