Re: [PATCH 3/3] packfile: use oidset for bad objects

Am 11.09.21 um 19:03 schrieb Jeff King:

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

quoted

quoted

quoted

+	nth_midxed_object_oid(&oid, m, pos);
+	if (oidset_contains(&p->bad_objects, &oid))
+		return 0;

Calling nth_midxed_object_oid() implies a memcpy() under the hood. In
the old code, we'd skip that in the common case that we had no corrupt
objects, but now we'll pay the cost regardless. memcpy() isn't _that_
expensive, but I'd expect this to be a relatively hot code path.

Is it worth sticking all of this inside:

  if (oidset_size(&p->bad_objects))

?

Hard to say.  It would certainly match the old code more closely.  Is a
function call cheaper than copying 32 bytes?  Depends on the CPU and
whether the hash is cached, I guess.  And cached it probably is, because
the caller did a binary search for it..

You already have a function call for nth_midxed_object_oid(), so
checking oidset_size() would be a strict improvement.

If I read the assembly correctly nth_midxed_object_oid() is inlined by the
compiler in my build, as is nth_midxed_pack_entry().  Both are defined in
the same file, so other compilers may easily do the same.

quoted

We can pass on the original oid to avoid the nth_midxed_object_oid()
call, but inlining the whole thing might even be nicer.

Yeah, it occurs to me that oidset_size() would be a good candidate for
inlining, if that's what you mean.

True, but I meant something else (see patch 4/3). :)

René