{"thread":{"id":"56365","subject":"[PATCH 1/1] upload-pack: buffer ref advertisement writes","startedAt":"2021-08-24T14:03:34Z","lastAt":"2021-09-02T09:19:17Z","messageCount":27,"participants":["Jacob Vosmaer","Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"433553","messageId":"20210824140259.89332-1-jacob@gitlab.com","threadId":"56365","inReplyTo":null,"subject":"[PATCH 1/1] upload-pack: buffer ref advertisement writes","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-08-24T14:02:59Z","receivedAt":"2021-08-24T14:03:34Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"In both protocol v0 and v2, upload-pack writes one pktline packet per\nadvertised ref to stdout. That means one or two write(2) syscalls per\nref. This is problematic if these writes become network sends with\nhigh overhead.\n\nThis change adds a strbuf buffer to the send_ref callbacks of the v0\nref advertisement and v2's ls-refs. Instead of writing directly to\nstdout, send_ref now writes into the buffer, and then checks if there\nare more than 32768 bytes in the buffer. If so we flush the buffer to\nstdout.\n\nTo give an example: I set up a single-threaded loop that calls\nls-remote (with HTTP and protocol v2) on a local GitLab instance, on a\nrepository with 11K refs. When I switch from Git v2.32.0 to this\npatch, I see a 50% reduction in CPU time for Git, and 75% for Gitaly\n(GitLab's Git RPC service).\n\nSo having these buffers not only saves syscalls in upload-pack, it\nalso saves time in things that consume upload-pack's output.\n---\n ls-refs.c | 13 ++++++++++++-\n upload-pack.c | 18 +++++++++++++++---\n 2 files changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 88f6c3f60d..7b9d5813bf 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -7,6 +7,8 @@\n #include \"pkt-line.h\"\n #include \"config.h\"\n \n+#define OUT_WRITE_SIZE 32768\n+\n static int config_read;\n static int advertise_unborn;\n static int allow_unborn;\n@@ -65,6 +67,7 @@ struct ls_refs_data {\n \tunsigned peel;\n \tunsigned symrefs;\n \tstruct strvec prefixes;\n+\tstruct strbuf out;\n \tunsigned unborn : 1;\n };\n \n@@ -105,7 +108,12 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t}\n \n \tstrbuf_addch(&refline, '\\n');\n-\tpacket_write(1, refline.buf, refline.len);\n+\n+\tpacket_buf_write_len(&data->out, refline.buf, refline.len);\n+\tif (data->out.len >= OUT_WRITE_SIZE) {\n+\t\twrite_or_die(1, data->out.buf, data->out.len);\n+\t\tstrbuf_reset(&data->out);\n+\t}\n \n \tstrbuf_release(&refline);\n \treturn 0;\n@@ -145,6 +153,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.out, OUT_WRITE_SIZE);\n \n \tensure_config_read();\n \tgit_config(ls_refs_config, NULL);\n@@ -171,7 +180,9 @@ int ls_refs(struct repository *r, struct strvec *keys,\n \t\tstrvec_push(&data.prefixes, \"\");\n \tfor_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v,\n \t\t\t\t send_ref, &data, 0);\n+\twrite_or_die(1, data.out.buf, data.out.len);\n \tpacket_flush(1);\n+\tstrbuf_release(&data.out);\n \tstrvec_clear(&data.prefixes);\n \treturn 0;\n }\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 297b76fcb4..15f9aab4f6 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -42,6 +42,8 @@\n #define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \\\n \t\tNOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF)\n \n+#define SEND_REF_OUT_WRITE_SIZE 32768\n+\n /* Enum for allowed unadvertised object request (UOR) */\n enum allow_uor {\n \t/* Allow specifying sha1 if it is a ref tip. */\n@@ -58,6 +60,7 @@ enum allow_uor {\n */\n struct upload_pack_data {\n \tstruct string_list symref;\t\t\t\t/* v0 only */\n+\tstruct strbuf send_ref_out;\t\t\t\t/* v0 only */\n \tstruct object_array want_obj;\n \tstruct object_array have_obj;\n \tstruct oid_array haves;\t\t\t\t\t/* v2 only */\n@@ -126,6 +129,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tstruct string_list uri_protocols = STRING_LIST_INIT_DUP;\n \tstruct object_array extra_edge_obj = OBJECT_ARRAY_INIT;\n \tstruct string_list allowed_filters = STRING_LIST_INIT_DUP;\n+\tstruct strbuf send_ref_out = STRBUF_INIT;\n \n \tmemset(data, 0, sizeof(*data));\n \tdata->symref = symref;\n@@ -141,6 +145,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->allow_filter_fallback = 1;\n \tdata->tree_filter_max_depth = ULONG_MAX;\n \tpacket_writer_init(&data->writer, 1);\n+\tdata->send_ref_out = send_ref_out;\n \n \tdata->keepalive = 5;\n \tdata->advertise_sid = 0;\n@@ -158,6 +163,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tobject_array_clear(&data->extra_edge_obj);\n \tlist_objects_filter_release(&data->filter_options);\n \tstring_list_clear(&data->allowed_filters, 0);\n+\tstrbuf_release(&data->send_ref_out);\n \n \tfree((char *)data->pack_objects_hook);\n }\n@@ -1207,7 +1213,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \n \t\tformat_symref_info(&symref_info, &data->symref);\n \t\tformat_session_id(&session_id, data);\n-\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n+\t\tpacket_buf_write(&data->send_ref_out, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n \t\t\t (data->allow_uor & ALLOW_TIP_SHA1) ?\n@@ -1223,11 +1229,15 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\tstrbuf_release(&symref_info);\n \t\tstrbuf_release(&session_id);\n \t} else {\n-\t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n+\t\tpacket_buf_write(&data->send_ref_out, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n \tcapabilities = NULL;\n \tif (!peel_iterated_oid(oid, &peeled))\n-\t\tpacket_write_fmt(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+\t\tpacket_buf_write(&data->send_ref_out, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+\tif (data->send_ref_out.len >= SEND_REF_OUT_WRITE_SIZE) {\n+\t\twrite_or_die(1, data->send_ref_out.buf, data->send_ref_out.len);\n+\t\tstrbuf_reset(&data->send_ref_out);\n+\t}\n \treturn 0;\n }\n \n@@ -1346,8 +1356,10 @@ void upload_pack(struct upload_pack_options *options)\n \n \tif (options->advertise_refs || !data.stateless_rpc) {\n \t\treset_timeout(data.timeout);\n+\t\tstrbuf_grow(&data.send_ref_out, SEND_REF_OUT_WRITE_SIZE);\n \t\thead_ref_namespaced(send_ref, &data);\n \t\tfor_each_namespaced_ref(send_ref, &data);\n+\t\twrite_or_die(1, data.send_ref_out.buf, data.send_ref_out.len);\n \t\tadvertise_shallow_grafts(1);\n \t\tpacket_flush(1);\n \t} else {\n-- \n2.32.0\n\n"},{"id":"433621","messageId":"YSVfq9lZGdSRCcP9@nand.local","threadId":"56365","inReplyTo":"20210824140259.89332-1-jacob@gitlab.com","subject":"Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-24T21:07:55Z","receivedAt":"2021-08-24T21:07:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 24, 2021 at 04:02:59PM +0200, Jacob Vosmaer wrote:\n> In both protocol v0 and v2, upload-pack writes one pktline packet per\n> advertised ref to stdout. That means one or two write(2) syscalls per\n> ref. This is problematic if these writes become network sends with\n> high overhead.\n>\n> This change adds a strbuf buffer to the send_ref callbacks of the v0\n> ref advertisement and v2's ls-refs. Instead of writing directly to\n> stdout, send_ref now writes into the buffer, and then checks if there\n> are more than 32768 bytes in the buffer. If so we flush the buffer to\n> stdout.\n\nAre there any consumers that rely on having information early (where\nbuffering would be a detriment to them)?\n\n> To give an example: I set up a single-threaded loop that calls\n> ls-remote (with HTTP and protocol v2) on a local GitLab instance, on a\n> repository with 11K refs. When I switch from Git v2.32.0 to this\n> patch, I see a 50% reduction in CPU time for Git, and 75% for Gitaly\n> (GitLab's Git RPC service).\n\nHmm. Seeing a reduction in CPU time is surprising to me: do you have an\nexplanation for why the time-savings isn't coming purely from \"system\"\n(i.e., any blocking I/O)?\n\n> So having these buffers not only saves syscalls in upload-pack, it\n> also saves time in things that consume upload-pack's output.\n\nI don't think this explains it, since any time spent waiting on\nupload-pack output should be counted against the system bucket, not CPU\ntime.\n\n> ---\n> ls-refs.c | 13 ++++++++++++-\n> upload-pack.c | 18 +++++++++++++++---\n> 2 files changed, 27 insertions(+), 4 deletions(-)\n>\n> diff --git a/ls-refs.c b/ls-refs.c\n> index 88f6c3f60d..7b9d5813bf 100644\n> --- a/ls-refs.c\n> +++ b/ls-refs.c\n> @@ -7,6 +7,8 @@\n> #include \"pkt-line.h\"\n> #include \"config.h\"\n>\n> +#define OUT_WRITE_SIZE 32768\n> +\n> static int config_read;\n> static int advertise_unborn;\n> static int allow_unborn;\n> @@ -65,6 +67,7 @@ struct ls_refs_data {\n> \tunsigned peel;\n> \tunsigned symrefs;\n> \tstruct strvec prefixes;\n> +\tstruct strbuf out;\n> \tunsigned unborn : 1;\n> };\n>\n> @@ -105,7 +108,12 @@ static int send_ref(const char *refname, const struct object_id *oid,\n> \t}\n>\n> \tstrbuf_addch(&refline, '\\n');\n> -\tpacket_write(1, refline.buf, refline.len);\n> +\n> +\tpacket_buf_write_len(&data->out, refline.buf, refline.len);\n> +\tif (data->out.len >= OUT_WRITE_SIZE) {\n> +\t\twrite_or_die(1, data->out.buf, data->out.len);\n> +\t\tstrbuf_reset(&data->out);\n> +\t}\n\nNone of this looks wrong to me, but it might be nice to teach the\npacket writer a buffered mode that would handle this for us. It would be\nespecially nice to bolt the final flush onto packet_flush(), since it is\notherwise easy to forget to do.\n\nThanks,\nTaylor\n"},{"id":"433628","messageId":"xmqqczq2mseb.fsf@gitster.g","threadId":"56365","inReplyTo":"YSVfq9lZGdSRCcP9@nand.local","subject":"Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-24T21:42:20Z","receivedAt":"2021-08-24T21:42:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> @@ -105,7 +108,12 @@ static int send_ref(const char *refname, const struct object_id *oid,\n>> \t}\n>>\n>> \tstrbuf_addch(&refline, '\\n');\n>> -\tpacket_write(1, refline.buf, refline.len);\n>> +\n>> +\tpacket_buf_write_len(&data->out, refline.buf, refline.len);\n>> +\tif (data->out.len >= OUT_WRITE_SIZE) {\n>> +\t\twrite_or_die(1, data->out.buf, data->out.len);\n>> +\t\tstrbuf_reset(&data->out);\n\nThis is somewhat unexpected. When one introduces size limit, it is\nmore natural to make it upper bound and arrange the code more like\n\n\tif (currently buffered plus new data exceeds size limit) {\n\t\tflush the currently buffered data;\n\t\tif (new data alone exceeds size limit)\n\t\t\tflush the new data;\n\t\telse\n\t\t\tbuffer the new data;\n\t} else {\n\t\tbuffer the new data;\n\t}\n\nWhen a single packet exceeds the size limit, you'd end up making an\noversize write() call anyway, but otherwise it would keep the write\nbelow the limit, not slightly above the limit, which makes it much\neasier to justify why the limit exists at the level it is set.\n\nAlso, why is it 32k? Our jumbo packet limit is 65k, I think, and it\nmay probably be easier to explain if we matched it.\n\n>> +\t}\n>\n> None of this looks wrong to me, but it might be nice to teach the\n> packet writer a buffered mode that would handle this for us. It would be\n> especially nice to bolt the final flush onto packet_flush(), since it is\n> otherwise easy to forget to do.\n\nFWIW, the packet-line part of the system was from the beginning\nwritten with an eye to allow buffering until _flush() comes; we may\nhave added some buggy conversation path that deadlocks if we make\nthe non-flush packets fully buffered, so there may need some fixes,\nbut I do not expect the fallout would be too hard to diagnose.\n\nIt may be worth trying that avenue first before piling on the user\nlevel buffering like this patch does.\n\nThanks.\n"},{"id":"433682","messageId":"YSWSd5fMbSD5duOU@coredump.intra.peff.net","threadId":"56365","inReplyTo":"xmqqczq2mseb.fsf@gitster.g","subject":"Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-25T00:44:39Z","receivedAt":"2021-08-25T00:44:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 24, 2021 at 02:42:20PM -0700, Junio C Hamano wrote:\n\n> > None of this looks wrong to me, but it might be nice to teach the\n> > packet writer a buffered mode that would handle this for us. It would be\n> > especially nice to bolt the final flush onto packet_flush(), since it is\n> > otherwise easy to forget to do.\n> \n> FWIW, the packet-line part of the system was from the beginning\n> written with an eye to allow buffering until _flush() comes; we may\n> have added some buggy conversation path that deadlocks if we make\n> the non-flush packets fully buffered, so there may need some fixes,\n> but I do not expect the fallout would be too hard to diagnose.\n> \n> It may be worth trying that avenue first before piling on the user\n> level buffering like this patch does.\n\nYeah, I had the same thought. It also feels like this is a problem\nalready solved by stdio. I.e., a lot of the packet_* functions can\nhandle descriptors or strbufs. Why not \"FILE *\" handles?\n\nIt would probably involve using the original descriptor _and_ the\nfilehandle in some cases (e.g., ref advertisement over the handle, and\nthen muxing pack-objects output straight to the descriptor). But that's\nOK as long we are sensible about flushing at the right moments.\n\nIt may not be much less complex than just implementing buffering in the\npacket_* interfaces, though. The tricky part is likely to be the\ninterface (not itself, but avoiding repetition between all the\nfd/strbuf/buffered variants).\n\n-Peff\n"},{"id":"433817","messageId":"CADMWQoMpURczcnZne=0cr2vavoLm_VT5eEMg4FCu3VeSg_UJaQ@mail.gmail.com","threadId":"56365","inReplyTo":"YSWSd5fMbSD5duOU@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-08-26T10:02:26Z","receivedAt":"2021-08-26T10:02:40Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"> Are there any consumers that rely on having information early (where\n> buffering would be a detriment to them)?\n\nI think not.\n\n> Hmm. Seeing a reduction in CPU time is surprising to me: do you have an\n> explanation for why the time-savings isn't coming purely from \"system\"\n> (i.e., any blocking I/O)?\n\nPipe writes only block in the IO sense if the pipe buffer is full.\nOtherwise they seem to spend their time in spinlocks and copying into\nthe page buffer. In my benchmark, I don't believe the pipe buffer was\never full.\n\nI'm not an expert on the Linux kernel; you can see CPU flame graphs in\nhttps://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1257.\n\n> Yeah, I had the same thought. It also feels like this is a problem\n> already solved by stdio. I.e., a lot of the packet_* functions can\n> handle descriptors or strbufs. Why not \"FILE *\" handles?\n\nMy colleague Patrick Steinhardt (cc) made the same suggestion\noff-list. I'll post an alternative patch set to this thread.\n\nJacob\n\n\nOn Wed, Aug 25, 2021 at 2:44 AM Jeff King wrote:\n>\n> On Tue, Aug 24, 2021 at 02:42:20PM -0700, Junio C Hamano wrote:\n>\n> > > None of this looks wrong to me, but it might be nice to teach the\n> > > packet writer a buffered mode that would handle this for us. It would be\n> > > especially nice to bolt the final flush onto packet_flush(), since it is\n> > > otherwise easy to forget to do.\n> >\n> > FWIW, the packet-line part of the system was from the beginning\n> > written with an eye to allow buffering until _flush() comes; we may\n> > have added some buggy conversation path that deadlocks if we make\n> > the non-flush packets fully buffered, so there may need some fixes,\n> > but I do not expect the fallout would be too hard to diagnose.\n> >\n> > It may be worth trying that avenue first before piling on the user\n> > level buffering like this patch does.\n>\n> Yeah, I had the same thought. It also feels like this is a problem\n> already solved by stdio. I.e., a lot of the packet_* functions can\n> handle descriptors or strbufs. Why not \"FILE *\" handles?\n>\n> It would probably involve using the original descriptor _and_ the\n> filehandle in some cases (e.g., ref advertisement over the handle, and\n> then muxing pack-objects output straight to the descriptor). But that's\n> OK as long we are sensible about flushing at the right moments.\n>\n> It may not be much less complex than just implementing buffering in the\n> packet_* interfaces, though. The tricky part is likely to be the\n> interface (not itself, but avoiding repetition between all the\n> fd/strbuf/buffered variants).\n>\n> -Peff\n"},{"id":"433818","messageId":"20210826100648.10333-1-jacob@gitlab.com","threadId":"56365","inReplyTo":"CADMWQoMpURczcnZne=0cr2vavoLm_VT5eEMg4FCu3VeSg_UJaQ@mail.gmail.com","subject":"[PATCH 1/2] pkt-line: add packet_fwrite","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-08-26T10:06:47Z","receivedAt":"2021-08-26T10:09:45Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"This adds a new function packet_fwrite which works like packet_write,\nexcept it writes to a stdio stream.\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Jacob Vosmaer \n---\n cache.h | 1 +\n pkt-line.c | 16 ++++++++++++++++\n pkt-line.h | 1 +\n write-or-die.c | 6 ++++++\n 4 files changed, 24 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex bd4869beee..fbefe0927b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1736,6 +1736,7 @@ extern const char *git_mailmap_blob;\n void maybe_flush_or_die(FILE *, const char *);\n __attribute__((format (printf, 2, 3)))\n void fprintf_or_die(FILE *, const char *fmt, ...);\n+void fwrite_or_die(FILE *f, const void *buf, size_t count);\n \n #define COPY_READ_ERROR (-2)\n #define COPY_WRITE_ERROR (-3)\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 9f63eae2e6..244b326708 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -243,6 +243,22 @@ void packet_write(int fd_out, const char *buf, size_t size)\n \t\tdie(\"%s\", err.buf);\n }\n \n+void packet_fwrite(FILE *f, const char *buf, size_t size)\n+{\n+\tsize_t packet_size;\n+\tchar header[4];\n+\n+\tif (size > LARGE_PACKET_DATA_MAX)\n+\t\tdie(_(\"packet write failed - data exceeds max packet size\"));\n+\n+\tpacket_trace(buf, size, 1);\n+\tpacket_size = size + 4;\n+\n+\tset_packet_header(header, packet_size);\n+\tfwrite_or_die(f, header, 4);\n+\tfwrite_or_die(f, buf, size);\n+}\n+\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 5af5f45687..c9cb5e1719 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -28,6 +28,7 @@ void packet_buf_flush(struct strbuf *buf);\n void packet_buf_delim(struct strbuf *buf);\n void set_packet_header(char *buf, int size);\n void packet_write(int fd_out, const char *buf, size_t size);\n+void packet_fwrite(FILE *f, const char *buf, size_t size);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);\n int packet_flush_gently(int fd);\ndiff --git a/write-or-die.c b/write-or-die.c\nindex d33e68f6ab..d82ef54f90 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -70,3 +70,9 @@ void write_or_die(int fd, const void *buf, size_t count)\n \t\tdie_errno(\"write error\");\n \t}\n }\n+\n+void fwrite_or_die(FILE *f, const void *buf, size_t count)\n+{\n+\tif (fwrite(buf, count, 1, f) != 1)\n+\t\tdie_errno(\"write error\");\n+}\n-- \n2.32.0\n\n"},{"id":"433819","messageId":"20210826100648.10333-2-jacob@gitlab.com","threadId":"56365","inReplyTo":"20210826100648.10333-1-jacob@gitlab.com","subject":"[PATCH 2/2] upload-pack: use stdio in send_ref callbacks","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-08-26T10:06:48Z","receivedAt":"2021-08-26T10:09:57Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"In both protocol v0 and v2, upload-pack writes one pktline packet per\nadvertised ref to stdout. That means one or two write(2) syscalls per\nref. This is problematic if these writes become network sends with\nhigh overhead.\n\nThis commit changes both send_ref callbacks to use buffered IO using\nstdio. The default buffer size is usually 4KB, but with musl libc it\nis only 1KB. So for consistent results we need to set a buffer size\nourselves. During startup of upload-pack, we set the stdout buffer\nsize to LARGE_PACKET_MAX, i.e. just shy of 64KB.\n\nTo give an example of the impact: I set up a single-threaded loop that\ncalls ls-remote (with HTTP and protocol v2) on a local GitLab\ninstance, on a repository with 11K refs. When I switch from Git\nv2.32.0 to this patch, I see a 50% reduction in CPU time for Git, and\n75% for Gitaly (GitLab's Git RPC service).\n\nSo using buffered IO not only saves syscalls in upload-pack, it also\nsaves time in things that consume upload-pack's output.\n\nSigned-off-by: Jacob Vosmaer \n---\n builtin/upload-pack.c | 10 ++++++++++\n ls-refs.c | 5 ++++-\n upload-pack.c | 15 ++++++++++++---\n 3 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/upload-pack.c b/builtin/upload-pack.c\nindex 6da8fa2607..8033f84124 100644\n--- a/builtin/upload-pack.c\n+++ b/builtin/upload-pack.c\n@@ -48,6 +48,16 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \tif (!enter_repo(dir, strict))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n \n+\t/*\n+\t * Increase the stdio buffer size for stdout, for the benefit of ref\n+\t * advertisement writes. We are only allowed to call setvbuf(3) \"after\n+\t * opening a stream and before any other operations have been performed\n+\t * on it\", so let's call it before we have written anything to stdout.\n+\t */\n+\tif (setvbuf(stdout, xmalloc(LARGE_PACKET_MAX), _IOFBF,\n+\t\t\tLARGE_PACKET_MAX))\n+\t\tdie_errno(\"failed to grow stdout buffer\");\n+\n \tswitch (determine_protocol_version_server()) {\n \tcase protocol_v2:\n \t\tserve_opts.advertise_capabilities = opts.advertise_refs;\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 88f6c3f60d..83f2948fc3 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -105,7 +105,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t}\n \n \tstrbuf_addch(&refline, '\\n');\n-\tpacket_write(1, refline.buf, refline.len);\n+\tpacket_fwrite(stdout, refline.buf, refline.len);\n \n \tstrbuf_release(&refline);\n \treturn 0;\n@@ -171,6 +171,9 @@ int ls_refs(struct repository *r, struct strvec *keys,\n \t\tstrvec_push(&data.prefixes, \"\");\n \tfor_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v,\n \t\t\t\t send_ref, &data, 0);\n+\t/* Call fflush because send_ref uses stdio. */\n+\tif (fflush(stdout))\n+\t\tdie_errno(_(\"write failure on standard output\"));\n \tpacket_flush(1);\n \tstrvec_clear(&data.prefixes);\n \treturn 0;\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 297b76fcb4..b592ac6cfb 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -58,6 +58,7 @@ enum allow_uor {\n */\n struct upload_pack_data {\n \tstruct string_list symref;\t\t\t\t/* v0 only */\n+\tstruct strbuf send_ref_buf;\t\t\t\t/* v0 only */\n \tstruct object_array want_obj;\n \tstruct object_array have_obj;\n \tstruct oid_array haves;\t\t\t\t\t/* v2 only */\n@@ -126,6 +127,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tstruct string_list uri_protocols = STRING_LIST_INIT_DUP;\n \tstruct object_array extra_edge_obj = OBJECT_ARRAY_INIT;\n \tstruct string_list allowed_filters = STRING_LIST_INIT_DUP;\n+\tstruct strbuf send_ref_buf = STRBUF_INIT;\n \n \tmemset(data, 0, sizeof(*data));\n \tdata->symref = symref;\n@@ -141,6 +143,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->allow_filter_fallback = 1;\n \tdata->tree_filter_max_depth = ULONG_MAX;\n \tpacket_writer_init(&data->writer, 1);\n+\tdata->send_ref_buf = send_ref_buf;\n \n \tdata->keepalive = 5;\n \tdata->advertise_sid = 0;\n@@ -158,6 +161,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tobject_array_clear(&data->extra_edge_obj);\n \tlist_objects_filter_release(&data->filter_options);\n \tstring_list_clear(&data->allowed_filters, 0);\n+\tstrbuf_release(&data->send_ref_buf);\n \n \tfree((char *)data->pack_objects_hook);\n }\n@@ -1201,13 +1205,14 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tif (mark_our_ref(refname_nons, refname, oid))\n \t\treturn 0;\n \n+\tstrbuf_reset(&data->send_ref_buf);\n \tif (capabilities) {\n \t\tstruct strbuf symref_info = STRBUF_INIT;\n \t\tstruct strbuf session_id = STRBUF_INIT;\n \n \t\tformat_symref_info(&symref_info, &data->symref);\n \t\tformat_session_id(&session_id, data);\n-\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n+\t\tpacket_buf_write(&data->send_ref_buf, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n \t\t\t (data->allow_uor & ALLOW_TIP_SHA1) ?\n@@ -1223,11 +1228,12 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\tstrbuf_release(&symref_info);\n \t\tstrbuf_release(&session_id);\n \t} else {\n-\t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n+\t\tpacket_buf_write(&data->send_ref_buf, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n \tcapabilities = NULL;\n \tif (!peel_iterated_oid(oid, &peeled))\n-\t\tpacket_write_fmt(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+\t\tpacket_buf_write(&data->send_ref_buf, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+\tfwrite_or_die(stdout, data->send_ref_buf.buf, data->send_ref_buf.len);\n \treturn 0;\n }\n \n@@ -1348,6 +1354,9 @@ void upload_pack(struct upload_pack_options *options)\n \t\treset_timeout(data.timeout);\n \t\thead_ref_namespaced(send_ref, &data);\n \t\tfor_each_namespaced_ref(send_ref, &data);\n+\t\t/* Call fflush because send_ref uses stdio. */\n+\t\tif (fflush(stdout))\n+\t\t\tdie_errno(_(\"write failure on standard output\"));\n \t\tadvertise_shallow_grafts(1);\n \t\tpacket_flush(1);\n \t} else {\n-- \n2.32.0\n\n"},{"id":"433844","messageId":"xmqqpmu0f9ob.fsf@gitster.g","threadId":"56365","inReplyTo":"20210826100648.10333-2-jacob@gitlab.com","subject":"Re: [PATCH 2/2] upload-pack: use stdio in send_ref callbacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T16:33:08Z","receivedAt":"2021-08-26T16:33:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Vosmaer writes:\n\n> In both protocol v0 and v2, upload-pack writes one pktline packet per\n> advertised ref to stdout. That means one or two write(2) syscalls per\n> ref. This is problematic if these writes become network sends with\n> high overhead.\n>\n> This commit changes both send_ref callbacks to use buffered IO using\n> stdio. The default buffer size is usually 4KB, but with musl libc it\n> is only 1KB. So for consistent results we need to set a buffer size\n> ourselves. During startup of upload-pack, we set the stdout buffer\n> size to LARGE_PACKET_MAX, i.e. just shy of 64KB.\n>\n> To give an example of the impact: I set up a single-threaded loop that\n> calls ls-remote (with HTTP and protocol v2) on a local GitLab\n> instance, on a repository with 11K refs. When I switch from Git\n> v2.32.0 to this patch, I see a 50% reduction in CPU time for Git, and\n> 75% for Gitaly (GitLab's Git RPC service).\n>\n> So using buffered IO not only saves syscalls in upload-pack, it also\n> saves time in things that consume upload-pack's output.\n>\n> Signed-off-by: Jacob Vosmaer \n> ---\n\nNicely explained.\n\n> +\t/*\n> +\t * Increase the stdio buffer size for stdout, for the benefit of ref\n> +\t * advertisement writes. We are only allowed to call setvbuf(3) \"after\n> +\t * opening a stream and before any other operations have been performed\n> +\t * on it\", so let's call it before we have written anything to stdout.\n> +\t */\n> +\tif (setvbuf(stdout, xmalloc(LARGE_PACKET_MAX), _IOFBF,\n> +\t\t\tLARGE_PACKET_MAX))\n> +\t\tdie_errno(\"failed to grow stdout buffer\");\n\nNice to see a comment on the tricky part. I do not think we mind if\nwe rounded up the allocation size to the next power of two here, but\nthere probably won't be any measurable benefit for doing so.\n\n> diff --git a/ls-refs.c b/ls-refs.c\n> index 88f6c3f60d..83f2948fc3 100644\n> --- a/ls-refs.c\n> +++ b/ls-refs.c\n> @@ -105,7 +105,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n> \t}\n> \n> \tstrbuf_addch(&refline, '\\n');\n> -\tpacket_write(1, refline.buf, refline.len);\n> +\tpacket_fwrite(stdout, refline.buf, refline.len);\n> \n> \tstrbuf_release(&refline);\n> \treturn 0;\n> @@ -171,6 +171,9 @@ int ls_refs(struct repository *r, struct strvec *keys,\n> \t\tstrvec_push(&data.prefixes, \"\");\n> \tfor_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v,\n> \t\t\t\t send_ref, &data, 0);\n> +\t/* Call fflush because send_ref uses stdio. */\n> +\tif (fflush(stdout))\n> +\t\tdie_errno(_(\"write failure on standard output\"));\n\nThere is maybe_flush_or_die() helper that does a bit too much (I do\nnot think this code path needs to worry about GIT_FLUSH) but does\ncall check_pipe(errno) like packet_write_fmt() does upon seeing a\nwrite failure.\n\n> \tpacket_flush(1);\n\nI briefly wondered if all the operations on refline can be turned\ninto direct operations on stdout, but the answer obviously is no.\nWe still need to have a strbuf to assemble a single packet and\nmeasure its final length before we can send it out to stdout.\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 297b76fcb4..b592ac6cfb 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -58,6 +58,7 @@ enum allow_uor {\n> */\n> struct upload_pack_data {\n> \tstruct string_list symref;\t\t\t\t/* v0 only */\n> +\tstruct strbuf send_ref_buf;\t\t\t\t/* v0 only */\n> \tstruct object_array want_obj;\n> \tstruct object_array have_obj;\n> \tstruct oid_array haves;\t\t\t\t\t/* v2 only */\n> @@ -126,6 +127,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n> \tstruct string_list uri_protocols = STRING_LIST_INIT_DUP;\n> \tstruct object_array extra_edge_obj = OBJECT_ARRAY_INIT;\n> \tstruct string_list allowed_filters = STRING_LIST_INIT_DUP;\n> +\tstruct strbuf send_ref_buf = STRBUF_INIT;\n\nIIUC, the most notable difference between this round and the\nprevious one is that now we are no longer buffering more than one\npacket worth of data because we let the stdio to accumulate them.\nI was a bit surprised that we still want to have a strbuf inside\nthis structure (which is there only because it wants to persist\nduring the whole conversation with the other side).\n\nAhh, sorry, scratch that. I do remember the discussion/patch that\nit was hurting to make calls to strbuf-init/strbuf-release once per\nref, and it is an easy way to have the structure here to reuse it.\n\nOK. This step looks quite sensible, other than the same \"do we want\ncheck_pipe() before dying upon fflush() failure?\" we see in the last\nhunk below. I didn't mention this in the review of 1/2, but the new\nfwrite_or_die() helper function may also have the same issue.\n\nThanks.\n\n> @@ -1348,6 +1354,9 @@ void upload_pack(struct upload_pack_options *options)\n> \t\treset_timeout(data.timeout);\n> \t\thead_ref_namespaced(send_ref, &data);\n> \t\tfor_each_namespaced_ref(send_ref, &data);\n> +\t\t/* Call fflush because send_ref uses stdio. */\n> +\t\tif (fflush(stdout))\n> +\t\t\tdie_errno(_(\"write failure on standard output\"));\n> \t\tadvertise_shallow_grafts(1);\n> \t\tpacket_flush(1);\n> \t} else {\n"},{"id":"433845","messageId":"xmqqilzsf9nh.fsf@gitster.g","threadId":"56365","inReplyTo":"20210826100648.10333-1-jacob@gitlab.com","subject":"Re: [PATCH 1/2] pkt-line: add packet_fwrite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T16:33:38Z","receivedAt":"2021-08-26T16:33:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Vosmaer writes:\n\n> This adds a new function packet_fwrite which works like packet_write,\n> except it writes to a stdio stream.\n>\n> Helped-by: Patrick Steinhardt \n> Signed-off-by: Jacob Vosmaer \n> ---\n> cache.h | 1 +\n> pkt-line.c | 16 ++++++++++++++++\n> pkt-line.h | 1 +\n> write-or-die.c | 6 ++++++\n> 4 files changed, 24 insertions(+)\n\n\nI wonder if our readers are all proficient enough not to require\nsome comment to warn them about the care that must be taken when\npacket_fwrite() and packet_write() are mixed together (i.e. stdio\nflushing, etc.), but other than that, this sounds quite\nstraight-forward.\n\n"},{"id":"433868","messageId":"xmqqv93sc5z2.fsf@gitster.g","threadId":"56365","inReplyTo":"xmqqpmu0f9ob.fsf@gitster.g","subject":"Re: [PATCH 2/2] upload-pack: use stdio in send_ref callbacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T20:21:21Z","receivedAt":"2021-08-26T20:21:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> IIUC, the most notable difference between this round and the\n> previous one is that now we are no longer buffering more than one\n> packet worth of data because we let the stdio to accumulate them.\n> I was a bit surprised that we still want to have a strbuf inside\n> this structure (which is there only because it wants to persist\n> during the whole conversation with the other side).\n>\n> Ahh, sorry, scratch that. I do remember the discussion/patch that\n> it was hurting to make calls to strbuf-init/strbuf-release once per\n> ref, and it is an easy way to have the structure here to reuse it.\n\nBut that means this majorly overlaps what Patrick is already doing\nin his ps/ls-refs-strbuf-optim topic. Perhaps these should be\nrebased on top of that topic branch, I wonder?\n"},{"id":"433887","messageId":"YSgXKgHgPwo/8aUW@nand.local","threadId":"56365","inReplyTo":"xmqqpmu0f9ob.fsf@gitster.g","subject":"Re: [PATCH 2/2] upload-pack: use stdio in send_ref callbacks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-26T22:35:22Z","receivedAt":"2021-08-26T22:35:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 26, 2021 at 09:33:08AM -0700, Junio C Hamano wrote:\n> > @@ -171,6 +171,9 @@ int ls_refs(struct repository *r, struct strvec *keys,\n> > \t\tstrvec_push(&data.prefixes, \"\");\n> > \tfor_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v,\n> > \t\t\t\t send_ref, &data, 0);\n> > +\t/* Call fflush because send_ref uses stdio. */\n> > +\tif (fflush(stdout))\n> > +\t\tdie_errno(_(\"write failure on standard output\"));\n>\n> There is maybe_flush_or_die() helper that does a bit too much (I do\n> not think this code path needs to worry about GIT_FLUSH) but does\n> call check_pipe(errno) like packet_write_fmt() does upon seeing a\n> write failure.\n>\n> > \tpacket_flush(1);\n\nI was somewhat surprised to see the fflush call here and not in a\ncompanion to the existing packet_flush (when working with stdio instead\nof file descriptors, of course).\n\nWhat Jacob wrote is not wrong, of course, but I think having\npacket_fflush() or similar would be less error-prone.\n\n> OK. This step looks quite sensible, other than the same \"do we want\n> check_pipe() before dying upon fflush() failure?\" we see in the last\n> hunk below. I didn't mention this in the review of 1/2, but the new\n> fwrite_or_die() helper function may also have the same issue.\n\nAgreed.\n\nThanks,\nTaylor\n"},{"id":"433892","messageId":"YSgiwXFnLrLy6pH3@coredump.intra.peff.net","threadId":"56365","inReplyTo":"xmqqpmu0f9ob.fsf@gitster.g","subject":"Re: [PATCH 2/2] upload-pack: use stdio in send_ref callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-26T23:24:49Z","receivedAt":"2021-08-26T23:24:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 26, 2021 at 09:33:08AM -0700, Junio C Hamano wrote:\n\n> > +\t/*\n> > +\t * Increase the stdio buffer size for stdout, for the benefit of ref\n> > +\t * advertisement writes. We are only allowed to call setvbuf(3) \"after\n> > +\t * opening a stream and before any other operations have been performed\n> > +\t * on it\", so let's call it before we have written anything to stdout.\n> > +\t */\n> > +\tif (setvbuf(stdout, xmalloc(LARGE_PACKET_MAX), _IOFBF,\n> > +\t\t\tLARGE_PACKET_MAX))\n> > +\t\tdie_errno(\"failed to grow stdout buffer\");\n> \n> Nice to see a comment on the tricky part. I do not think we mind if\n> we rounded up the allocation size to the next power of two here, but\n> there probably won't be any measurable benefit for doing so.\n\nI'm a little negative on this part, actually. The commit message claims\nit's for consistency across platforms. But I would argue that if your\nlibc buffer size is sub-optimal, then we shouldn't be sprinkling these\nadjustments through the code. Either:\n\n - we should consider it a quality-of-implementation issue, and people\n using that libc should push back on their platform to change the\n default size; or\n\n - we should fix it consistently and transparently throughout Git by\n adjusting std{in,out,err} in common-main, and using fopen()/fdopen()\n wrappers to adjust to something more sensible\n\nI also find the use of LARGE_PACKET_MAX weird here. Is that really the\noptimal stdio buffer size? The whole point of this is coalescing _small_\npackets into a single write() syscall. So how many small packets fit\ninto LARGE_PACKET_MAX is comparing apples and oranges.\n\n> > \tpacket_flush(1);\n> \n> I briefly wondered if all the operations on refline can be turned\n> into direct operations on stdout, but the answer obviously is no.\n> We still need to have a strbuf to assemble a single packet and\n> measure its final length before we can send it out to stdout.\n\nI think we could push quite a bit more down into the pkt-line code by\nmaking a few more operations FILE-aware. E.g., the patch below shows an\nalternate version of patch 2, where we have more helpers and the changes\nto the caller are much smaller.\n\nThe proliferation of packet_write_fmt() vs packet_fwrite_fmt() is a\nlittle ugly, but:\n\n - I didn't bother to factor out commonality for this proof-of-concept.\n I think we could be sharing a bit more code in pkt-line.c, as noted.\n\n - This would all be much nicer if we refactored upload-pack to use\n the packet_writer interface. And then the caller sets up the \"FILE\n vs descriptor\" choice once when creating the writer, and all of the\n code it calls doesn't have to care either way.\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 88f6c3f60d..e6a2dbd962 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -105,7 +105,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t}\n \n \tstrbuf_addch(&refline, '\\n');\n-\tpacket_write(1, refline.buf, refline.len);\n+\tpacket_fwrite(stdout, refline.buf, refline.len);\n \n \tstrbuf_release(&refline);\n \treturn 0;\n@@ -171,7 +171,7 @@ int ls_refs(struct repository *r, struct strvec *keys,\n \t\tstrvec_push(&data.prefixes, \"\");\n \tfor_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v,\n \t\t\t\t send_ref, &data, 0);\n-\tpacket_flush(1);\n+\tpacket_fflush(stdout);\n \tstrvec_clear(&data.prefixes);\n \treturn 0;\n }\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 244b326708..12cf09804b 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -92,6 +92,14 @@ void packet_flush(int fd)\n \t\tdie_errno(_(\"unable to write flush packet\"));\n }\n \n+void packet_fflush(FILE *fh)\n+{\n+\tpacket_trace(\"0000\", 4, 1);\n+\tfwrite_or_die(fh, \"0000\", 4);\n+\tif (fflush(fh))\n+\t\tdie_errno(_(\"unable to flush packet stream\"));\n+}\n+\n void packet_delim(int fd)\n {\n \tpacket_trace(\"0001\", 4, 1);\n@@ -194,6 +202,21 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)\n \treturn status;\n }\n \n+/* this could be refactored to share code with packet_write_fmt_1 */\n+void packet_fwrite_fmt(FILE *fh, const char *fmt, ...)\n+{\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\tva_list args;\n+\n+\tstrbuf_reset(&buf);\n+\n+\tva_start(args, fmt);\n+\tformat_packet(&buf, \"\", fmt, args);\n+\tva_end(args);\n+\n+\tfwrite_or_die(fh, buf.buf, buf.len);\n+}\n+\n static int do_packet_write(const int fd_out, const char *buf, size_t size,\n \t\t\t struct strbuf *err)\n {\ndiff --git a/pkt-line.h b/pkt-line.h\nindex c9cb5e1719..7899f8d8e1 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -21,9 +21,11 @@\n * side can't, we stay with pure read/write interfaces.\n */\n void packet_flush(int fd);\n+void packet_fflush(FILE *fh);\n void packet_delim(int fd);\n void packet_response_end(int fd);\n void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n+void packet_fwrite_fmt(FILE *fh, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_flush(struct strbuf *buf);\n void packet_buf_delim(struct strbuf *buf);\n void set_packet_header(char *buf, int size);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 297b76fcb4..7c98c04d73 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1207,7 +1207,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \n \t\tformat_symref_info(&symref_info, &data->symref);\n \t\tformat_session_id(&session_id, data);\n-\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n+\t\tpacket_fwrite_fmt(stdout, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n \t\t\t (data->allow_uor & ALLOW_TIP_SHA1) ?\n@@ -1223,11 +1223,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\tstrbuf_release(&symref_info);\n \t\tstrbuf_release(&session_id);\n \t} else {\n-\t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n+\t\tpacket_fwrite_fmt(stdout, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n \tcapabilities = NULL;\n \tif (!peel_iterated_oid(oid, &peeled))\n-\t\tpacket_write_fmt(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+\t\tpacket_fwrite_fmt(stdout, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n \treturn 0;\n }\n \n@@ -1348,6 +1348,14 @@ void upload_pack(struct upload_pack_options *options)\n \t\treset_timeout(data.timeout);\n \t\thead_ref_namespaced(send_ref, &data);\n \t\tfor_each_namespaced_ref(send_ref, &data);\n+\t\t/*\n+\t\t * sadly we still have to fflush manually here, because\n+\t\t * advertise_shallow_grafts() uses the descriptor directly\n+\t\t * before we could call packet_fflush(). But isn't that a good\n+\t\t * reason to convert that function to stdio, too?\n+\t\t */\n+\t\tif (fflush(stdout))\n+\t\t\tdie_errno(_(\"write failure on standard output\"));\n \t\tadvertise_shallow_grafts(1);\n \t\tpacket_flush(1);\n \t} else {\n\n> > +\tstruct strbuf send_ref_buf = STRBUF_INIT;\n> \n> IIUC, the most notable difference between this round and the\n> previous one is that now we are no longer buffering more than one\n> packet worth of data because we let the stdio to accumulate them.\n> I was a bit surprised that we still want to have a strbuf inside\n> this structure (which is there only because it wants to persist\n> during the whole conversation with the other side).\n> \n> Ahh, sorry, scratch that. I do remember the discussion/patch that\n> it was hurting to make calls to strbuf-init/strbuf-release once per\n> ref, and it is an easy way to have the structure here to reuse it.\n\nYou'll note in my patch above that we format into a static strbuf to\navoid this cost. Perhaps a little ugly, but that's exactly what the\nexisting packet_write_fmt() code is doing!\n\nIf we want to get rid of that, I think there are two options:\n\n - for stdio handles, we don't ever need the fully-formatted packet. We\n can get the formatted size with a too-small snprintf(), and then\n directly fprintf() out the result (this doesn't work with\n descriptors, because we have to format the result to feed it to\n write()).\n\n - if this were all using packet_writer, it could easily learn to\n heap-allocate a single buffer for repeated use. That's roughly the\n same as what's happening in this series, but it would all be taken\n care of by the pkt-line code.\n\n-Peff\n"},{"id":"433893","messageId":"YSgkgfCr6/kIGzWX@coredump.intra.peff.net","threadId":"56365","inReplyTo":"20210826100648.10333-2-jacob@gitlab.com","subject":"Re: [PATCH 2/2] upload-pack: use stdio in send_ref callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-26T23:32:17Z","receivedAt":"2021-08-26T23:32:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 26, 2021 at 12:06:48PM +0200, Jacob Vosmaer wrote:\n\n> @@ -126,6 +127,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n> \tstruct string_list uri_protocols = STRING_LIST_INIT_DUP;\n> \tstruct object_array extra_edge_obj = OBJECT_ARRAY_INIT;\n> \tstruct string_list allowed_filters = STRING_LIST_INIT_DUP;\n> +\tstruct strbuf send_ref_buf = STRBUF_INIT;\n> \n> \tmemset(data, 0, sizeof(*data));\n> \tdata->symref = symref;\n> @@ -141,6 +143,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n> \tdata->allow_filter_fallback = 1;\n> \tdata->tree_filter_max_depth = ULONG_MAX;\n> \tpacket_writer_init(&data->writer, 1);\n> +\tdata->send_ref_buf = send_ref_buf;\n\nThis does a struct copy of the strbuf, which is usually a bad thing\n(both copies think they own the pointer). It works here because the\noriginal immediately goes out of scope. The usual thing would be to do\nthis instead:\n\n strbuf_init(&data->send_ref_buf, 0);\n\nbut I notice this whole function is somewhat odd that way (lots of\nstatic initializers followed by struct assignment, rather than using\ninitialization functions).\n\nI'm not sure if it's worth changing or not. Again, I don't think it's\ndoing the wrong thing, but just sort of unusual for our code base. A few\nof the data structures in use here don't have _init() functions\n(object_array and oid_array), but that probably means we ought to add\nthem.\n\n-Peff\n"},{"id":"433935","messageId":"xmqqbl5ic19t.fsf@gitster.g","threadId":"56365","inReplyTo":"YSgiwXFnLrLy6pH3@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] upload-pack: use stdio in send_ref callbacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-27T16:15:10Z","receivedAt":"2021-08-27T16:15:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Aug 26, 2021 at 09:33:08AM -0700, Junio C Hamano wrote:\n>\n>> > +\t/*\n>> > +\t * Increase the stdio buffer size for stdout, for the benefit of ref\n>> > +\t * advertisement writes. We are only allowed to call setvbuf(3) \"after\n>> > +\t * opening a stream and before any other operations have been performed\n>> > +\t * on it\", so let's call it before we have written anything to stdout.\n>> > +\t */\n>> > +\tif (setvbuf(stdout, xmalloc(LARGE_PACKET_MAX), _IOFBF,\n>> > +\t\t\tLARGE_PACKET_MAX))\n>> > +\t\tdie_errno(\"failed to grow stdout buffer\");\n>> \n>> Nice to see a comment on the tricky part. I do not think we mind if\n>> we rounded up the allocation size to the next power of two here, but\n>> there probably won't be any measurable benefit for doing so.\n>\n> I'm a little negative on this part, actually. The commit message claims\n> it's for consistency across platforms. But I would argue that if your\n> libc buffer size is sub-optimal, then we shouldn't be sprinkling these\n> adjustments through the code. Either:\n>\n> - we should consider it a quality-of-implementation issue, and people\n> using that libc should push back on their platform to change the\n> default size; or\n>\n> - we should fix it consistently and transparently throughout Git by\n> adjusting std{in,out,err} in common-main, and using fopen()/fdopen()\n> wrappers to adjust to something more sensible\n\nI agree that it would be ideal if we didn't do setvbuf() at all, the\nsecond best would be to do so at a central place. My \"Nice\" applied\nonly to the comment ;-)\n\n> I also find the use of LARGE_PACKET_MAX weird here. Is that really the\n> optimal stdio buffer size? The whole point of this is coalescing _small_\n> packets into a single write() syscall. So how many small packets fit\n> into LARGE_PACKET_MAX is comparing apples and oranges.\n\nThe sizing is all my fault. The original used 32k threashold and\nimplemented manual buffering by flushing whenever the accumulated\ndata exceeded the threashold, as opposed to \"if we buffer this new\npiece, it would exceed, so let's flush first what we got so far,\nwhich is smaller than the threashold\", which I found indefensible in\ntwo ways. The \"flush _after_ we go over it\" semantics looked iffy,\nand 32k was totally out of thin air. As LARGE_PACKET_MAX is the\nhard upper limit of each packet that has been with us forever, it\nwas more defensible than 32k ;-)\n\nBut if we are using stdio, I agree that it is much better not to\nworry about sizing at all by not doing setvbuf() and leaving it to\nlibc implementation. They ought to know what works on their\nplatform the best.\n\nThanks.\n"},{"id":"434253","messageId":"20210831093444.28199-1-jacob@gitlab.com","threadId":"56365","inReplyTo":"xmqqbl5ic19t.fsf@gitster.g","subject":"[PATCH v3 0/2] send_ref buffering","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-08-31T09:34:42Z","receivedAt":"2021-08-31T09:35:23Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Changes compared to v2:\n- remove setvbuf call\n- add packet_fwrite_fmt\n- add packet_fflush\n\nNon-changes:\n- no ferror calls because those are for reads, not for writes\n\nThanks for the reactions everyone. I agree that packet_fwrite_fmt\nsimplifies the patch nicely. Jeff, I hope I have given you credit\nin an appropriate way, let me know if you want me to change something\nthere.\n\nRegarding setvbuf: I have found out that GNU coreutils has a utility\ncalled stdbuf that lets you modify the stdout buffer size at runtime\nusing some LD_PRELOAD hack so we can use that in Gitaly. I don't\nthink this is the best outcome for users, we ought to give them a\ngood default instead of expecting them to invoke git-upload-pack\nas 'stdbuf -o 64K git-upload-pack'. But I can't judge the impact\nof globally changing the stdout buffer size for Git so I'll settle\nfor having to use stdbuf.\n\nJacob Vosmaer (2):\n pkt-line: add packet_fwrite and packet_fwrite_fmt\n upload-pack: use stdio in send_ref callbacks\n\n cache.h | 2 ++\n ls-refs.c | 4 +++-\n pkt-line.c | 30 ++++++++++++++++++++++++++++++\n pkt-line.h | 8 ++++++++\n upload-pack.c | 8 +++++---\n write-or-die.c | 12 ++++++++++++\n 6 files changed, 60 insertions(+), 4 deletions(-)\n\n-- \n2.32.0\n\n"},{"id":"434254","messageId":"20210831093444.28199-2-jacob@gitlab.com","threadId":"56365","inReplyTo":"20210831093444.28199-1-jacob@gitlab.com","subject":"[PATCH v3 1/2] pkt-line: add stdio packet write functions","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-08-31T09:34:43Z","receivedAt":"2021-08-31T09:35:29Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"This adds three new functions to pkt-line.c: packet_fwrite,\npacket_fwrite_fmt and packet_fflush. Besides writing a pktline flush\npacket, packet_fflush also flushes the stdio buffer of the stream.\n\nHelped-by: Patrick Steinhardt \nHelped-by: Jeff King \nSigned-off-by: Jacob Vosmaer \n---\n cache.h | 2 ++\n pkt-line.c | 37 +++++++++++++++++++++++++++++++++++++\n pkt-line.h | 11 +++++++++++\n write-or-die.c | 12 ++++++++++++\n 4 files changed, 62 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex bd4869beee..dcf2454c3b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1736,6 +1736,8 @@ extern const char *git_mailmap_blob;\n void maybe_flush_or_die(FILE *, const char *);\n __attribute__((format (printf, 2, 3)))\n void fprintf_or_die(FILE *, const char *fmt, ...);\n+void fwrite_or_die(FILE *f, const void *buf, size_t count);\n+void fflush_or_die(FILE *f);\n \n #define COPY_READ_ERROR (-2)\n #define COPY_WRITE_ERROR (-3)\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 9f63eae2e6..de4a94b437 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -243,6 +243,43 @@ void packet_write(int fd_out, const char *buf, size_t size)\n \t\tdie(\"%s\", err.buf);\n }\n \n+void packet_fwrite(FILE *f, const char *buf, size_t size)\n+{\n+\tsize_t packet_size;\n+\tchar header[4];\n+\n+\tif (size > LARGE_PACKET_DATA_MAX)\n+\t\tdie(_(\"packet write failed - data exceeds max packet size\"));\n+\n+\tpacket_trace(buf, size, 1);\n+\tpacket_size = size + 4;\n+\n+\tset_packet_header(header, packet_size);\n+\tfwrite_or_die(f, header, 4);\n+\tfwrite_or_die(f, buf, size);\n+}\n+\n+void packet_fwrite_fmt(FILE *fh, const char *fmt, ...)\n+{\n+ static struct strbuf buf = STRBUF_INIT;\n+ va_list args;\n+\n+ strbuf_reset(&buf);\n+\n+ va_start(args, fmt);\n+ format_packet(&buf, \"\", fmt, args);\n+ va_end(args);\n+\n+ fwrite_or_die(fh, buf.buf, buf.len);\n+}\n+\n+void packet_fflush(FILE *f)\n+{\n+\tpacket_trace(\"0000\", 4, 1);\n+\tfwrite_or_die(f, \"0000\", 4);\n+\tfflush_or_die(f);\n+}\n+\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 5af5f45687..0a38a68d15 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -35,6 +35,17 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format\n int write_packetized_from_fd_no_flush(int fd_in, int fd_out);\n int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_out);\n \n+/*\n+ * Stdio versions of packet_write functions. When mixing these with fd\n+ * based functions, take care to call fflush or packet_fflush before\n+ * doing fd writes or closing the fd.\n+ */\n+void packet_fwrite(FILE *f, const char *buf, size_t size);\n+void packet_fwrite_fmt(FILE *f, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n+\n+/* packet_fflush writes a flush packet and flushes the stdio buffer of f */\n+void packet_fflush(FILE *f);\n+\n /*\n * Read a packetized line into the buffer, which must be at least size bytes\n * long. The return value specifies the number of bytes read into the buffer.\ndiff --git a/write-or-die.c b/write-or-die.c\nindex d33e68f6ab..7a2f84e2ee 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -70,3 +70,15 @@ void write_or_die(int fd, const void *buf, size_t count)\n \t\tdie_errno(\"write error\");\n \t}\n }\n+\n+void fwrite_or_die(FILE *f, const void *buf, size_t count)\n+{\n+\tif (fwrite(buf, count, 1, f) != 1)\n+\t\tdie_errno(\"fwrite error\");\n+}\n+\n+void fflush_or_die(FILE *f)\n+{\n+\tif (fflush(f))\n+\t\tdie_errno(\"fflush error\");\n+}\n-- \n2.32.0\n\n"},{"id":"434255","messageId":"20210831093444.28199-3-jacob@gitlab.com","threadId":"56365","inReplyTo":"20210831093444.28199-1-jacob@gitlab.com","subject":"[PATCH v3 2/2] upload-pack: use stdio in send_ref callbacks","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-08-31T09:34:44Z","receivedAt":"2021-08-31T09:35:30Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"In both protocol v0 and v2, upload-pack writes one pktline packet per\nadvertised ref to stdout. That means one or two write(2) syscalls per\nref. This is problematic if these writes become network sends with\nhigh overhead.\n\nThis commit changes both send_ref callbacks to use buffered IO using\nstdio.\n\nTo give an example of the impact: I set up a single-threaded loop that\ncalls ls-remote (with HTTP and protocol v2) on a local GitLab\ninstance, on a repository with 11K refs. When I switch from Git\nv2.32.0 to this patch, I see a 40% reduction in CPU time for Git, and\n65% for Gitaly (GitLab's Git RPC service).\n\nSo using buffered IO not only saves syscalls in upload-pack, it also\nsaves time in things that consume upload-pack's output.\n\nHelped-by: Jeff King \nSigned-off-by: Jacob Vosmaer \n---\n ls-refs.c | 4 ++--\n upload-pack.c | 11 ++++++++---\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 88f6c3f60d..e6a2dbd962 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -105,7 +105,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t}\n \n \tstrbuf_addch(&refline, '\\n');\n-\tpacket_write(1, refline.buf, refline.len);\n+\tpacket_fwrite(stdout, refline.buf, refline.len);\n \n \tstrbuf_release(&refline);\n \treturn 0;\n@@ -171,7 +171,7 @@ int ls_refs(struct repository *r, struct strvec *keys,\n \t\tstrvec_push(&data.prefixes, \"\");\n \tfor_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v,\n \t\t\t\t send_ref, &data, 0);\n-\tpacket_flush(1);\n+\tpacket_fflush(stdout);\n \tstrvec_clear(&data.prefixes);\n \treturn 0;\n }\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 297b76fcb4..2fdd73dfcb 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1207,7 +1207,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \n \t\tformat_symref_info(&symref_info, &data->symref);\n \t\tformat_session_id(&session_id, data);\n-\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n+\t\tpacket_fwrite_fmt(stdout, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n \t\t\t (data->allow_uor & ALLOW_TIP_SHA1) ?\n@@ -1223,11 +1223,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\tstrbuf_release(&symref_info);\n \t\tstrbuf_release(&session_id);\n \t} else {\n-\t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n+\t\tpacket_fwrite_fmt(stdout, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n \tcapabilities = NULL;\n \tif (!peel_iterated_oid(oid, &peeled))\n-\t\tpacket_write_fmt(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+\t\tpacket_fwrite_fmt(stdout, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n \treturn 0;\n }\n \n@@ -1348,6 +1348,11 @@ void upload_pack(struct upload_pack_options *options)\n \t\treset_timeout(data.timeout);\n \t\thead_ref_namespaced(send_ref, &data);\n \t\tfor_each_namespaced_ref(send_ref, &data);\n+\t\t/* \n+\t\t * fflush stdout before calling advertise_shallow_grafts because send_ref\n+\t\t * uses stdio.\n+\t\t */\n+\t\tfflush_or_die(stdout);\n \t\tadvertise_shallow_grafts(1);\n \t\tpacket_flush(1);\n \t} else {\n-- \n2.32.0\n\n"},{"id":"434257","messageId":"YS4DmlJdjxeo+QI0@coredump.intra.peff.net","threadId":"56365","inReplyTo":"20210831093444.28199-1-jacob@gitlab.com","subject":"Re: [PATCH v3 0/2] send_ref buffering","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-31T10:25:30Z","receivedAt":"2021-08-31T10:25:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2021 at 11:34:42AM +0200, Jacob Vosmaer wrote:\n\n> Thanks for the reactions everyone. I agree that packet_fwrite_fmt\n> simplifies the patch nicely. Jeff, I hope I have given you credit\n> in an appropriate way, let me know if you want me to change something\n> there.\n\nWhat you did looks fine.\n\nOverall the series looks much nicer, and I don't have any real\ncomplaints. I do think it would be nice to take the packet_writer\ninterface further (letting it replace the static buf, and use stdio\nhandles, and using it throughout upload-pack). But this is a strict\nimprovement, so we can do that other refactoring later.\n\n> Regarding setvbuf: I have found out that GNU coreutils has a utility\n> called stdbuf that lets you modify the stdout buffer size at runtime\n> using some LD_PRELOAD hack so we can use that in Gitaly. I don't\n> think this is the best outcome for users, we ought to give them a\n> good default instead of expecting them to invoke git-upload-pack\n> as 'stdbuf -o 64K git-upload-pack'. But I can't judge the impact\n> of globally changing the stdout buffer size for Git so I'll settle\n> for having to use stdbuf.\n\nDoes the 64k buffer actually improve things? Here are the timings I get\non a repo with ~1M refs (it's linux.git with one ref per commit). \"git\"\nis current unbuffered version, and \"git.compile\" is master with your\npatches on top:\n\n $ hyperfine -i 'git upload-pack .' 'git.compile upload-pack .' 'stdbuf -o 64K git.compile upload-pack .'\n Benchmark #1: git upload-pack .\n Time (mean ± σ): 948.6 ms ± 7.3 ms [User: 840.8 ms, System: 107.8 ms]\n Range (min … max): 937.7 ms … 961.1 ms 10 runs\n \n Warning: Ignoring non-zero exit code.\n \n Benchmark #2: git.compile upload-pack .\n Time (mean ± σ): 867.3 ms ± 6.8 ms [User: 821.5 ms, System: 45.7 ms]\n Range (min … max): 859.7 ms … 883.0 ms 10 runs\n \n Warning: Ignoring non-zero exit code.\n \n Benchmark #3: stdbuf -o 64K git.compile upload-pack .\n Time (mean ± σ): 861.1 ms ± 8.2 ms [User: 815.5 ms, System: 45.6 ms]\n Range (min … max): 846.1 ms … 872.0 ms 10 runs\n \n Warning: Ignoring non-zero exit code.\n \n Summary\n 'stdbuf -o 64K git.compile upload-pack .' ran\n 1.01 ± 0.01 times faster than 'git.compile upload-pack .'\n 1.10 ± 0.01 times faster than 'git upload-pack .'\n\nThis is on a glibc system, so the default buffers should be 4k. It\ndoesn't appear to make any difference (there's a slight improvement, but\nwell within the noise, and I had other runs where it did worse).\n\nBy the way, if you really want to speed things up, try this:\n\n $ hyperfine -i 'git.compile upload-pack .' 'GIT_REF_PARANOIA=1 git.compile upload-pack .'\n Benchmark #1: git.compile upload-pack .\n Time (mean ± σ): 855.4 ms ± 5.8 ms [User: 803.4 ms, System: 52.0 ms]\n Range (min … max): 848.7 ms … 869.5 ms 10 runs\n \n Warning: Ignoring non-zero exit code.\n \n Benchmark #2: GIT_REF_PARANOIA=1 git.compile upload-pack .\n Time (mean ± σ): 394.4 ms ± 3.0 ms [User: 357.9 ms, System: 36.4 ms]\n Range (min … max): 390.6 ms … 400.3 ms 10 runs\n \n Warning: Ignoring non-zero exit code.\n \n Summary\n 'GIT_REF_PARANOIA=1 git.compile upload-pack .' ran\n 2.17 ± 0.02 times faster than 'git.compile upload-pack .'\n\nIt's not exactly the intended use of that environment variable, but its\nside effect is that we do not call has_object_file() on each ref tip.\n\n-Peff\n"},{"id":"434258","messageId":"YS4GVWdsnRqpM+4y@coredump.intra.peff.net","threadId":"56365","inReplyTo":"20210831093444.28199-2-jacob@gitlab.com","subject":"Re: [PATCH v3 1/2] pkt-line: add stdio packet write functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-31T10:37:09Z","receivedAt":"2021-08-31T10:38:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2021 at 11:34:43AM +0200, Jacob Vosmaer wrote:\n\n> +void fwrite_or_die(FILE *f, const void *buf, size_t count)\n> +{\n> +\tif (fwrite(buf, count, 1, f) != 1)\n> +\t\tdie_errno(\"fwrite error\");\n> +}\n\nOne small oddity I noticed. The definition of fwrite is\nfwrite(ptr, size, nmemb, strea), where we write \"nmemb\" items of \"size\"\nbytes each. I'd argue we're writing \"count\" single bytes, so it should\nbe:\n\n if (fwrite(buf, 1, count, f) != count)\n\nThis matters a lot for fread(), where any read shorter than \"count\"\n(e.g., due to EOF) would return \"0\" rather than a partial result. But I\nhave a hard time imagining an implementation of fwrite() where the\ndistinction would matter. And grepping around, we seem to have both\nforms in our code base already. So it's probably fine.\n\n-Peff\n"},{"id":"434262","messageId":"CADMWQoPerffQcTfKh3bfesjgHaBqXGzW2805knyRW3R_q4V-YA@mail.gmail.com","threadId":"56365","inReplyTo":"YS4DmlJdjxeo+QI0@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/2] send_ref buffering","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-08-31T13:08:25Z","receivedAt":"2021-08-31T13:08:41Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"On Tue, Aug 31, 2021 at 12:25 PM Jeff King wrote:\n> I do think it would be nice to take the packet_writer\n> interface further (letting it replace the static buf, and use stdio\n> handles, and using it throughout upload-pack).\nI would like that too, for the sake of neatness and general\nperformance, but I don't have the time to take on a larger project\nlike that at the moment.\n\n> Does the 64k buffer actually improve things? Here are the timings I get\n> on a repo with ~1M refs (it's linux.git with one ref per commit).\nThanks for challenging that. I have a repeatable benchmark where it\nmatters, because each write syscall wakes up a chain of proxies\nbetween the user and git-upload-pack. Larger buffers means fewer\nwake-ups. But then I tried to simplify my example by having sshd as\nthe only intermediary, and in that experiment 64K buffers were not\nbetter than 4K buffers. I think that goes to show that picking a good\nbuffer size is hard, and we'd be better off picking one specifically\nfor Gitaly (and GitLab) that works with our stack.\n\n> Summary\n> 'GIT_REF_PARANOIA=1 git.compile upload-pack .' ran\n> 2.17 ± 0.02 times faster than 'git.compile upload-pack .'\n>\n> It's not exactly the intended use of that environment variable, but its\n> side effect is that we do not call has_object_file() on each ref tip.\nThat is nice to know, but as a user of Git I don't know when it is or\nis not safe to skip those has_object_file() calls. If it's safe to\nskip them then Git should skip them always. If not, then I will err on\nthe side of caution and keep the checks.\n\nJacob\n"},{"id":"434310","messageId":"CADMWQoOfPr47a6bJtApt=wSE9BrgXQKJRfJuNAj0zbtio2BRHw@mail.gmail.com","threadId":"56365","inReplyTo":"CADMWQoPerffQcTfKh3bfesjgHaBqXGzW2805knyRW3R_q4V-YA@mail.gmail.com","subject":"Re: [PATCH v3 0/2] send_ref buffering","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-08-31T17:44:02Z","receivedAt":"2021-08-31T17:44:15Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"On Tue, Aug 31, 2021 at 3:08 PM Jacob Vosmaer wrote:\n>\n> On Tue, Aug 31, 2021 at 12:25 PM Jeff King wrote:\n> > I do think it would be nice to take the packet_writer\n> > interface further (letting it replace the static buf, and use stdio\n> > handles, and using it throughout upload-pack).\n> I would like that too, for the sake of neatness and general\n> performance, but I don't have the time to take on a larger project\n> like that at the moment.\nI gave solving the problem with packet_writer a couple of hours today.\nThe diff gets too big, and I have too little confidence I'm not\nintroducing deadlocks. This really is more work than I can chew off\nright now. Sorry!\n\nJacob\n"},{"id":"434315","messageId":"xmqqy28htrc6.fsf@gitster.g","threadId":"56365","inReplyTo":"20210831093444.28199-2-jacob@gitlab.com","subject":"Re: [PATCH v3 1/2] pkt-line: add stdio packet write functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-31T18:13:45Z","receivedAt":"2021-08-31T18:13:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Vosmaer writes:\n\n> +/*\n> + * Stdio versions of packet_write functions. When mixing these with fd\n> + * based functions, take care to call fflush or packet_fflush before\n> + * doing fd writes or closing the fd.\n> + */\n\nYou may have wanted to say that fflush() is not needed immediately\nafter packet_fflush() (because the latter calls fflush()), but I\nfind the \"or packet_fflush\" in the above comment highly misleading.\n\nIt's not like you can randomly call packet_fflush() where you would\ncall fflush(), as the former will insert a FLUSH packet to the\noutput stream. \"... take care to call fflush(3) before doihng fd\nwrites or closing the fd\" would be more appropriate. After all, if\nyou make fflush() even after calling packet_fflush(), nothing will\nbreak.\n\n> +void packet_fwrite(FILE *f, const char *buf, size_t size);\n> +void packet_fwrite_fmt(FILE *f, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n> +\n> +/* packet_fflush writes a flush packet and flushes the stdio buffer of f */\n> +void packet_fflush(FILE *f);\n> +\n> /*\n> * Read a packetized line into the buffer, which must be at least size bytes\n> * long. The return value specifies the number of bytes read into the buffer.\n> diff --git a/write-or-die.c b/write-or-die.c\n> index d33e68f6ab..7a2f84e2ee 100644\n> --- a/write-or-die.c\n> +++ b/write-or-die.c\n> @@ -70,3 +70,15 @@ void write_or_die(int fd, const void *buf, size_t count)\n> \t\tdie_errno(\"write error\");\n> \t}\n> }\n> +\n> +void fwrite_or_die(FILE *f, const void *buf, size_t count)\n> +{\n> +\tif (fwrite(buf, count, 1, f) != 1)\n\nThis counts the size of the buffer the wrong way.\n\nfwrite() gives the size of each element to be written out first, and\nthen number of elements that are written, which is the same as\nfread() but unfortunately the other way around from calloc() where\ncount comes first X-<.\n\nOther than that, nicely done.\n\nThanks.\n\n> +\t\tdie_errno(\"fwrite error\");\n> +}\n> +\n> +void fflush_or_die(FILE *f)\n> +{\n> +\tif (fflush(f))\n> +\t\tdie_errno(\"fflush error\");\n> +}\n"},{"id":"434374","messageId":"YS7GItELaEM8lq2z@coredump.intra.peff.net","threadId":"56365","inReplyTo":"CADMWQoOfPr47a6bJtApt=wSE9BrgXQKJRfJuNAj0zbtio2BRHw@mail.gmail.com","subject":"Re: [PATCH v3 0/2] send_ref buffering","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-01T00:15:30Z","receivedAt":"2021-09-01T00:15:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2021 at 03:08:25PM +0200, Jacob Vosmaer wrote:\n\n> > Does the 64k buffer actually improve things? Here are the timings I get\n> > on a repo with ~1M refs (it's linux.git with one ref per commit).\n> Thanks for challenging that. I have a repeatable benchmark where it\n> matters, because each write syscall wakes up a chain of proxies\n> between the user and git-upload-pack. Larger buffers means fewer\n> wake-ups. But then I tried to simplify my example by having sshd as\n> the only intermediary, and in that experiment 64K buffers were not\n> better than 4K buffers. I think that goes to show that picking a good\n> buffer size is hard, and we'd be better off picking one specifically\n> for Gitaly (and GitLab) that works with our stack.\n\nThanks for explaining. Yeah, I think leaving it as a custom thing makes\nsense, then.\n\n> > Summary\n> > 'GIT_REF_PARANOIA=1 git.compile upload-pack .' ran\n> > 2.17 ± 0.02 times faster than 'git.compile upload-pack .'\n> >\n> > It's not exactly the intended use of that environment variable, but its\n> > side effect is that we do not call has_object_file() on each ref tip.\n> That is nice to know, but as a user of Git I don't know when it is or\n> is not safe to skip those has_object_file() calls. If it's safe to\n> skip them then Git should skip them always. If not, then I will err on\n> the side of caution and keep the checks.\n\nYeah, the use of REF_PARANOIA there was just an easy illustration. IMHO\nit would be reasonable for upload-pack to just assume that the refs\nfiles are valid. If they aren't, then either:\n\n - the receiver is uninterested in those objects or already has them,\n so won't ask for them. They're happy either way.\n\n - the receiver _will_ ask for them, at which point we'd barf later in\n pack-objects when we try to access them.\n\nThere are some thoughts in this old thread which introduce\nGIT_REF_PARANOIA:\n\n https://lore.kernel.org/git/20150317073730.GA25267@peff.net/\n\nI think I was mostly too cowardly to make the change back then. And I\nhadn't considered that the performance implications would be all that\nbig (though I will say this million-ref repo is at the edge of what I'd\nconsider reasonable).\n\n> > > I do think it would be nice to take the packet_writer\n> > > interface further (letting it replace the static buf, and use stdio\n> > > handles, and using it throughout upload-pack).\n> > I would like that too, for the sake of neatness and general\n> > performance, but I don't have the time to take on a larger project\n> > like that at the moment.\n> I gave solving the problem with packet_writer a couple of hours today.\n> The diff gets too big, and I have too little confidence I'm not\n> introducing deadlocks. This really is more work than I can chew off\n> right now. Sorry!\n\nThanks for taking a look! I think we can proceed with your series for\nnow, then.\n\n-Peff\n"},{"id":"434430","messageId":"20210901125442.34446-1-jacob@gitlab.com","threadId":"56365","inReplyTo":"xmqqy28htrc6.fsf@gitster.g","subject":"[PATCH v4 0/2] send_ref buffering","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-09-01T12:54:40Z","receivedAt":"2021-09-01T13:06:41Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Changes:\n- pkt-line.h comment\n- fwrite argument order\n\nJacob Vosmaer (2):\n pkt-line: add stdio packet write functions\n upload-pack: use stdio in send_ref callbacks\n\n cache.h | 2 ++\n ls-refs.c | 4 ++--\n pkt-line.c | 37 +++++++++++++++++++++++++++++++++++++\n pkt-line.h | 11 +++++++++++\n upload-pack.c | 11 ++++++++---\n write-or-die.c | 12 ++++++++++++\n 6 files changed, 72 insertions(+), 5 deletions(-)\n\n-- \n2.32.0\n\n"},{"id":"434431","messageId":"20210901125442.34446-3-jacob@gitlab.com","threadId":"56365","inReplyTo":"20210901125442.34446-1-jacob@gitlab.com","subject":"[PATCH v4 2/2] upload-pack: use stdio in send_ref callbacks","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-09-01T12:54:42Z","receivedAt":"2021-09-01T13:06:42Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"In both protocol v0 and v2, upload-pack writes one pktline packet per\nadvertised ref to stdout. That means one or two write(2) syscalls per\nref. This is problematic if these writes become network sends with\nhigh overhead.\n\nThis commit changes both send_ref callbacks to use buffered IO using\nstdio.\n\nTo give an example of the impact: I set up a single-threaded loop that\ncalls ls-remote (with HTTP and protocol v2) on a local GitLab\ninstance, on a repository with 11K refs. When I switch from Git\nv2.32.0 to this patch, I see a 40% reduction in CPU time for Git, and\n65% for Gitaly (GitLab's Git RPC service).\n\nSo using buffered IO not only saves syscalls in upload-pack, it also\nsaves time in things that consume upload-pack's output.\n\nHelped-by: Jeff King \nSigned-off-by: Jacob Vosmaer \n---\n ls-refs.c | 4 ++--\n upload-pack.c | 11 ++++++++---\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex 88f6c3f60d..e6a2dbd962 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -105,7 +105,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t}\n \n \tstrbuf_addch(&refline, '\\n');\n-\tpacket_write(1, refline.buf, refline.len);\n+\tpacket_fwrite(stdout, refline.buf, refline.len);\n \n \tstrbuf_release(&refline);\n \treturn 0;\n@@ -171,7 +171,7 @@ int ls_refs(struct repository *r, struct strvec *keys,\n \t\tstrvec_push(&data.prefixes, \"\");\n \tfor_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v,\n \t\t\t\t send_ref, &data, 0);\n-\tpacket_flush(1);\n+\tpacket_fflush(stdout);\n \tstrvec_clear(&data.prefixes);\n \treturn 0;\n }\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 297b76fcb4..2fdd73dfcb 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1207,7 +1207,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \n \t\tformat_symref_info(&symref_info, &data->symref);\n \t\tformat_session_id(&session_id, data);\n-\t\tpacket_write_fmt(1, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n+\t\tpacket_fwrite_fmt(stdout, \"%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\\n\",\n \t\t\t oid_to_hex(oid), refname_nons,\n \t\t\t 0, capabilities,\n \t\t\t (data->allow_uor & ALLOW_TIP_SHA1) ?\n@@ -1223,11 +1223,11 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\tstrbuf_release(&symref_info);\n \t\tstrbuf_release(&session_id);\n \t} else {\n-\t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n+\t\tpacket_fwrite_fmt(stdout, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n \tcapabilities = NULL;\n \tif (!peel_iterated_oid(oid, &peeled))\n-\t\tpacket_write_fmt(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+\t\tpacket_fwrite_fmt(stdout, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n \treturn 0;\n }\n \n@@ -1348,6 +1348,11 @@ void upload_pack(struct upload_pack_options *options)\n \t\treset_timeout(data.timeout);\n \t\thead_ref_namespaced(send_ref, &data);\n \t\tfor_each_namespaced_ref(send_ref, &data);\n+\t\t/* \n+\t\t * fflush stdout before calling advertise_shallow_grafts because send_ref\n+\t\t * uses stdio.\n+\t\t */\n+\t\tfflush_or_die(stdout);\n \t\tadvertise_shallow_grafts(1);\n \t\tpacket_flush(1);\n \t} else {\n-- \n2.32.0\n\n"},{"id":"434432","messageId":"20210901125442.34446-2-jacob@gitlab.com","threadId":"56365","inReplyTo":"20210901125442.34446-1-jacob@gitlab.com","subject":"[PATCH v4 1/2] pkt-line: add stdio packet write functions","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-09-01T12:54:41Z","receivedAt":"2021-09-01T13:06:43Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"This adds three new functions to pkt-line.c: packet_fwrite,\npacket_fwrite_fmt and packet_fflush. Besides writing a pktline flush\npacket, packet_fflush also flushes the stdio buffer of the stream.\n\nHelped-by: Patrick Steinhardt \nHelped-by: Jeff King \nSigned-off-by: Jacob Vosmaer \n---\n cache.h | 2 ++\n pkt-line.c | 37 +++++++++++++++++++++++++++++++++++++\n pkt-line.h | 11 +++++++++++\n write-or-die.c | 12 ++++++++++++\n 4 files changed, 62 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex bd4869beee..dcf2454c3b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1736,6 +1736,8 @@ extern const char *git_mailmap_blob;\n void maybe_flush_or_die(FILE *, const char *);\n __attribute__((format (printf, 2, 3)))\n void fprintf_or_die(FILE *, const char *fmt, ...);\n+void fwrite_or_die(FILE *f, const void *buf, size_t count);\n+void fflush_or_die(FILE *f);\n \n #define COPY_READ_ERROR (-2)\n #define COPY_WRITE_ERROR (-3)\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 9f63eae2e6..de4a94b437 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -243,6 +243,43 @@ void packet_write(int fd_out, const char *buf, size_t size)\n \t\tdie(\"%s\", err.buf);\n }\n \n+void packet_fwrite(FILE *f, const char *buf, size_t size)\n+{\n+\tsize_t packet_size;\n+\tchar header[4];\n+\n+\tif (size > LARGE_PACKET_DATA_MAX)\n+\t\tdie(_(\"packet write failed - data exceeds max packet size\"));\n+\n+\tpacket_trace(buf, size, 1);\n+\tpacket_size = size + 4;\n+\n+\tset_packet_header(header, packet_size);\n+\tfwrite_or_die(f, header, 4);\n+\tfwrite_or_die(f, buf, size);\n+}\n+\n+void packet_fwrite_fmt(FILE *fh, const char *fmt, ...)\n+{\n+ static struct strbuf buf = STRBUF_INIT;\n+ va_list args;\n+\n+ strbuf_reset(&buf);\n+\n+ va_start(args, fmt);\n+ format_packet(&buf, \"\", fmt, args);\n+ va_end(args);\n+\n+ fwrite_or_die(fh, buf.buf, buf.len);\n+}\n+\n+void packet_fflush(FILE *f)\n+{\n+\tpacket_trace(\"0000\", 4, 1);\n+\tfwrite_or_die(f, \"0000\", 4);\n+\tfflush_or_die(f);\n+}\n+\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n {\n \tva_list args;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 5af5f45687..82b95e4bdd 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -35,6 +35,17 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format\n int write_packetized_from_fd_no_flush(int fd_in, int fd_out);\n int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_out);\n \n+/*\n+ * Stdio versions of packet_write functions. When mixing these with fd\n+ * based functions, take care to call fflush(3) before doing fd writes or\n+ * closing the fd.\n+ */\n+void packet_fwrite(FILE *f, const char *buf, size_t size);\n+void packet_fwrite_fmt(FILE *f, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n+\n+/* packet_fflush writes a flush packet and flushes the stdio buffer of f */\n+void packet_fflush(FILE *f);\n+\n /*\n * Read a packetized line into the buffer, which must be at least size bytes\n * long. The return value specifies the number of bytes read into the buffer.\ndiff --git a/write-or-die.c b/write-or-die.c\nindex d33e68f6ab..0b1ec8190b 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -70,3 +70,15 @@ void write_or_die(int fd, const void *buf, size_t count)\n \t\tdie_errno(\"write error\");\n \t}\n }\n+\n+void fwrite_or_die(FILE *f, const void *buf, size_t count)\n+{\n+\tif (fwrite(buf, 1, count, f) != count)\n+\t\tdie_errno(\"fwrite error\");\n+}\n+\n+void fflush_or_die(FILE *f)\n+{\n+\tif (fflush(f))\n+\t\tdie_errno(\"fflush error\");\n+}\n-- \n2.32.0\n\n"},{"id":"434506","messageId":"YTCW+yeQ47OcnEMO@coredump.intra.peff.net","threadId":"56365","inReplyTo":"20210901125442.34446-1-jacob@gitlab.com","subject":"Re: [PATCH v4 0/2] send_ref buffering","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-02T09:18:51Z","receivedAt":"2021-09-02T09:19:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 01, 2021 at 02:54:40PM +0200, Jacob Vosmaer wrote:\n\n> Changes:\n> - pkt-line.h comment\n> - fwrite argument order\n> \n> Jacob Vosmaer (2):\n> pkt-line: add stdio packet write functions\n> upload-pack: use stdio in send_ref callbacks\n\nThanks, this version looks good to me.\n\n-Peff\n"}]}