{"thread":{"id":"55017","subject":"Re: [PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","startedAt":"2021-01-20T15:40:12Z","lastAt":"2021-01-22T17:27:58Z","messageCount":11,"participants":["Taylor Blau","Jeff King","Jacob Vosmaer"],"isPatch":true,"patchVersion":2,"patchTotal":1},"messages":[{"id":"414802","messageId":"YAhC8Gsp4H17e28n@nand.local","threadId":"55017","inReplyTo":"20210120124514.49737-2-jacob@gitlab.com","subject":"Re: [PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-20T14:49:20Z","receivedAt":"2021-01-20T15:40:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Jacob,\n\nOn Wed, Jan 20, 2021 at 01:45:14PM +0100, Jacob Vosmaer wrote:\n> In git-pack-objects, we iterate over all the tags if the --include-tag\n> option is passed on the command line. For some reason this uses\n> for_each_ref which is expensive if the repo has many refs. We should\n> use for_each_tag_ref instead.\n\nI don't think it's worth sending another version, but I would have liked\nto see: \"... because we can save time by only iterating over some of the\nrefs\" at the end of this paragraph.\n\n> Because the add_ref_tag callback will now only visit tags we\n> simplified it a bit.\n>\n> The motivation for this change is that we observed performance issues\n> with a repository on gitlab.com that has 500,000 refs but only 2,000\n> tags. The fetch traffic on that repo is dominated by CI, and when we\n> changed CI to fetch with 'git fetch --no-tags' we saw a dramatic\n> change in the CPU profile of git-pack-objects. This lead us to this\n> particular ref walk. More details in:\n> https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/746#note_483546598\n>\n> Signed-off-by: Jacob Vosmaer \n> ---\n> builtin/pack-objects.c | 8 +++-----\n> 1 file changed, 3 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 2a00358f34..ad52c91bdb 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -2803,13 +2803,11 @@ static void add_tag_chain(const struct object_id *oid)\n> \t}\n> }\n>\n> -static int add_ref_tag(const char *path, const struct object_id *oid, int flag, void *cb_data)\n> +static int add_ref_tag(const char *tag, const struct object_id *oid, int flag, void *cb_data)\n> {\n> \tstruct object_id peeled;\n>\n> -\tif (starts_with(path, \"refs/tags/\") && /* is a tag? */\n> -\t !peel_ref(path, &peeled) && /* peelable? */\n> -\t obj_is_packed(&peeled)) /* object packed? */\n> +\tif (!peel_ref(tag, &peeled) && obj_is_packed(&peeled))\n> \t\tadd_tag_chain(oid);\n> \treturn 0;\n> }\n> @@ -3740,7 +3738,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t}\n> \tcleanup_preferred_base();\n> \tif (include_tag && nr_result)\n> -\t\tfor_each_ref(add_ref_tag, NULL);\n> +\t\tfor_each_tag_ref(add_ref_tag, NULL);\n\nOK. Seeing another caller (builtin/pack-objects.c:compute_write_order())\nthat passes a callback to for_each_tag_ref() makes me feel more\ncomfortable about using it here.\n\nThanks for investigating and resolving this in a way which cleans up the\nsurrounding code.\n\n> \tstop_progress(&progress_state);\n> \ttrace2_region_leave(\"pack-objects\", \"enumerate-objects\",\n> \t\t\t the_repository);\n> --\n> 2.30.0\n\nThis version looks good to me, thanks for digging!\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"414809","messageId":"YAhXw9Gvn5Pyvacq@coredump.intra.peff.net","threadId":"55017","inReplyTo":"YAhC8Gsp4H17e28n@nand.local","subject":"Re: [PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-20T16:18:11Z","receivedAt":"2021-01-20T16:19:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 20, 2021 at 09:49:20AM -0500, Taylor Blau wrote:\n\n> > @@ -3740,7 +3738,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> > \t}\n> > \tcleanup_preferred_base();\n> > \tif (include_tag && nr_result)\n> > -\t\tfor_each_ref(add_ref_tag, NULL);\n> > +\t\tfor_each_tag_ref(add_ref_tag, NULL);\n> \n> OK. Seeing another caller (builtin/pack-objects.c:compute_write_order())\n> that passes a callback to for_each_tag_ref() makes me feel more\n> comfortable about using it here.\n\nIt actually made me wonder if that call might potentially be\nproblematic, too. ;)\n\nIt is not clear to me from the commit message if anybody actually looked\nat how peel_ref() works, and whether there are any gotchas. I don't\nthink \"there is another call like this\" is compelling because we might\nnot notice any problem:\n\n - checking the ref_iterator's peeled value is an optimization; we'd\n expect to receive the correct answer but slower if it doesn't kick\n in\n\n - if the fallback code is looking up by refname, would it use the dwim\n rules and find an unqualified \"v1.2.3\". That would work most of the\n time, but fail if there was an ambiguous ref.\n\nBut looking at the implementation, I think the answer is \"no\". When\niterating over unqualified refs, the iterator stores a pointer to the\nunqualified name, and so the optimization does kick in. And even if we\nare not looking at a packed-refs value that can return the answer\nquickly, we eventually end up in cache_ref_iterator_peel(), which\nignores the refname and directly peels the oid.\n\nIf we do not match the ref iterator, then we use read_ref_full(), which\ndoesn't do any dwim magic. And hence our unqualified refname would fail.\nSo it's a bit weird to pass such a refname into the function, but it\nworks because of the optimization magic. And if that ever changed, I\nthink the test suite would notice, since many peels would fail in that\ncase.\n\nSo I think both the existing and the new calls using for_each_tag_ref()\nare OK here.\n\n As an aside, this peel_ref() interface is just awful, exactly because\n of this confusion. The nicest thing would be for the each_ref_fn\n callback to actually just get the peel information if we have it. But\n that would require changing tons of callbacks all over the code base,\n which is why we've never done it.\n\n But probably we would just peel by oid, and have the same \"is it the\n current iterated ref\" magic kick in. It doesn't matter if it's\n _actually_ the same ref or not. If two refs point to the same oid,\n then their peeled values are the same anyway.\n\n So that might be a nice cleanup, but definitely out of scope for this\n patch.\n\n-Peff\n"},{"id":"414810","messageId":"YAhYHUcdynbWyhwo@nand.local","threadId":"55017","inReplyTo":"YAhXw9Gvn5Pyvacq@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-20T16:19:41Z","receivedAt":"2021-01-20T16:21:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 20, 2021 at 11:18:11AM -0500, Jeff King wrote:\n> So I think both the existing and the new calls using for_each_tag_ref()\n> are OK here.\n\nIndeed, I followed the same trail of calls as you did and reached the\nsame conclusion, but didn't write any of it down here since I thought it\nwasn't worthwhile.\n\nBut, yes, I agree that both are safe.\n\nThanks,\nTaylor\n"},{"id":"414823","messageId":"20210120124514.49737-1-jacob@gitlab.com","threadId":"55017","inReplyTo":null,"subject":"[PATCH v2 0/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-20T12:45:13Z","receivedAt":"2021-01-20T17:27:10Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"I have updated the patch with more background information in the\ncommit message, and I have cleaned up the add_ref_tag iterator\ncallback a bit. I also switched to for_each_tag_ref.\n\nOn the previous version of the patch there was some discussion about\nwhether we can use for_each_tag_ref in combination with peel_ref. It\nturns out we can, and we were already doing it in the same file:\nsearch for mark_tagged to see what I mean.\n\nJacob Vosmaer (1):\n builtin/pack-objects.c: avoid iterating all refs\n\n builtin/pack-objects.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\n-- \n2.30.0\n\n"},{"id":"414824","messageId":"20210120124514.49737-2-jacob@gitlab.com","threadId":"55017","inReplyTo":"20210120124514.49737-1-jacob@gitlab.com","subject":"[PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-20T12:45:14Z","receivedAt":"2021-01-20T17:27:25Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"In git-pack-objects, we iterate over all the tags if the --include-tag\noption is passed on the command line. For some reason this uses\nfor_each_ref which is expensive if the repo has many refs. We should\nuse for_each_tag_ref instead.\n\nBecause the add_ref_tag callback will now only visit tags we\nsimplified it a bit.\n\nThe motivation for this change is that we observed performance issues\nwith a repository on gitlab.com that has 500,000 refs but only 2,000\ntags. The fetch traffic on that repo is dominated by CI, and when we\nchanged CI to fetch with 'git fetch --no-tags' we saw a dramatic\nchange in the CPU profile of git-pack-objects. This lead us to this\nparticular ref walk. More details in:\nhttps://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/746#note_483546598\n\nSigned-off-by: Jacob Vosmaer \n---\n builtin/pack-objects.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 2a00358f34..ad52c91bdb 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2803,13 +2803,11 @@ static void add_tag_chain(const struct object_id *oid)\n \t}\n }\n \n-static int add_ref_tag(const char *path, const struct object_id *oid, int flag, void *cb_data)\n+static int add_ref_tag(const char *tag, const struct object_id *oid, int flag, void *cb_data)\n {\n \tstruct object_id peeled;\n \n-\tif (starts_with(path, \"refs/tags/\") && /* is a tag? */\n-\t !peel_ref(path, &peeled) && /* peelable? */\n-\t obj_is_packed(&peeled)) /* object packed? */\n+\tif (!peel_ref(tag, &peeled) && obj_is_packed(&peeled))\n \t\tadd_tag_chain(oid);\n \treturn 0;\n }\n@@ -3740,7 +3738,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t}\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n-\t\tfor_each_ref(add_ref_tag, NULL);\n+\t\tfor_each_tag_ref(add_ref_tag, NULL);\n \tstop_progress(&progress_state);\n \ttrace2_region_leave(\"pack-objects\", \"enumerate-objects\",\n \t\t\t the_repository);\n-- \n2.30.0\n\n"},{"id":"414837","messageId":"CADMWQoPwNQafPcr2NvRcGzcWDwku1VYoGsKaKiBWSCRCvqY4Tg@mail.gmail.com","threadId":"55017","inReplyTo":"YAhYHUcdynbWyhwo@nand.local","subject":"Re: [PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-20T18:49:26Z","receivedAt":"2021-01-20T18:54:42Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"I also spent time being confused about what is going on, and wondering\nif that other ref iteration ever worked. I went as far as inspecting\ngit-verify-pack -v output to look at object order. :)\n\nOn the other hand, through working on this I learned that include-tag\nhas pretty effective test coverage so if peel_ref didn't work or\nstopped working, we'd find out.\n\nIf there is a better way to write \"for each tag + peel ref\" I am happy\nto change the patch, just let me know what it should look like.\n\nBest regards,\n\nJacob Vosmaer\nGitLab, Inc.\n\nOn Wed, Jan 20, 2021 at 5:19 PM Taylor Blau wrote:\n>\n> On Wed, Jan 20, 2021 at 11:18:11AM -0500, Jeff King wrote:\n> > So I think both the existing and the new calls using for_each_tag_ref()\n> > are OK here.\n>\n> Indeed, I followed the same trail of calls as you did and reached the\n> same conclusion, but didn't write any of it down here since I thought it\n> wasn't worthwhile.\n>\n> But, yes, I agree that both are safe.\n>\n> Thanks,\n> Taylor\n"},{"id":"414846","messageId":"YAiIbEAZSlL7B+am@coredump.intra.peff.net","threadId":"55017","inReplyTo":"YAhYHUcdynbWyhwo@nand.local","subject":"Re: [PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-20T19:45:48Z","receivedAt":"2021-01-20T19:47:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 20, 2021 at 11:19:41AM -0500, Taylor Blau wrote:\n\n> On Wed, Jan 20, 2021 at 11:18:11AM -0500, Jeff King wrote:\n> > So I think both the existing and the new calls using for_each_tag_ref()\n> > are OK here.\n> \n> Indeed, I followed the same trail of calls as you did and reached the\n> same conclusion, but didn't write any of it down here since I thought it\n> wasn't worthwhile.\n> \n> But, yes, I agree that both are safe.\n\nOK. I think it's worth saying such things (in case that was not obvious.\n;) ).\n\nI also rage-replaced peel_ref() with a function taking an oid so we\nnever have to do that digging again. Posted separately in its own\nthread.\n\n-Peff\n"},{"id":"414864","messageId":"YAimIr9j7SsmJJ55@nand.local","threadId":"55017","inReplyTo":"CADMWQoPrKBjLM5ABhhhCibEyJVOXQxsNkncSU6dmwjynQ1oCcQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-20T21:52:34Z","receivedAt":"2021-01-20T23:42:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 20, 2021 at 10:46:29PM +0100, Jacob Vosmaer wrote:\n> On Wed, Jan 20, 2021 at 8:45 PM Jeff King wrote:\n>\n> > I also rage-replaced peel_ref() with a function taking an oid so we\n> > never have to do that digging again. Posted separately in its own\n> > thread.\n>\n> That sounds like a good solution. Where does that leave my patch? Do I\n> need to wait for that new commit somehow? I don't know how that works,\n> \"contributing to Git\" wise.\n\nPeff noted in the patch to nuke \"peel_ref()\" that merging his and then\nyours will cause a merge conflict, but it is trivial to resolve. So, I'd\nexpect that Junio will pick up both and resolve the merge conflict when\nqueuing.\n\nOr, in other words, I don't think there's anything left on your part\n;-). Thanks for providing your patches; there was a nice digression and\nI'm quite happy with where we ended up.\n\nThanks,\nTaylor\n"},{"id":"414871","messageId":"CADMWQoPrKBjLM5ABhhhCibEyJVOXQxsNkncSU6dmwjynQ1oCcQ@mail.gmail.com","threadId":"55017","inReplyTo":"YAiIbEAZSlL7B+am@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-20T21:46:29Z","receivedAt":"2021-01-21T00:48:56Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"On Wed, Jan 20, 2021 at 8:45 PM Jeff King wrote:\n\n> I also rage-replaced peel_ref() with a function taking an oid so we\n> never have to do that digging again. Posted separately in its own\n> thread.\n\nThat sounds like a good solution. Where does that leave my patch? Do I\nneed to wait for that new commit somehow? I don't know how that works,\n\"contributing to Git\" wise.\n"},{"id":"414879","messageId":"YAjs1FjELfSdGhI+@coredump.intra.peff.net","threadId":"55017","inReplyTo":"CADMWQoPrKBjLM5ABhhhCibEyJVOXQxsNkncSU6dmwjynQ1oCcQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-21T02:54:12Z","receivedAt":"2021-01-21T03:51:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 20, 2021 at 10:46:29PM +0100, Jacob Vosmaer wrote:\n\n> On Wed, Jan 20, 2021 at 8:45 PM Jeff King wrote:\n> \n> > I also rage-replaced peel_ref() with a function taking an oid so we\n> > never have to do that digging again. Posted separately in its own\n> > thread.\n> \n> That sounds like a good solution. Where does that leave my patch? Do I\n> need to wait for that new commit somehow? I don't know how that works,\n> \"contributing to Git\" wise.\n\nNope, you shouldn't need to do anything. The conflict is minimal\nenough that the two can proceed independently, and the maintainer can\nresolve it.\n\n-Peff\n"},{"id":"414976","messageId":"CADMWQoPf9_49LcLT8RuYvQN45h_a3P0LswTPrKXn36rFBnoyig@mail.gmail.com","threadId":"55017","inReplyTo":"YAjs1FjELfSdGhI+@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-22T16:46:04Z","receivedAt":"2021-01-22T17:27:58Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Great, thanks!\n\nBest regards,\n\nJacob Vosmaer\nGitLab, Inc.\n\nOn Thu, Jan 21, 2021 at 3:54 AM Jeff King wrote:\n>\n> On Wed, Jan 20, 2021 at 10:46:29PM +0100, Jacob Vosmaer wrote:\n>\n> > On Wed, Jan 20, 2021 at 8:45 PM Jeff King wrote:\n> >\n> > > I also rage-replaced peel_ref() with a function taking an oid so we\n> > > never have to do that digging again. Posted separately in its own\n> > > thread.\n> >\n> > That sounds like a good solution. Where does that leave my patch? Do I\n> > need to wait for that new commit somehow? I don't know how that works,\n> > \"contributing to Git\" wise.\n>\n> Nope, you shouldn't need to do anything. The conflict is minimal\n> enough that the two can proceed independently, and the maintainer can\n> resolve it.\n>\n> -Peff\n"}]}