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

[Jeff King <peff@peff.net>]

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

> >> +	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.

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

-Peff