{"thread":{"id":"64449","subject":"[PATCH] object: fix performance regression when peeling tags","startedAt":"2025-11-06T08:53:08Z","lastAt":"2025-11-07T06:19:28Z","messageCount":3,"participants":["Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"530302","messageId":"20251106-b4-pks-peel-object-performance-regression-v1-1-a386147750b0@pks.im","threadId":"64449","inReplyTo":null,"subject":"[PATCH] object: fix performance regression when peeling tags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-06T08:52:54Z","receivedAt":"2025-11-06T08:53:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Our Bencher dashboards [1] have recently alerted us about a bunch of\nperformance regressions when writing references, specifically with the\nreftable backend. There is a 3x regression when writing many refs with\npreexisting refs in the reftable format, and a 10x regression when\nmigrating refs between backends in either of the formats.\n\nBisecting the issue lands us at 6ec4c0b45b (refs: don't store peeled\nobject IDs for invalid tags, 2025-10-23). The gist of the commit is that\nwe may end up storing peeled objects in both reftables and packed-refs\nfor corrupted tags, where the claimed tagged object type is different\nthan the actual tagged object type. This will then cause us to create\nthe `struct object *` with a wrong type, as well, and obviously nothing\ngood comes out of that.\n\nThe fix for this issue was to introduce a new flag to `peel_object()`\nthat causes us to verify the tagged object's type before writing it into\nthe refdb -- if the tag is corrupt, we skip writing the peeled value.\nTo verify whether the peeled value is correct we have to look up the\nobject type via the ODB and compare the actual type with the claimed\ntype, and that additional object lookup is costly.\n\nThis also explains why we see the regression only when writing refs with\nthe reftable backend, but we see the regression with both backends when\nmigrating refs:\n\n  - The reftable backend knows to store peeled values in the new table\n    immediately, so it has to try and peel each ref it's about to write\n    to the transaction. So the performance regression is visible for all\n    writes.\n\n  - The files backend only stores peeled values when writing the\n    packed-refs file, so it wouldn't hit the performance regression for\n    normal writes. But on ref migrations we know to write all new values\n    into the packed-refs file immediately, and that's why we see the\n    regression for both backends there.\n\nTaking a step back though reveals an oddity in the new verification\nlogic: we not only verify the _tagged_ object's type, but we also verify\nthe type of the tag itself. But this isn't really needed, as we wouldn't\nhit the bug in such a case anyway, as we only hit the issue with corrupt\ntags claiming an invalid type for the tagged object.\n\nThe consequence of this is that we now started to look up the target\nobject of every single reference we're about to write, regardless of\nwhether it even is a tag or not. And that is of course quite costly.\n\nFix the issue by only verifying the type of the tagged objects. This\nmeans that we of course still have a performance hit for actual tags.\nBut this only happens for writes anyway, and I'd claim it's preferable\nto not store corrupted data in the refdb than to be fast here. Rename\nthe flag accordingly to clarify that we only verify the tagged object's\ntype.\n\nThis fix brings performance back to previous levels:\n\n    Benchmark 1: baseline\n      Time (mean ± σ):      46.0 ms ±   0.4 ms    [User: 40.0 ms, System: 5.7 ms]\n      Range (min … max):    45.0 ms …  47.1 ms    54 runs\n\n    Benchmark 2: regression\n      Time (mean ± σ):     140.2 ms ±   1.3 ms    [User: 77.5 ms, System: 60.5 ms]\n      Range (min … max):   138.0 ms … 142.7 ms    20 runs\n\n    Benchmark 3: fix\n      Time (mean ± σ):      46.2 ms ±   0.4 ms    [User: 40.2 ms, System: 5.7 ms]\n      Range (min … max):    45.0 ms …  47.3 ms    55 runs\n\n    Summary\n      update-ref: baseline\n        1.00 ± 0.01 times faster than fix\n        3.05 ± 0.04 times faster than regression\n\n[1]: https://bencher.dev/perf/git/plots\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nPatrick\n---\n object.c                |  4 ++--\n object.h                | 12 ++++++------\n ref-filter.c            |  2 +-\n refs/packed-backend.c   |  2 +-\n refs/reftable-backend.c |  2 +-\n 5 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex e72b0ed436..b08fc7a163 100644\n--- a/object.c\n+++ b/object.c\n@@ -214,7 +214,7 @@ enum peel_status peel_object(struct repository *r,\n {\n \tstruct object *o = lookup_unknown_object(r, name);\n \n-\tif (o->type == OBJ_NONE || flags & PEEL_OBJECT_VERIFY_OBJECT_TYPE) {\n+\tif (o->type == OBJ_NONE) {\n \t\tint type = odb_read_object_info(r->objects, name, NULL);\n \t\tif (type < 0 || !object_as_type(o, type, 0))\n \t\t\treturn PEEL_INVALID;\n@@ -228,7 +228,7 @@ enum peel_status peel_object(struct repository *r,\n \t\tif (o && o->type == OBJ_TAG && ((struct tag *)o)->tagged) {\n \t\t\to = ((struct tag *)o)->tagged;\n \n-\t\t\tif (flags & PEEL_OBJECT_VERIFY_OBJECT_TYPE) {\n+\t\t\tif (flags & PEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE) {\n \t\t\t\tint type = odb_read_object_info(r->objects, &o->oid, NULL);\n \t\t\t\tif (type < 0 || !object_as_type(o, type, 0))\n \t\t\t\t\treturn PEEL_INVALID;\ndiff --git a/object.h b/object.h\nindex 1499f63d50..e9baade1e0 100644\n--- a/object.h\n+++ b/object.h\n@@ -289,13 +289,13 @@ enum peel_status {\n \n enum peel_object_flags {\n \t/*\n-\t * Always verify the object type, even in the case where the looked-up\n-\t * object already has an object type. This can be useful when the\n-\t * stored object type may be invalid. One such case is when looking up\n-\t * objects via tags, where we blindly trust the object type declared by\n-\t * the tag.\n+\t * Always verify the object type of the tagged object, even in the case\n+\t * where the looked-up object already has an object type. This can be\n+\t * useful when the tagged object type may be invalid. One such case is\n+\t * when looking up objects via tags, where we blindly trust the object\n+\t * type declared by the tag.\n \t */\n-\tPEEL_OBJECT_VERIFY_OBJECT_TYPE = (1 << 0),\n+\tPEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE = (1 << 0),\n };\n \n /*\ndiff --git a/ref-filter.c b/ref-filter.c\nindex d8667c569a..d7454269e8 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2654,7 +2654,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\tif (!is_null_oid(&ref->peeled_oid)) {\n \t\t\toidcpy(&oi_deref.oid, &ref->peeled_oid);\n \t\t} else if (!peel_object(the_repository, &oi.oid, &oi_deref.oid,\n-\t\t\t\t\tPEEL_OBJECT_VERIFY_OBJECT_TYPE)) {\n+\t\t\t\t\tPEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE)) {\n \t\t\t/* We managed to peel the object ourselves. */\n \t\t} else {\n \t\t\tdie(\"bad tag\");\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 1ab0c50393..5aa615011a 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1528,7 +1528,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n \t\t} else {\n \t\t\tstruct object_id peeled;\n \t\t\tint peel_error = peel_object(refs->base.repo, &update->new_oid,\n-\t\t\t\t\t\t     &peeled, PEEL_OBJECT_VERIFY_OBJECT_TYPE);\n+\t\t\t\t\t\t     &peeled, PEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE);\n \n \t\t\tif (write_packed_entry(out, update->refname,\n \t\t\t\t\t       &update->new_oid,\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 6bbfd5618d..1ac1f6156f 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1633,7 +1633,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\t\tref.update_index = ts;\n \n \t\t\tpeel_error = peel_object(arg->refs->base.repo, &u->new_oid, &peeled,\n-\t\t\t\t\t\t PEEL_OBJECT_VERIFY_OBJECT_TYPE);\n+\t\t\t\t\t\t PEEL_OBJECT_VERIFY_TAGGED_OBJECT_TYPE);\n \t\t\tif (!peel_error) {\n \t\t\t\tref.value_type = REFTABLE_REF_VAL2;\n \t\t\t\tmemcpy(ref.value.val2.target_value, peeled.hash, GIT_MAX_RAWSZ);\n\n---\nbase-commit: ad5e7227123df84edc0cc82d18c5962cd9983b85\nchange-id: 20251106-b4-pks-peel-object-performance-regression-70148db5da9e\n\n"},{"id":"530318","messageId":"xmqqy0ojjkmv.fsf@gitster.g","threadId":"64449","inReplyTo":"20251106-b4-pks-peel-object-performance-regression-v1-1-a386147750b0@pks.im","subject":"Re: [PATCH] object: fix performance regression when peeling tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-06T14:33:44Z","receivedAt":"2025-11-06T14:33:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Bisecting the issue lands us at 6ec4c0b45b (refs: don't store peeled\n> object IDs for invalid tags, 2025-10-23). The gist of the commit is that\n> we may end up storing peeled objects in both reftables and packed-refs\n> for corrupted tags, where the claimed tagged object type is different\n> than the actual tagged object type. This will then cause us to create\n> the `struct object *` with a wrong type, as well, and obviously nothing\n> good comes out of that.\n\nSo does the flow of the logic, which led to the original \"validation\nwhile recording peeled tags\", go like this?\n\n - It is handy to be able to get peeled object cheaply, let's cache\n   it, because the same tag peels to the same object every time.\n\n - Usually when we see an object we make sure that is what we\n   expect.  Not having to do this validation costs us less, so why\n   not validate when caching peeled object?  It would amortise the\n   cost of validating the peeled object at runtime every time we use\n   it into a one-time cost when we cache.\n\nBut of course we do not really get rid of the type checking at\nruntime, so we certainly should notice, no?\n\n> Taking a step back though reveals an oddity in the new verification\n> logic: we not only verify the _tagged_ object's type, but we also verify\n> the type of the tag itself. But this isn't really needed, as we wouldn't\n> hit the bug in such a case anyway, as we only hit the issue with corrupt\n> tags claiming an invalid type for the tagged object.\n>\n> The consequence of this is that we now started to look up the target\n> object of every single reference we're about to write, regardless of\n> whether it even is a tag or not. And that is of course quite costly.\n\n;-).\n\n> Fix the issue by only verifying the type of the tagged objects. This\n> means that we of course still have a performance hit for actual tags.\n> But this only happens for writes anyway, and I'd claim it's preferable\n> to not store corrupted data in the refdb than to be fast here. Rename\n> the flag accordingly to clarify that we only verify the tagged object's\n> type.\n\nOK.\n\nWill queue.\n"},{"id":"530356","messageId":"aQ2Paa7pDTtbTRpC@pks.im","threadId":"64449","inReplyTo":"xmqqy0ojjkmv.fsf@gitster.g","subject":"Re: [PATCH] object: fix performance regression when peeling tags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-07T06:19:21Z","receivedAt":"2025-11-07T06:19:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 06, 2025 at 06:33:44AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Bisecting the issue lands us at 6ec4c0b45b (refs: don't store peeled\n> > object IDs for invalid tags, 2025-10-23). The gist of the commit is that\n> > we may end up storing peeled objects in both reftables and packed-refs\n> > for corrupted tags, where the claimed tagged object type is different\n> > than the actual tagged object type. This will then cause us to create\n> > the `struct object *` with a wrong type, as well, and obviously nothing\n> > good comes out of that.\n> \n> So does the flow of the logic, which led to the original \"validation\n> while recording peeled tags\", go like this?\n> \n>  - It is handy to be able to get peeled object cheaply, let's cache\n>    it, because the same tag peels to the same object every time.\n> \n>  - Usually when we see an object we make sure that is what we\n>    expect.  Not having to do this validation costs us less, so why\n>    not validate when caching peeled object?  It would amortise the\n>    cost of validating the peeled object at runtime every time we use\n>    it into a one-time cost when we cache.\n> \n> But of course we do not really get rid of the type checking at\n> runtime, so we certainly should notice, no?\n\nYeah, that's basically it. It's only the ref backends that cause us to\ncache the peeled values, so the fix was to adapt them so that they know\nto never cache data for corrupted tags. Otherwise we can end up in all\nkinds of different situations where we may end up with an invalid\nin-memory representation of the tagged object.\n\nSo the performance hit we have is on the writing side, and that should\namortize the cost somewhat. For the \"files\" backend that's mostly fine,\nas we only pay that cost when packing refs. But for the \"reftable\"\nbackend it's less so because we basically pay the cost every time we\nupdate a ref.\n\nDuring runtime we (in most cases) don't verify the tagged object type.\nWe could, but that would make the added overhead that we now see as a\nperformance regression visible on every single read of a cached peeled\ntag value. And I don't really think we should go there, doubly so\nbecause the issue can only manifest in a corrupt repository.\n\nOne could of course argue that it's corrupt anyway, so why even bother\non the writing side? But such corrupted tags can be served to us by a\nremote, and clients do not perform fsck by default. So it is possible\nfor such broken tags to end up in user repositories, and I'm not sure\nabout all the consequences it can have. So I'd rather put clients into a\nposition where they can at least detect the corruption at runtime, and\nnot caching the values is part of that mitigation.\n\nPatrick\n"}]}