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
