Re: [PATCH] object: fix performance regression when peeling tags

[Patrick Steinhardt <ps@pks.im>]

On Thu, Nov 06, 2025 at 06:33:44AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Bisecting the issue lands us at 6ec4c0b45b (refs: don't store peeled
> > object IDs for invalid tags, 2025-10-23). The gist of the commit is that
> > we may end up storing peeled objects in both reftables and packed-refs
> > for corrupted tags, where the claimed tagged object type is different
> > than the actual tagged object type. This will then cause us to create
> > the `struct object *` with a wrong type, as well, and obviously nothing
> > good comes out of that.
> 
> So does the flow of the logic, which led to the original "validation
> while recording peeled tags", go like this?
> 
>  - It is handy to be able to get peeled object cheaply, let's cache
>    it, because the same tag peels to the same object every time.
> 
>  - Usually when we see an object we make sure that is what we
>    expect.  Not having to do this validation costs us less, so why
>    not validate when caching peeled object?  It would amortise the
>    cost of validating the peeled object at runtime every time we use
>    it into a one-time cost when we cache.
> 
> But of course we do not really get rid of the type checking at
> runtime, so we certainly should notice, no?

Yeah, that's basically it. It's only the ref backends that cause us to
cache the peeled values, so the fix was to adapt them so that they know
to never cache data for corrupted tags. Otherwise we can end up in all
kinds of different situations where we may end up with an invalid
in-memory representation of the tagged object.

So the performance hit we have is on the writing side, and that should
amortize the cost somewhat. For the "files" backend that's mostly fine,
as we only pay that cost when packing refs. But for the "reftable"
backend it's less so because we basically pay the cost every time we
update a ref.

During runtime we (in most cases) don't verify the tagged object type.
We could, but that would make the added overhead that we now see as a
performance regression visible on every single read of a cached peeled
tag value. And I don't really think we should go there, doubly so
because the issue can only manifest in a corrupt repository.

One could of course argue that it's corrupt anyway, so why even bother
on the writing side? But such corrupted tags can be served to us by a
remote, and clients do not perform fsck by default. So it is possible
for such broken tags to end up in user repositories, and I'm not sure
about all the consequences it can have. So I'd rather put clients into a
position where they can at least detect the corruption at runtime, and
not caching the values is part of that mitigation.

Patrick