{"thread":{"id":"56374","subject":"[PATCH] ls-refs: reuse buffer when sending refs","startedAt":"2021-08-25T13:50:03Z","lastAt":"2021-08-25T17:23:57Z","messageCount":5,"participants":["Patrick Steinhardt","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"433733","messageId":"ccd03e685af0f5cf25c68272a758fc88d115e37a.1629899211.git.ps@pks.im","threadId":"56374","inReplyTo":null,"subject":"[PATCH] ls-refs: reuse buffer when sending refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-25T13:49:51Z","receivedAt":"2021-08-25T13:50:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the initial reference advertisement, the Git server will first\nannounce all of its references to the client. The logic is handled in\n`send_ref()`, which will allocate a new buffer for each refline it is\nabout to send. This is quite wasteful: instead of allocating a new\nbuffer each time, we can just reuse a buffer.\n\nImprove this by passing in a buffer via the `ls_refs_data` struct which\nis then reused on each reference. In a repository with about 2.3M refs,\nthis speeds up local mirror fetches by about 2%:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     25.415 s ±  0.131 s    [User: 22.722 s, System: 4.740 s]\n      Range (min … max):   25.240 s … 25.543 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     24.922 s ±  0.110 s    [User: 22.404 s, System: 4.476 s]\n      Range (min … max):   24.825 s … 25.081 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.02 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@kps.im>\n---\n\nNote that while this topic applies on top of \"master\", I've done the\nbenchmark on top of my other optimizations for fetches. It's cheating a\nbit, but it's easier to see that the optimization does something when\nthe remaining constant part is lower.\n\n ls-refs.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 88f6c3f60d..84021416ca 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -65,6 +65,7 @@ struct ls_refs_data {\n \tunsigned peel;\n \tunsigned symrefs;\n \tstruct strvec prefixes;\n+\tstruct strbuf buf;\n \tunsigned unborn : 1;\n };\n \n@@ -73,7 +74,8 @@ static int send_ref(const char *refname, const struct object_id *oid,\n {\n \tstruct ls_refs_data *data = cb_data;\n \tconst char *refname_nons = strip_namespace(refname);\n-\tstruct strbuf refline = STRBUF_INIT;\n+\n+\tstrbuf_reset(&data->buf);\n \n \tif (ref_is_hidden(refname_nons, refname))\n \t\treturn 0;\n@@ -82,9 +84,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \n \tif (oid)\n-\t\tstrbuf_addf(&refline, \"%s %s\", oid_to_hex(oid), refname_nons);\n+\t\tstrbuf_addf(&data->buf, \"%s %s\", oid_to_hex(oid), refname_nons);\n \telse\n-\t\tstrbuf_addf(&refline, \"unborn %s\", refname_nons);\n+\t\tstrbuf_addf(&data->buf, \"unborn %s\", refname_nons);\n \tif (data->symrefs && flag & REF_ISSYMREF) {\n \t\tstruct object_id unused;\n \t\tconst char *symref_target = resolve_ref_unsafe(refname, 0,\n@@ -94,20 +96,19 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\tif (!symref_target)\n \t\t\tdie(\"'%s' is a symref but it is not?\", refname);\n \n-\t\tstrbuf_addf(&refline, \" symref-target:%s\",\n+\t\tstrbuf_addf(&data->buf, \" symref-target:%s\",\n \t\t\t    strip_namespace(symref_target));\n \t}\n \n \tif (data->peel && oid) {\n \t\tstruct object_id peeled;\n \t\tif (!peel_iterated_oid(oid, &peeled))\n-\t\t\tstrbuf_addf(&refline, \" peeled:%s\", oid_to_hex(&peeled));\n+\t\t\tstrbuf_addf(&data->buf, \" peeled:%s\", oid_to_hex(&peeled));\n \t}\n \n-\tstrbuf_addch(&refline, '\\n');\n-\tpacket_write(1, refline.buf, refline.len);\n+\tstrbuf_addch(&data->buf, '\\n');\n+\tpacket_write(1, data->buf.buf, data->buf.len);\n \n-\tstrbuf_release(&refline);\n \treturn 0;\n }\n \n@@ -145,6 +146,7 @@ int ls_refs(struct repository *r, struct strvec *keys,\n \n \tmemset(&data, 0, sizeof(data));\n \tstrvec_init(&data.prefixes);\n+\tstrbuf_init(&data.buf, 0);\n \n \tensure_config_read();\n \tgit_config(ls_refs_config, NULL);\n@@ -173,6 +175,7 @@ int ls_refs(struct repository *r, struct strvec *keys,\n \t\t\t\t     send_ref, &data, 0);\n \tpacket_flush(1);\n \tstrvec_clear(&data.prefixes);\n+\tstrbuf_release(&data.buf);\n \treturn 0;\n }\n \n-- \n2.33.0\n\n"},{"id":"433736","messageId":"YSZPUYk5J10DID+7@ncase","threadId":"56374","inReplyTo":"ccd03e685af0f5cf25c68272a758fc88d115e37a.1629899211.git.ps@pks.im","subject":"Re: [PATCH] ls-refs: reuse buffer when sending refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-25T14:10:25Z","receivedAt":"2021-08-25T14:10:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 25, 2021 at 03:49:51PM +0200, Patrick Steinhardt wrote:\n> In the initial reference advertisement, the Git server will first\n> announce all of its references to the client. The logic is handled in\n> `send_ref()`, which will allocate a new buffer for each refline it is\n> about to send. This is quite wasteful: instead of allocating a new\n> buffer each time, we can just reuse a buffer.\n> \n> Improve this by passing in a buffer via the `ls_refs_data` struct which\n> is then reused on each reference. In a repository with about 2.3M refs,\n> this speeds up local mirror fetches by about 2%:\n> \n>     Benchmark #1: HEAD~: git-fetch\n>       Time (mean ± σ):     25.415 s ±  0.131 s    [User: 22.722 s, System: 4.740 s]\n>       Range (min … max):   25.240 s … 25.543 s    5 runs\n> \n>     Benchmark #2: HEAD: git-fetch\n>       Time (mean ± σ):     24.922 s ±  0.110 s    [User: 22.404 s, System: 4.476 s]\n>       Range (min … max):   24.825 s … 25.081 s    5 runs\n> \n>     Summary\n>       'HEAD: git-fetch' ran\n>         1.02 ± 0.01 times faster than 'HEAD~: git-fetch'\n> \n> Signed-off-by: Patrick Steinhardt <ps@kps.im>\n\nI accidentally mis-typed my mail here. Really shows I should be using\n`--signoff` instead of manually signing things. Anyway, I'll fix this in\ncase I'll need to re-roll.\n\nPatrick\n"},{"id":"433742","messageId":"705fee2c-7711-ac99-f692-ab08cd7b4e26@gmail.com","threadId":"56374","inReplyTo":"ccd03e685af0f5cf25c68272a758fc88d115e37a.1629899211.git.ps@pks.im","subject":"Re: [PATCH] ls-refs: reuse buffer when sending refs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-25T14:50:30Z","receivedAt":"2021-08-25T14:50:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/25/2021 9:49 AM, Patrick Steinhardt wrote:\n> In the initial reference advertisement, the Git server will first\n> announce all of its references to the client. The logic is handled in\n> `send_ref()`, which will allocate a new buffer for each refline it is\n> about to send. This is quite wasteful: instead of allocating a new\n> buffer each time, we can just reuse a buffer.\n\nReusing a buffer makes perfect sense and is a clear improvement.\n \n> Improve this by passing in a buffer via the `ls_refs_data` struct which\n> is then reused on each reference. In a repository with about 2.3M refs,\n> this speeds up local mirror fetches by about 2%:\n> \n>     Benchmark #1: HEAD~: git-fetch\n>       Time (mean ± σ):     25.415 s ±  0.131 s    [User: 22.722 s, System: 4.740 s]\n>       Range (min … max):   25.240 s … 25.543 s    5 runs\n> \n>     Benchmark #2: HEAD: git-fetch\n>       Time (mean ± σ):     24.922 s ±  0.110 s    [User: 22.404 s, System: 4.476 s]\n>       Range (min … max):   24.825 s … 25.081 s    5 runs\n> \n>     Summary\n>       'HEAD: git-fetch' ran\n>         1.02 ± 0.01 times faster than 'HEAD~: git-fetch'\n> \n> Signed-off-by: Patrick Steinhardt <ps@kps.im>\n> ---\n> \n> Note that while this topic applies on top of \"master\", I've done the\n> benchmark on top of my other optimizations for fetches. It's cheating a\n> bit, but it's easier to see that the optimization does something when\n> the remaining constant part is lower.\n\nI don't mind demonstrating an optimization using the other work.\n\nPerhaps this would be better grouped with those other changes?\nI know that the text is independent and merges cleanly without it,\nbut it can be helpful to think about the effort as one unified\ntopic instead of juggling multiple, especially because I don't\nsee the other one needing many revisions.\n\n> -\tstruct strbuf refline = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&data->buf);\n\nIt's nice that this is the only _real_ change, and everything\nelse is a find-and-replace.\n\n> @@ -145,6 +146,7 @@ int ls_refs(struct repository *r, struct strvec *keys,\n>  \n>  \tmemset(&data, 0, sizeof(data));\n>  \tstrvec_init(&data.prefixes);\n> +\tstrbuf_init(&data.buf, 0);\n>  \n>  \tensure_config_read();\n>  \tgit_config(ls_refs_config, NULL);\n> @@ -173,6 +175,7 @@ int ls_refs(struct repository *r, struct strvec *keys,\n>  \t\t\t\t     send_ref, &data, 0);\n>  \tpacket_flush(1);\n>  \tstrvec_clear(&data.prefixes);\n> +\tstrbuf_release(&data.buf);\n>  \treturn 0;\n>  }\n\nExcept, of course, these two lines.\n\nI think this patch is good to go!\n\nThanks,\n-Stolee\n"},{"id":"433746","messageId":"YSZlK2y74wmYQcSd@ncase","threadId":"56374","inReplyTo":"705fee2c-7711-ac99-f692-ab08cd7b4e26@gmail.com","subject":"Re: [PATCH] ls-refs: reuse buffer when sending refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-25T15:43:39Z","receivedAt":"2021-08-25T15:43:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 25, 2021 at 10:50:30AM -0400, Derrick Stolee wrote:\n> On 8/25/2021 9:49 AM, Patrick Steinhardt wrote:\n> > Improve this by passing in a buffer via the `ls_refs_data` struct which\n> > is then reused on each reference. In a repository with about 2.3M refs,\n> > this speeds up local mirror fetches by about 2%:\n> > \n> >     Benchmark #1: HEAD~: git-fetch\n> >       Time (mean ± σ):     25.415 s ±  0.131 s    [User: 22.722 s, System: 4.740 s]\n> >       Range (min … max):   25.240 s … 25.543 s    5 runs\n> > \n> >     Benchmark #2: HEAD: git-fetch\n> >       Time (mean ± σ):     24.922 s ±  0.110 s    [User: 22.404 s, System: 4.476 s]\n> >       Range (min … max):   24.825 s … 25.081 s    5 runs\n> > \n> >     Summary\n> >       'HEAD: git-fetch' ran\n> >         1.02 ± 0.01 times faster than 'HEAD~: git-fetch'\n> > \n> > Signed-off-by: Patrick Steinhardt <ps@kps.im>\n> > ---\n> > \n> > Note that while this topic applies on top of \"master\", I've done the\n> > benchmark on top of my other optimizations for fetches. It's cheating a\n> > bit, but it's easier to see that the optimization does something when\n> > the remaining constant part is lower.\n> \n> I don't mind demonstrating an optimization using the other work.\n> \n> Perhaps this would be better grouped with those other changes?\n> I know that the text is independent and merges cleanly without it,\n> but it can be helpful to think about the effort as one unified\n> topic instead of juggling multiple, especially because I don't\n> see the other one needing many revisions.\n\nI don't know. I just happen to revisit this topic every few days, and\nevery time I stumble upon some more performance improvements. It would\nfeel wrong to shift the goalposts of the other series every time I find\nsomething new, so I instead opt for separate patch series.\n\nIf this proves to be annoying for reviewers, then feel free to shout at\nme and I'll change my approach.\n\nThanks for your review!\n\nPatrick\n"},{"id":"433755","messageId":"xmqqzgt5igk6.fsf@gitster.g","threadId":"56374","inReplyTo":"YSZlK2y74wmYQcSd@ncase","subject":"Re: [PATCH] ls-refs: reuse buffer when sending refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-25T17:23:53Z","receivedAt":"2021-08-25T17:23:57Z","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> I don't know. I just happen to revisit this topic every few days, and\n> every time I stumble upon some more performance improvements. It would\n> feel wrong to shift the goalposts of the other series every time I find\n> something new, so I instead opt for separate patch series.\n>\n> If this proves to be annoying for reviewers, then feel free to shout at\n> me and I'll change my approach.\n\nThe easiest would be to keep them independent and to justify them\nindependently.  There is no shifting the goalposts involved if you\ndid so.  Of course, you wouldn't be able to include the improvements\nthe follow-on topics make as part of the \"advertising material\" to\nsell the benefit of the base series, though.  It can only work when\nthe follow-on topic is truly independent and is a good idea by\nitself.  Another is to keep these follow-on topics unpublished\nbefore the base topic graduates, and pretend that you came up with\nthem much later than you originally did.  Nobody would notice and\nmind, as the base topic would be cast in stone by that time.\n\nAt the receiving end, what is most irritating is a series of topics\nthat pretends to be about different things but depend on each other\nto function well [*].  I would imagine that it would be a lot more\ntrivial and pleasant to handle if any of the patches involved did\nnot come before these follow-on topics are all already thought of by\nthe author and instead came in a well-structured single topic, but\nwe do not live in a perfect world ;-).\n\nIn the case of this particular patch, I think the logic behind the\nchange makes sense by itself, so if I were doing it, I'd probably\nchoose to sell it as an independent change unrelated to the other\ntopic.\n\nThanks.\n\n\n[Footnote]\n\n* Any time the base topic gets rerolled, I'd be the one who ends up\n  having to remember which other topics that did not rerolled depend\n  on it in what order and rebase them correctly, and then I have to\n  replace the follow-on topics that have been rerolled.  Even a\n  single missed subtopic will cause the day's integration work\n  redone, and all that robs time and concentration that the topics\n  by other contributors needs from me, which would make me grumpy\n  X-<.\n"}]}