{"thread":{"id":"55011","subject":"[PATCH 0/1] builtin/pack-objects.c: avoid iterating all refs","startedAt":"2021-01-19T22:42:56Z","lastAt":"2021-01-21T15:36:58Z","messageCount":13,"participants":["Jacob Vosmaer","Taylor Blau","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"414737","messageId":"20210119143348.27535-1-jacob@gitlab.com","threadId":"55011","inReplyTo":null,"subject":"[PATCH 0/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-19T14:33:47Z","receivedAt":"2021-01-19T22:42:56Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"This is a small patch for git-pack-objects which will help server side\nperformance on repositories with lots of refs. I will post a related\nbut slightly larger patch for ls-refs.c in a separate thread.\n\nThe back story is in\nhttps://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/746 but I\nwill try to summarize it here.\n\nWe have a particular Gitaly (Git RPC) server at GitLab that has a very\nhomogenous workload, dominated by CI. While trying to reduce CPU\nutilization on the server we configured CI to fetch with the\n'--no-tags' option. This had an unexpectedly large impact so I started\nlooking closer at why that may be.\n\nWhat I learned is that by default, a fetch ends up using the\n'--include-tag' command line option of git-pack-objects. This causes\ngit-pack-objects to iterate through all the tags of the repository to\nsee if any should be included in the pack because they point to packed\nobjects. The problem is that this \"iterate through all the tags\" uses\nfor_each_ref which iterates through all references in the repository,\nand in doing so loads each associated object into memory to check if\nthe ref is broken. But all we need for '--include-tag' is to iterate\nthrough refs/tags/.\n\nOn the repo we were testing this on, there are about\n500,000 refs but only 2,000 tags. So we had to load a lot of objects\njust for the sake of '--include-tag'. It was common to see more than\nhalf the CPU time in git-pack-objects being spent in do_for_each_ref,\nand that in turn was dominated by ref_resolves_to_object.\n\nSo, I think it would be nice to just iterate over those 2,000 tags and\nnot load 500,000 objects outside refs/tags we already know we don't\ncare about.\n\nJacob Vosmaer (1):\n  builtin/pack-objects.c: avoid iterating all refs\n\n builtin/pack-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\n-- \n2.30.0\n\n"},{"id":"414738","messageId":"20210119143348.27535-2-jacob@gitlab.com","threadId":"55011","inReplyTo":"20210119143348.27535-1-jacob@gitlab.com","subject":"[PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-19T14:33:48Z","receivedAt":"2021-01-19T22:43:45Z","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 a prefix iterator instead.\n\nSigned-off-by: Jacob Vosmaer <jacob@gitlab.com>\n---\n builtin/pack-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 2a00358f34..2b32bc93bd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3740,7 +3740,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_fullref_in(\"refs/tags/\", add_ref_tag, NULL, 0);\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":"414746","messageId":"YAdmb3EfNVHYavNk@nand.local","threadId":"55011","inReplyTo":"20210119143348.27535-2-jacob@gitlab.com","subject":"Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-19T23:08:31Z","receivedAt":"2021-01-19T23:10:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 19, 2021 at 03:33:48PM +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 a prefix iterator instead.\n\nOK, makes sense to me.\n\n> Signed-off-by: Jacob Vosmaer <jacob@gitlab.com>\n> ---\n>  builtin/pack-objects.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 2a00358f34..2b32bc93bd 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3740,7 +3740,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_fullref_in(\"refs/tags/\", add_ref_tag, NULL, 0);\n\nMakes sense. I was curious why it wasn't this way from the beginning in\nf0a24aa56e (git-pack-objects: Automatically pack annotated tags if\nobject was packed, 2008-03-03).\n\nThe patch doesn't say, but presumably it was because there wasn't any\nspeed-up to be had iterating only a subset of references back then if\nthey were packed (did packed refs even exist in 2008? unsure). In any\ncase, builtin/pack-objects.c:add_ref_tag() discards anything that\ndoesn't start with \"refs/tags/\", so I think your change is doing the\nright thing here.\n\nThat said, you could shorten this to use 'for_each_tag_ref()' instead\n(which compiles to the exact same thing).\n\nIt probably wouldn't be a bad time to drop the extra prefix check in\nadd_ref_tag().\n\nThanks,\nTaylor\n"},{"id":"414749","messageId":"YAdn/zdQP1Hz70ba@coredump.intra.peff.net","threadId":"55011","inReplyTo":"20210119143348.27535-2-jacob@gitlab.com","subject":"Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-19T23:15:11Z","receivedAt":"2021-01-19T23:17:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2021 at 03:33:48PM +0100, Jacob Vosmaer wrote:\n\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 a prefix iterator instead.\n> [...]\n> -\t\tfor_each_ref(add_ref_tag, NULL);\n> +\t\tfor_each_fullref_in(\"refs/tags/\", add_ref_tag, NULL, 0);\n\nYeah, this is trivially correct, since the first thing we check in\nadd_ref_tag() is whether it starts with refs/tags/. It would be safe to\nremove that check as part of this patch (and unlike the multi-prefix\none, I am comfortable assuming that for_each_fullref_in() will not\nreturn anything outside of the prefix).\n\nI also wondered if we could just use for_each_ref_in() here, but I think\nwe are better having the full refname to feed to peel_ref().\n\n-Peff\n"},{"id":"414762","messageId":"YAdqkYtxqeXVnzN3@coredump.intra.peff.net","threadId":"55011","inReplyTo":"20210119143348.27535-1-jacob@gitlab.com","subject":"Re: [PATCH 0/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-19T23:26:09Z","receivedAt":"2021-01-19T23:30:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2021 at 03:33:47PM +0100, Jacob Vosmaer wrote:\n\n> What I learned is that by default, a fetch ends up using the\n> '--include-tag' command line option of git-pack-objects. This causes\n> git-pack-objects to iterate through all the tags of the repository to\n> see if any should be included in the pack because they point to packed\n> objects. The problem is that this \"iterate through all the tags\" uses\n> for_each_ref which iterates through all references in the repository,\n> and in doing so loads each associated object into memory to check if\n> the ref is broken. But all we need for '--include-tag' is to iterate\n> through refs/tags/.\n> \n> On the repo we were testing this on, there are about\n> 500,000 refs but only 2,000 tags. So we had to load a lot of objects\n> just for the sake of '--include-tag'. It was common to see more than\n> half the CPU time in git-pack-objects being spent in do_for_each_ref,\n> and that in turn was dominated by ref_resolves_to_object.\n\nSome of these details may be useful in the commit message, too. :)\n\nYour \"load a lot of objects\" had me worried for a moment. We try hard\nnot to load objects during such an iteration, even when peeling them\n(because the packed-refs format has a magic shortcut there). But I think\nthat is all working as intended. What you were seeing was just tons of\nhas_object_file() to make sure the ref was not corrupt (so finding the\nentry in a packfile, but not actually inflating the object contents).\n\nArguably both upload-pack and pack-objects could use the INCLUDE_BROKEN\nflag to avoid even checking this. We'd notice the problem when somebody\nactually tried to fetch the object in question. That would speed things\nup further on top of your patch, because we wouldn't need to check the\nexistence of even the tags. But it's definitely orthogonal, and should\nbe considered separately.\n\n-Peff\n"},{"id":"414763","messageId":"YAdsTjDXImYlfTn8@coredump.intra.peff.net","threadId":"55011","inReplyTo":"YAdmb3EfNVHYavNk@nand.local","subject":"Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-19T23:33:34Z","receivedAt":"2021-01-19T23:34:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2021 at 06:08:31PM -0500, Taylor Blau wrote:\n\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 2a00358f34..2b32bc93bd 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -3740,7 +3740,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_fullref_in(\"refs/tags/\", add_ref_tag, NULL, 0);\n> \n> Makes sense. I was curious why it wasn't this way from the beginning in\n> f0a24aa56e (git-pack-objects: Automatically pack annotated tags if\n> object was packed, 2008-03-03).\n> \n> The patch doesn't say, but presumably it was because there wasn't any\n> speed-up to be had iterating only a subset of references back then if\n> they were packed (did packed refs even exist in 2008? unsure). In any\n> case, builtin/pack-objects.c:add_ref_tag() discards anything that\n> doesn't start with \"refs/tags/\", so I think your change is doing the\n> right thing here.\n> \n> That said, you could shorten this to use 'for_each_tag_ref()' instead\n> (which compiles to the exact same thing).\n\nYou'd end up with \"v1.2.3\" in the refname field of the callback then,\nrather than \"refs/tags/v1.2.3\". So we'd definitely need to drop the\nprefix check in add_ref_tag() then (though I think it is a good idea\nanyway). But I'm also not sure if it would interfere with peel_ref(),\nand its magic for using packed-refs peeled information.\n\n-Peff\n"},{"id":"414765","messageId":"YAdxIfjkK+ShZ3Ux@nand.local","threadId":"55011","inReplyTo":"YAdsTjDXImYlfTn8@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-19T23:54:09Z","receivedAt":"2021-01-19T23:55:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 19, 2021 at 06:33:34PM -0500, Jeff King wrote:\n> On Tue, Jan 19, 2021 at 06:08:31PM -0500, Taylor Blau wrote:\n> > That said, you could shorten this to use 'for_each_tag_ref()' instead\n> > (which compiles to the exact same thing).\n>\n> You'd end up with \"v1.2.3\" in the refname field of the callback then,\n> rather than \"refs/tags/v1.2.3\". So we'd definitely need to drop the\n> prefix check in add_ref_tag() then (though I think it is a good idea\n> anyway). But I'm also not sure if it would interfere with peel_ref(),\n> and its magic for using packed-refs peeled information.\n\nAck, thanks for correcting me. Looking at ref_peel_ref(), I am almost\npositive that calling peel_ref() on \"v1.2.3\" (referring to\n\"refs/tags/v1.2.3\") wouldn't work. So I think the current implementation\nis good.\n\nThanks,\nTaylor\n"},{"id":"414782","messageId":"87lfco801g.fsf@evledraar.gmail.com","threadId":"55011","inReplyTo":"20210119143348.27535-2-jacob@gitlab.com","subject":"Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-20T08:50:19Z","receivedAt":"2021-01-20T09:00:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 19 2021, Jacob Vosmaer wrote:\n\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 a prefix iterator instead.\n>\n> Signed-off-by: Jacob Vosmaer <jacob@gitlab.com>\n> ---\n>  builtin/pack-objects.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 2a00358f34..2b32bc93bd 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3740,7 +3740,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_fullref_in(\"refs/tags/\", add_ref_tag, NULL, 0);\n>  \tstop_progress(&progress_state);\n>  \ttrace2_region_leave(\"pack-objects\", \"enumerate-objects\",\n>  \t\t\t    the_repository);\n\nNot really a new problem, but it would be nice to also have a test in\nt5305-include-tag.sh to check what happens with \"tags\" outside the\nrefs/tags/ namespace. I.e. if this & the existing prefix in\nadd_ref_tag() were dropped.\n\nTo elaborate on other things that aren't really your problem & Taylor's\nE-Mail downthread we originally added this because:\n\n    If new annotated tags have been introduced then we can also include\n    them in the packfile, saving the client from needing to request them\n    through a second connection.\n\nI've barked up this whole tag fetch tree before 97716d217c (fetch: add a\n--prune-tags option and fetch.pruneTags config, 2018-02-09) but really\nnot this specific area.\n\nI wonder if longer term simply moving this to be work the client does\nwouldn't make more sense. I.e. if we just delete this for_each_ref()\nloop.\n    \nWe're just saving a client from saying they \"want\" a tag. I.e. with the\nwhole thing removed we need this to make t5503-tagfollow.sh pass (see\n[1] at the end for the dialog, the tag is 3253df4d...):\n\n    diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\n    index 6041a4dd32..1ddc430aef 100755\n    --- a/t/t5503-tagfollow.sh\n    +++ b/t/t5503-tagfollow.sh\n    @@ -134,6 +134,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '\n     test_expect_success 'setup expect' '\n     cat - <<EOF >expect\n     want $B\n    +want $T\n     want $S\n     EOF\n     '\n    @@ -146,6 +147,7 @@ test_expect_success 'new clone fetch master and tags' '\n                    cd clone2 &&\n                    git init &&\n                    git remote add origin .. &&\n    +               git config --add remote.origin.fetch \"+refs/tags/*:refs/tags/*\" &&\n                    GIT_TRACE_PACKET=$UPATH git fetch &&\n                    test $B = $(git rev-parse --verify origin/master) &&\n                    test $S = $(git rev-parse --verify tag2) &&\n\nWe're also saving the client the work of having to go through\nrefs/tags/* and figure out whether there are tags there that aren't on\nour main history.\n\nI think that since f0a24aa56e (git-pack-objects: Automatically pack\nannotated tags if object was packed, 2008-03-03) was written it's become\nclear that in the wild that's almost nobody's use-case. I.e. people with\na set of refs/heads/* branches and a refs/tags/* set that doesn't refer\nto those branches.\n\nOr if they do, I think it's such an obscure use-case that if we were\ndesigning this today we could pass that work of figuring out if there's\nsuch tags in refs/tags/* off to the client.\n\nIt seems we might just be able to delete this code on the server-side,\nper protocol-capabilities.txt:\n\n    Clients MUST be prepared for the case where a server has ignored\n    include-tag and has not actually sent tags in the pack.  In such\n    cases the client SHOULD issue a subsequent fetch to acquire the tags\n    that include-tag would have otherwise given the client.\n\nI.e. in the case where the server isn't playing along and I haven't set\n\"+refs/tags/*:refs/tags/*\". But as the test shows we don't do that\nfollowing ourselves unless refs/tags/* is in the refspec (and then it's\nnot really \"following\", we're just getting all the tags).\n\n1. From t5503-tagfollow.sh:\n    $ grep -C5 3253df4d1cf6fb138b52b1938473bcfec1483223 UPLOAD_LOG \n    packet:  upload-pack< ref-prefix refs/tags/\n    packet:  upload-pack< ref-prefix refs/tags/\n    packet:  upload-pack< 0000\n    packet:  upload-pack> 8e10cf4e007ad7e003463c30c34b1050b039db78 refs/heads/master\n    packet:        fetch< 8e10cf4e007ad7e003463c30c34b1050b039db78 refs/heads/master\n    packet:  upload-pack> 3253df4d1cf6fb138b52b1938473bcfec1483223 refs/tags/tag1 peeled:c06aaaf9c64f9ef1f209bcb6d7bdba68a5e27fab\n    packet:        fetch< 3253df4d1cf6fb138b52b1938473bcfec1483223 refs/tags/tag1 peeled:c06aaaf9c64f9ef1f209bcb6d7bdba68a5e27fab\n    packet:  upload-pack> ddfa4a33562179aca1ace2bcc662244a17d0b503 refs/tags/tag2 peeled:8e10cf4e007ad7e003463c30c34b1050b039db78\n    packet:  upload-pack> 0000\n    packet:        fetch< ddfa4a33562179aca1ace2bcc662244a17d0b503 refs/tags/tag2 peeled:8e10cf4e007ad7e003463c30c34b1050b039db78\n    packet:        fetch< 0000\n    packet:        fetch> command=fetch\n    --\n    packet:        fetch> 0001\n    packet:        fetch> thin-pack\n    packet:        fetch> include-tag\n    packet:        fetch> ofs-delta\n    packet:        fetch> want 8e10cf4e007ad7e003463c30c34b1050b039db78\n    packet:        fetch> want 3253df4d1cf6fb138b52b1938473bcfec1483223\n    packet:        fetch> want ddfa4a33562179aca1ace2bcc662244a17d0b503\n    packet:        fetch> done\n    packet:        fetch> 0000\n    packet:  upload-pack< object-format=sha1\n    packet:  upload-pack< command=fetch\n    --\n    packet:  upload-pack< 0001\n    packet:  upload-pack< thin-pack\n    packet:  upload-pack< include-tag\n    packet:  upload-pack< ofs-delta\n    packet:  upload-pack< want 8e10cf4e007ad7e003463c30c34b1050b039db78\n    packet:  upload-pack< want 3253df4d1cf6fb138b52b1938473bcfec1483223\n    packet:  upload-pack< want ddfa4a33562179aca1ace2bcc662244a17d0b503\n    packet:  upload-pack< done\n    packet:  upload-pack< 0000\n    packet:  upload-pack> packfile\n    packet:        fetch< packfile\n    \n"},{"id":"414797","messageId":"YAhF7HfOdu5AR42Q@nand.local","threadId":"55011","inReplyTo":"87lfco801g.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-20T15:02:04Z","receivedAt":"2021-01-20T15:22:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 20, 2021 at 09:50:19AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> It seems we might just be able to delete this code on the server-side,\n> per protocol-capabilities.txt:\n>\n>     Clients MUST be prepared for the case where a server has ignored\n>     include-tag and has not actually sent tags in the pack.  In such\n>     cases the client SHOULD issue a subsequent fetch to acquire the tags\n>     that include-tag would have otherwise given the client.\n>\n> I.e. in the case where the server isn't playing along and I haven't set\n> \"+refs/tags/*:refs/tags/*\". But as the test shows we don't do that\n> following ourselves unless refs/tags/* is in the refspec (and then it's\n> not really \"following\", we're just getting all the tags).\n\nReading your email, I see no reason not to do it, and that snippet from\nprotocol-capabilities.txt makes me feel even better about doing so.\n\nI'd be happy to have Jacob's patch picked up in the meantime, but I\nthink that this is a good direction to pursue.\n\nThanks,\nTaylor\n"},{"id":"414811","messageId":"87czxz8t7o.fsf@evledraar.gmail.com","threadId":"55011","inReplyTo":"YAhF7HfOdu5AR42Q@nand.local","subject":"Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-20T16:32:27Z","receivedAt":"2021-01-20T16:38:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 20 2021, Taylor Blau wrote:\n\n> On Wed, Jan 20, 2021 at 09:50:19AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> It seems we might just be able to delete this code on the server-side,\n>> per protocol-capabilities.txt:\n>>\n>>     Clients MUST be prepared for the case where a server has ignored\n>>     include-tag and has not actually sent tags in the pack.  In such\n>>     cases the client SHOULD issue a subsequent fetch to acquire the tags\n>>     that include-tag would have otherwise given the client.\n>>\n>> I.e. in the case where the server isn't playing along and I haven't set\n>> \"+refs/tags/*:refs/tags/*\". But as the test shows we don't do that\n>> following ourselves unless refs/tags/* is in the refspec (and then it's\n>> not really \"following\", we're just getting all the tags).\n>\n> Reading your email, I see no reason not to do it, and that snippet from\n> protocol-capabilities.txt makes me feel even better about doing so.\n>\n> I'd be happy to have Jacob's patch picked up in the meantime, but I\n> think that this is a good direction to pursue.\n\nOh yes, none of this is commentary on not accepting the much smaller and\nobviously correct change in the meantime.  Just musings this general\narea of fetching & ideas for further optimization.\n"},{"id":"414850","messageId":"YAiKQ0M0/14Q13Ee@coredump.intra.peff.net","threadId":"55011","inReplyTo":"87lfco801g.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-20T19:53:39Z","receivedAt":"2021-01-20T19:54:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 20, 2021 at 09:50:19AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> To elaborate on other things that aren't really your problem & Taylor's\n> E-Mail downthread we originally added this because:\n> \n>     If new annotated tags have been introduced then we can also include\n>     them in the packfile, saving the client from needing to request them\n>     through a second connection.\n> \n> I've barked up this whole tag fetch tree before 97716d217c (fetch: add a\n> --prune-tags option and fetch.pruneTags config, 2018-02-09) but really\n> not this specific area.\n> \n> I wonder if longer term simply moving this to be work the client does\n> wouldn't make more sense. I.e. if we just delete this for_each_ref()\n> loop.\n>     \n> We're just saving a client from saying they \"want\" a tag. I.e. with the\n> whole thing removed we need this to make t5503-tagfollow.sh pass (see\n> [1] at the end for the dialog, the tag is 3253df4d...):\n\nIsn't this an ordering problem, though? The client cannot mention\nauto-followed tags in a \"want\", because they first need to \"want\" the\nmain history, receive the pack, and then realize they want the others.\n\nSo we are not just saving the client from sending a \"want\", but making a\nsecond full connection to do so. That seems to be an optimization worth\ncontinuing to have.\n\nBut here...\n\n>     diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\n>     index 6041a4dd32..1ddc430aef 100755\n>     --- a/t/t5503-tagfollow.sh\n>     +++ b/t/t5503-tagfollow.sh\n>     @@ -134,6 +134,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '\n>      test_expect_success 'setup expect' '\n>      cat - <<EOF >expect\n>      want $B\n>     +want $T\n>      want $S\n>      EOF\n>      '\n>     @@ -146,6 +147,7 @@ test_expect_success 'new clone fetch master and tags' '\n>                     cd clone2 &&\n>                     git init &&\n>                     git remote add origin .. &&\n>     +               git config --add remote.origin.fetch \"+refs/tags/*:refs/tags/*\" &&\n>                     GIT_TRACE_PACKET=$UPATH git fetch &&\n>                     test $B = $(git rev-parse --verify origin/master) &&\n>                     test $S = $(git rev-parse --verify tag2) &&\n> \n> We're also saving the client the work of having to go through\n> refs/tags/* and figure out whether there are tags there that aren't on\n> our main history.\n\nYou seem to be against auto-following at all. And certainly I can see an\nargument that it is not worth the trouble it causes. But it is the\ndefault behavior, and I suspect many people are relying on it. Fetching\nevery tag indiscriminately is going to grab a bunch of extra unwanted\nobjects in some repos. An obvious case is any time \"clone\n--single-branch --depth\" is used.\n\nMaybe I'm not quite understanding what you're proposing.\n\n-Peff\n"},{"id":"414894","messageId":"874kja8u2i.fsf@evledraar.gmail.com","threadId":"55011","inReplyTo":"YAiKQ0M0/14Q13Ee@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-21T10:26:13Z","receivedAt":"2021-01-21T10:27:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 20 2021, Jeff King wrote:\n\n> On Wed, Jan 20, 2021 at 09:50:19AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> To elaborate on other things that aren't really your problem & Taylor's\n>> E-Mail downthread we originally added this because:\n>> \n>>     If new annotated tags have been introduced then we can also include\n>>     them in the packfile, saving the client from needing to request them\n>>     through a second connection.\n>> \n>> I've barked up this whole tag fetch tree before 97716d217c (fetch: add a\n>> --prune-tags option and fetch.pruneTags config, 2018-02-09) but really\n>> not this specific area.\n>> \n>> I wonder if longer term simply moving this to be work the client does\n>> wouldn't make more sense. I.e. if we just delete this for_each_ref()\n>> loop.\n>>     \n>> We're just saving a client from saying they \"want\" a tag. I.e. with the\n>> whole thing removed we need this to make t5503-tagfollow.sh pass (see\n>> [1] at the end for the dialog, the tag is 3253df4d...):\n>\n> Isn't this an ordering problem, though? The client cannot mention\n> auto-followed tags in a \"want\", because they first need to \"want\" the\n> main history, receive the pack, and then realize they want the others.\n>\n> So we are not just saving the client from sending a \"want\", but making a\n> second full connection to do so. That seems to be an optimization worth\n> continuing to have.\n\nCorrect. Suppose a history COMMIT(TAG) history like:\n\n    mainline: A(X) -> B(Y) -> C\n    topic:    A(X) -> B(Y) -> D(Z)\n\nYou only want the \"mainline\" history and its tags in your fetch. Now a\nserver will give you tags X & Y for free, ignoring Z.\n\nThe note in protocol-capabilities.txt supposes that you'll need to only\nget A/B/C in one fetch, then do another one where you see from the OIDs\nyou have and advertised (peeled) OIDs for the tags and know you just\nneed X/Y, not Z.\n\nSo we could also just fetch Z, then do our own walk on the client to see\nif it's reachable, and throw it away if not. Although I suppose that'll\nneed a list of \"bad\" tags on the client so we don't repeat the process\nthe next time around, hrm...\n\n>>     diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\n>>     index 6041a4dd32..1ddc430aef 100755\n>>     --- a/t/t5503-tagfollow.sh\n>>     +++ b/t/t5503-tagfollow.sh\n>>     @@ -134,6 +134,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '\n>>      test_expect_success 'setup expect' '\n>>      cat - <<EOF >expect\n>>      want $B\n>>     +want $T\n>>      want $S\n>>      EOF\n>>      '\n>>     @@ -146,6 +147,7 @@ test_expect_success 'new clone fetch master and tags' '\n>>                     cd clone2 &&\n>>                     git init &&\n>>                     git remote add origin .. &&\n>>     +               git config --add remote.origin.fetch \"+refs/tags/*:refs/tags/*\" &&\n>>                     GIT_TRACE_PACKET=$UPATH git fetch &&\n>>                     test $B = $(git rev-parse --verify origin/master) &&\n>>                     test $S = $(git rev-parse --verify tag2) &&\n>> \n>> We're also saving the client the work of having to go through\n>> refs/tags/* and figure out whether there are tags there that aren't on\n>> our main history.\n>\n> You seem to be against auto-following at all. And certainly I can see an\n> argument that it is not worth the trouble it causes. But it is the\n> default behavior, and I suspect many people are relying on it. Fetching\n> every tag indiscriminately is going to grab a bunch of extra unwanted\n> objects in some repos. An obvious case is any time \"clone\n> --single-branch --depth\" is used.\n\nI wonder if the use-cases for --depth in the wild aren't 99.9%\n--depth=1, in which case the peeled commit on the main branch being\nadvertised by a tag would alredy give you this information.\n\nI.e. in the common case you don't get a tag, but if you happen to land\non a release you can see that the commit at the tip is tagged. I wonder\nif doing that shortcut already in the client (so not send include-tag)\nis a useful micro-optimization.\n\n> Maybe I'm not quite understanding what you're proposing.\n\nNot much really, just that looking deeper into this area might be a\nuseful exercise. I.e. it's a case of server<->client cooperation where\nwe offlod the work to one or the other based on an old design decision,\nmaybe that trade-off is not optimal in most cases anymore.\n"},{"id":"414907","messageId":"YAmfAjZYDHoXBevr@coredump.intra.peff.net","threadId":"55011","inReplyTo":"874kja8u2i.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-21T15:34:26Z","receivedAt":"2021-01-21T15:36:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 21, 2021 at 11:26:13AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > So we are not just saving the client from sending a \"want\", but making a\n> > second full connection to do so. That seems to be an optimization worth\n> > continuing to have.\n> \n> Correct. Suppose a history COMMIT(TAG) history like:\n> \n>     mainline: A(X) -> B(Y) -> C\n>     topic:    A(X) -> B(Y) -> D(Z)\n> \n> You only want the \"mainline\" history and its tags in your fetch. Now a\n> server will give you tags X & Y for free, ignoring Z.\n> \n> The note in protocol-capabilities.txt supposes that you'll need to only\n> get A/B/C in one fetch, then do another one where you see from the OIDs\n> you have and advertised (peeled) OIDs for the tags and know you just\n> need X/Y, not Z.\n\nRight. That note basically indicates that what the server is doing is\n\"best effort\". If it sends you B, it will also send you Y. But it is\nultimately up to the client to decide if they want or need a tag the\nserver didn't send and do the follow-up.\n\nSo include-tag is an optimization, but it works often enough that it\nfrequently saves the second fetch from happening at all.\n\n> So we could also just fetch Z, then do our own walk on the client to see\n> if it's reachable, and throw it away if not. Although I suppose that'll\n> need a list of \"bad\" tags on the client so we don't repeat the process\n> the next time around, hrm...\n\nNot only that, but now you've fetched \"D\" that you never wanted. If it's\none commit, that's not so bad. But it could be pulling in arbitrary\namounts of reachable history that you don't want.\n\n> > You seem to be against auto-following at all. And certainly I can see an\n> > argument that it is not worth the trouble it causes. But it is the\n> > default behavior, and I suspect many people are relying on it. Fetching\n> > every tag indiscriminately is going to grab a bunch of extra unwanted\n> > objects in some repos. An obvious case is any time \"clone\n> > --single-branch --depth\" is used.\n> \n> I wonder if the use-cases for --depth in the wild aren't 99.9%\n> --depth=1, in which case the peeled commit on the main branch being\n> advertised by a tag would alredy give you this information.\n\nI'd have thought there was basically no use for anything but --depth=1,\nbut people seem really enamored with the notion of --depth=50. Many CI\nsystems use that for some reason.\n\n--depth is just one case where you might not have all of history,\nthough. Even without it, --single-branch means you wouldn't want to get\nthe history of all of the other branches. They may only be a few commits\nahead of the main history, in which case fetching extra tags that point\nto them might not be a big deal. But it really depends on how your repo\nis shaped, what tags point to, etc.\n\nEven without any options, your repo may have a disjoint chunk of history\nmentioned by a tag (e.g., a tag pointing to a segment of history that is\nmeant to be grafted on, or even an archived version of history from\nbefore filter-branch rewrite).\n\nPerhaps if we were designing from scratch we might write those off as\nunusual cases we don't need to care about. But I'd be very hesitant to\nchange the way tag following works now, after so many years. If we were\nto make any change, I think one plausible one would be to have clone set\nup refspecs to fetch all tags (and then avoid sending include-tag, since\nwe know we're not auto-following). That would let people still use\nauto-follow when they wanted to, but make things simpler to reason\nabout. And after all, clone already fetches all of the tags.\n\nBut I haven't thought too hard about it.\n\n> > Maybe I'm not quite understanding what you're proposing.\n> \n> Not much really, just that looking deeper into this area might be a\n> useful exercise. I.e. it's a case of server<->client cooperation where\n> we offlod the work to one or the other based on an old design decision,\n> maybe that trade-off is not optimal in most cases anymore.\n\nI don't think it's very much work on the server, though. Sure, iterating\nthe tags is O(nr_tags). But we do that iteration lots of other places,\ntoo (the advertisement unless the client asks for a very narrow prefix,\nor upload-pack's ref-marking for reachability).\n\nAnd saving us from sending unwanted objects is a potential win on the\nserver (even a single unlucky delta where we have to reconstruct and\nre-deflate an object will likely blow a ref iteration out of the water).\nLikewise, preventing the client from connecting a second time is a win\nfor the server, which doesn't have to spin up a new upload-pack\n(likewise for the client of course; it might even have to prompt the\nuser for auth again!).\n\n-Peff\n"}]}