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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 6, 2025, 14:33 UTC
Message-ID: <xmqqy0ojjkmv.fsf@gitster.g>
In-Reply-To: <20251106-b4-pks-peel-object-performance-regression-v1-1-a386147750b0@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 7 quoted lines

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

Show 9 quoted lines

> Taking a step back though reveals an oddity in the new verification
> logic: we not only verify the _tagged_ object's type, but we also verify
> the type of the tag itself. But this isn't really needed, as we wouldn't
> hit the bug in such a case anyway, as we only hit the issue with corrupt
> tags claiming an invalid type for the tagged object.
>
> The consequence of this is that we now started to look up the target
> object of every single reference we're about to write, regardless of
> whether it even is a tag or not. And that is of course quite costly.

;-).

Show 6 quoted lines

> Fix the issue by only verifying the type of the tagged objects. This
> means that we of course still have a performance hit for actual tags.
> But this only happens for writes anyway, and I'd claim it's preferable
> to not store corrupted data in the refdb than to be fast here. Rename
> the flag accordingly to clarify that we only verify the tagged object's
> type.

OK.

Will queue.