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

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

Am 11.09.21 um 19:07 schrieb Jeff King:

quoted

On Sat, Sep 11, 2021 at 06:08:42PM +0200, René Scharfe wrote:

quoted

@@ -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