{"thread":{"id":"57074","subject":"[PATCH 0/1] Make upload-pack pack write size configurable","startedAt":"2021-12-13T13:24:42Z","lastAt":"2021-12-15T21:35:01Z","messageCount":17,"participants":["Jacob Vosmaer","Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano","rsbecker@nexbridge.com"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"443974","messageId":"20211213132345.26310-1-jacob@gitlab.com","threadId":"57074","inReplyTo":null,"subject":"[PATCH 0/1] Make upload-pack pack write size configurable","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-12-13T13:23:44Z","receivedAt":"2021-12-13T13:24:42Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"When transfering packfile data, upload-pack.c uses an 8KB buffer.\nThis is a reasonable size but when you transfer a lot of packfile\ndata, like we do on GitLab.com, we find it is beneficial to use a\nlarger buffer size.\n\nBelow you will find a commit where we make the size of this 8KB\nbuffer configurable at compile time. It appears pack-objects always\ndoes 8KB writes so I don't think we should change the default. But\nfor GitLab, where we have a cache for the output of pack-objects,\nit is beneficial to use a larger IO size because the cache does\n64KB writes.\n\nI have also considered converting the packfile copying code to use\nstdio when writing to stdout, but that would be a bigger change\nbecause we have to be careful not to interleave stdio and stdlib\nwrites. And we would have to make the stdout output buffer size\nconfigurable, because the default stdio buffer size is 4KB which\nis no better than the status quo. A final argument against the stdio\napproach is that it only reduces the number of writes from upload-pack,\nwhile a larger buffer size reduces both the number of reads and\nwrites.\n\nHaving said all that, if the Git maintainers prefer the stdio\napproach over this compile time constant, I am happy to submit a\npatch series for that instead.\n\nThanks,\n\nJacob Vosmaer\n\nJacob Vosmaer (1):\n  upload-pack.c: make output buffer size configurable\n\n upload-pack.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\n-- \n2.33.0\n\n"},{"id":"443975","messageId":"20211213132345.26310-2-jacob@gitlab.com","threadId":"57074","inReplyTo":"20211213132345.26310-1-jacob@gitlab.com","subject":"[PATCH 1/1] upload-pack.c: make output buffer size configurable","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-12-13T13:23:45Z","receivedAt":"2021-12-13T13:24:47Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Add a new compile time constant UPLOAD_PACK_BUFFER_SIZE that makes the\nsize of the static buffer in output_state configurable.\n\nThe current size of the buffer is 8192+1. The '+1' is a technical\ndetail orthogonal to this change. On GitLab.com we use GitLab's\npack-objects cache which does writes of 65515 bytes. Because of the\ndefault 8KB buffer size, propagating these cache writes requires 8\npipe reads and 8 pipe writes from git-upload-pack, and 8 pipe reads\nfrom Gitaly (our Git RPC service). If we increase the size of the\nbuffer to the maximum Git packet size, we need only 1 pipe read and 1\npipe write in git-upload-pack, and 1 pipe read in Gitaly to transfer\nthe same amount of data. In benchmarks with a pure fetch and 100%\ncache hit rate workload we are seeing CPU utilization reductions of\nover 30%.\n\nSigned-off-by: Jacob Vosmaer <jacob@gitlab.com>\n---\n upload-pack.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c78d55bc67..b799fbe628 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -42,6 +42,10 @@\n #define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \\\n \t\tNOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF)\n \n+#ifndef UPLOAD_PACK_BUFFER_SIZE\n+#define UPLOAD_PACK_BUFFER_SIZE 8192\n+#endif\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@@ -194,7 +198,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n }\n \n struct output_state {\n-\tchar buffer[8193];\n+\tchar buffer[UPLOAD_PACK_BUFFER_SIZE+1];\n \tint used;\n \tunsigned packfile_uris_started : 1;\n \tunsigned packfile_started : 1;\n-- \n2.33.0\n\n"},{"id":"444098","messageId":"211214.86y24nbd46.gmgdl@evledraar.gmail.com","threadId":"57074","inReplyTo":"20211213132345.26310-2-jacob@gitlab.com","subject":"Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-14T12:08:55Z","receivedAt":"2021-12-14T12:11:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 13 2021, Jacob Vosmaer wrote:\n\n> Add a new compile time constant UPLOAD_PACK_BUFFER_SIZE that makes the\n> size of the static buffer in output_state configurable.\n>\n> The current size of the buffer is 8192+1. The '+1' is a technical\n> detail orthogonal to this change. On GitLab.com we use GitLab's\n> pack-objects cache which does writes of 65515 bytes. Because of the\n> default 8KB buffer size, propagating these cache writes requires 8\n> pipe reads and 8 pipe writes from git-upload-pack, and 8 pipe reads\n> from Gitaly (our Git RPC service). If we increase the size of the\n> buffer to the maximum Git packet size, we need only 1 pipe read and 1\n> pipe write in git-upload-pack, and 1 pipe read in Gitaly to transfer\n> the same amount of data. In benchmarks with a pure fetch and 100%\n> cache hit rate workload we are seeing CPU utilization reductions of\n> over 30%.\n>\n> Signed-off-by: Jacob Vosmaer <jacob@gitlab.com>\n> ---\n>  upload-pack.c | 6 +++++-\n>  1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index c78d55bc67..b799fbe628 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -42,6 +42,10 @@\n>  #define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \\\n>  \t\tNOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF)\n>  \n> +#ifndef UPLOAD_PACK_BUFFER_SIZE\n> +#define UPLOAD_PACK_BUFFER_SIZE 8192\n> +#endif\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> @@ -194,7 +198,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n>  }\n>  \n>  struct output_state {\n> -\tchar buffer[8193];\n> +\tchar buffer[UPLOAD_PACK_BUFFER_SIZE+1];\n>  \tint used;\n>  \tunsigned packfile_uris_started : 1;\n>  \tunsigned packfile_started : 1;\n\nMaking this configurable obviousl has big impact in some cases, but I'm\na bit iffy on the faciltity to do so + it not being documented.\n\nI don't think that the \"static buffer\" part here is important to anyone,\nbut the write() size is clearly important.\n\nSo doesn't it make more sense to have a uploadPack.bufferSize=8k\nvariable we can tweak, just make this \"buffer\" a \"struct strbuf\" instead\n(i.e. it'll by dynamically grown), and then just flush it whenever we\nhit the configured buffer size?\n"},{"id":"444112","messageId":"YbizfdGq+RSu9BGe@coredump.intra.peff.net","threadId":"57074","inReplyTo":"211214.86y24nbd46.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-14T15:08:45Z","receivedAt":"2021-12-14T15:08:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 14, 2021 at 01:08:55PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > +#ifndef UPLOAD_PACK_BUFFER_SIZE\n> > +#define UPLOAD_PACK_BUFFER_SIZE 8192\n> > +#endif\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> > @@ -194,7 +198,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n> >  }\n> >  \n> >  struct output_state {\n> > -\tchar buffer[8193];\n> > +\tchar buffer[UPLOAD_PACK_BUFFER_SIZE+1];\n> >  \tint used;\n> >  \tunsigned packfile_uris_started : 1;\n> >  \tunsigned packfile_started : 1;\n> \n> Making this configurable obviousl has big impact in some cases, but I'm\n> a bit iffy on the faciltity to do so + it not being documented.\n> \n> I don't think that the \"static buffer\" part here is important to anyone,\n> but the write() size is clearly important.\n> \n> So doesn't it make more sense to have a uploadPack.bufferSize=8k\n> variable we can tweak, just make this \"buffer\" a \"struct strbuf\" instead\n> (i.e. it'll by dynamically grown), and then just flush it whenever we\n> hit the configured buffer size?\n\nI don't think we want to grow dynamically, because we don't want to hold\narbitrary amounts of data in memory. We're just passing it through. But\nif there were a run-time config option, we could easily heap-allocate\nthe buffer up front.\n\nThat may be overkill, though I do agree this is kind of weirdly\nundocumented. There are two other subtleties I notice:\n\n  - This output_state struct does go on the stack, so something big like\n    64k is questionable there (though on Linux, without recursion, it's\n    usually OK).\n\n  - we're relaying the data into pkt-lines. Do we run into problems when\n    the buffer is larger than a packet? I think in send_sideband() we'll\n    break it up as appropriate. But before we hit the PACK header, we\n    send out packfile-uris directly with packet_write_fmt(). Those\n    aren't likely to be long, but if they are, we'd die() in\n    format_packet(). So something around LARGE_PACKET_DATA_MAX is\n    probably the highest you'd want to set it anyway (and the most\n    performant, since otherwise you have to write out extra partial\n    packets).\n\nSo I kind of wonder if there is any real _harm_ in just always using\nbigger packets, even if it does not always help. Given the subtle rules\nabout packet-max above, then we could just use that optimal value and\nnot worry about configurability.\n\n-Peff\n"},{"id":"444113","messageId":"Ybi0dSddal2DKmXQ@coredump.intra.peff.net","threadId":"57074","inReplyTo":"20211213132345.26310-1-jacob@gitlab.com","subject":"Re: [PATCH 0/1] Make upload-pack pack write size configurable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-14T15:12:53Z","receivedAt":"2021-12-14T15:12:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 13, 2021 at 02:23:44PM +0100, Jacob Vosmaer wrote:\n\n> When transfering packfile data, upload-pack.c uses an 8KB buffer.\n> This is a reasonable size but when you transfer a lot of packfile\n> data, like we do on GitLab.com, we find it is beneficial to use a\n> larger buffer size.\n> \n> Below you will find a commit where we make the size of this 8KB\n> buffer configurable at compile time. It appears pack-objects always\n> does 8KB writes so I don't think we should change the default. But\n> for GitLab, where we have a cache for the output of pack-objects,\n> it is beneficial to use a larger IO size because the cache does\n> 64KB writes.\n\nI suspect the big reason that it matters for the cache is that for\npack-objects, we're typically CPU bound computing the sha1 of the\noutgoing pack (for its trailer).\n\nI do suspect we could just always move to something closer to\nLARGE_PACKET_DATA_MAX (probably minus 1 for the sideband marker). Even\nif pack-objects only writes in 8k chunks, we may be able to pull several\nin one read(), especially if the network is a bottleneck (because we'd\nblock writing to the client, and pack-objects would fill up the pipe\nbuffer).\n\nI.e., it doesn't seem like there's any real downside to doing so.\n\n> I have also considered converting the packfile copying code to use\n> stdio when writing to stdout, but that would be a bigger change\n> because we have to be careful not to interleave stdio and stdlib\n> writes. And we would have to make the stdout output buffer size\n> configurable, because the default stdio buffer size is 4KB which\n> is no better than the status quo. A final argument against the stdio\n> approach is that it only reduces the number of writes from upload-pack,\n> while a larger buffer size reduces both the number of reads and\n> writes.\n\nYeah, I don't think that would help all that much. We really want to\nsize this based on pkt-line limits. That reduces syscalls, but also\nshrinks the overall size a little (since larger packets minimizes the\nframing overhead of the pkt-line headers).\n\n-Peff\n"},{"id":"444114","messageId":"Ybi6SwndUHLs27bO@coredump.intra.peff.net","threadId":"57074","inReplyTo":"20211213132345.26310-2-jacob@gitlab.com","subject":"Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-14T15:37:47Z","receivedAt":"2021-12-14T15:37:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 13, 2021 at 02:23:45PM +0100, Jacob Vosmaer wrote:\n\n> The current size of the buffer is 8192+1. The '+1' is a technical\n> detail orthogonal to this change. On GitLab.com we use GitLab's\n> pack-objects cache which does writes of 65515 bytes. Because of the\n> default 8KB buffer size, propagating these cache writes requires 8\n> pipe reads and 8 pipe writes from git-upload-pack, and 8 pipe reads\n> from Gitaly (our Git RPC service). If we increase the size of the\n> buffer to the maximum Git packet size, we need only 1 pipe read and 1\n> pipe write in git-upload-pack, and 1 pipe read in Gitaly to transfer\n> the same amount of data. In benchmarks with a pure fetch and 100%\n> cache hit rate workload we are seeing CPU utilization reductions of\n> over 30%.\n\nI was curious to reproduce this locally, so I came up with:\n\n  (\n    printf \"003fwant %s side-band-64k\" $(git rev-parse HEAD)\n    printf 0000\n    echo '0009done'\n  ) |\n  git -c uploadpack.packobjectsHook='cat objects/pack/pack-*.pack; :' upload-pack . |\n  pv >/dev/null\n\nwhich hackily simulates the server side of your \"cached\" case. :) I ran\nit on a fully-packed clone of linux.git.\n\nIt gets about 2.3GB/s with the tip of 'master' and 3.2GB/s with the\nequivalent of your patch (using LARGE_PACKET_DATA_MAX). So definitely an\nimprovement.\n\nWithout the cached case (so actually running pack-objects, albeit a\npretty quick one because of bitmaps and pack-reuse), the timings are\nabout the same (171MB/s versus 174MB/s, but really it's just pegging a\nCPU running pack-objects). So it would be fine to just do this\nunconditionally, I think.\n\nLooking at strace, the other thing I notice is that we write() the\npacket header separately in send_sideband(), which doubles the number of\nsyscalls. I hackily re-wrote this to use writev() instead (patch below),\nbut it doesn't seem to actually help much (maybe a curiosity to explore\nfurther, but definitely not something to hold up your patch).\n\n-Peff\n\n---\ndiff --git a/sideband.c b/sideband.c\nindex 85bddfdcd4..d0945507a3 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -5,6 +5,11 @@\n #include \"help.h\"\n #include \"pkt-line.h\"\n \n+/* hack; should go in git-compat-util, and should provide compat\n+ * wrapper around regular write()\n+ */\n+#include <sys/uio.h>\n+\n struct keyword_entry {\n \t/*\n \t * We use keyword as config key so it should be a single alphanumeric word.\n@@ -257,6 +262,7 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n \n \twhile (sz) {\n \t\tunsigned n;\n+\t\tstruct iovec iov[2];\n \t\tchar hdr[5];\n \n \t\tn = sz;\n@@ -265,12 +271,17 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n \t\tif (0 <= band) {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 5);\n \t\t\thdr[4] = band;\n-\t\t\twrite_or_die(fd, hdr, 5);\n+\t\t\tiov[0].iov_base = hdr;\n+\t\t\tiov[0].iov_len = 5;\n \t\t} else {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 4);\n-\t\t\twrite_or_die(fd, hdr, 4);\n+\t\t\tiov[0].iov_base = hdr;\n+\t\t\tiov[0].iov_len = 4;\n \t\t}\n-\t\twrite_or_die(fd, p, n);\n+\t\tiov[1].iov_base = (void *)p;\n+\t\tiov[1].iov_len = n;\n+\t\t/* should check for errors, but also short writes and EINTR, etc */\n+\t\twritev(fd, iov, 2);\n \t\tp += n;\n \t\tsz -= n;\n \t}\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c78d55bc67..111de8c60c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -194,7 +194,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n }\n \n struct output_state {\n-\tchar buffer[8193];\n+\tchar buffer[LARGE_PACKET_DATA_MAX];\n \tint used;\n \tunsigned packfile_uris_started : 1;\n \tunsigned packfile_started : 1;\n"},{"id":"444117","messageId":"20211214194626.33814-1-jacob@gitlab.com","threadId":"57074","inReplyTo":"YbizfdGq+RSu9BGe@coredump.intra.peff.net","subject":"[PATCH v2 0/1] upload-pack.c: increase output buffer size","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-12-14T19:46:25Z","receivedAt":"2021-12-14T19:46:47Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Thank you Ævar and Peff for your comments on the previous iteration\nof this patch. I have made the buffer size fixed at the pktline\nmaximum, and I have changed the storage of output_state from stack\nto heap.\n\nAs far as I can tell, all return paths from create_pack_file call\n\"die\", except the happy path. So I only free the heap-allocated\noutput_state on the happy path. Is that OK, or should I use a \"goto\nout\" function shape?\n\nJacob Vosmaer (1):\n  upload-pack.c: increase output buffer size\n\n upload-pack.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\n-- \n2.33.0\n\n"},{"id":"444118","messageId":"20211214194626.33814-2-jacob@gitlab.com","threadId":"57074","inReplyTo":"20211214194626.33814-1-jacob@gitlab.com","subject":"[PATCH v2 1/1] upload-pack.c: increase output buffer size","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-12-14T19:46:26Z","receivedAt":"2021-12-14T19:46:55Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"When serving a fetch, git upload-pack copies data from a git\npack-objects stdout pipe to its stdout. This commit increases the size\nof the buffer used for that copying from 8192 to 65515, the maximum\nsideband-64k packet size.\n\nPreviously, this buffer was allocated on the stack. Because the new\nbuffer size is nearly 64KB, we switch this to a heap allocation.\n\nOn GitLab.com we use GitLab's pack-objects cache which does writes of\n65515 bytes. Because of the default 8KB buffer size, propagating these\ncache writes requires 8 pipe reads and 8 pipe writes from\ngit-upload-pack, and 8 pipe reads from Gitaly (our Git RPC service).\nIf we increase the size of the buffer to the maximum Git packet size,\nwe need only 1 pipe read and 1 pipe write in git-upload-pack, and 1\npipe read in Gitaly to transfer the same amount of data. In benchmarks\nwith a pure fetch and 100% cache hit rate workload we are seeing CPU\nutilization reductions of over 30%.\n\nSigned-off-by: Jacob Vosmaer <jacob@gitlab.com>\n---\n upload-pack.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c78d55bc67..3b90fb69e6 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -194,7 +194,13 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n }\n \n struct output_state {\n-\tchar buffer[8193];\n+\t/*\n+\t * We do writes no bigger than LARGE_PACKET_DATA_MAX - 1, because with\n+\t * sideband-64k the band designator takes up 1 byte of space. Because\n+\t * relay_pack_data keeps the last byte to itself, we make the buffer 1\n+\t * byte bigger than the intended maximum write size.\n+\t */\n+\tchar buffer[(LARGE_PACKET_DATA_MAX - 1) + 1];\n \tint used;\n \tunsigned packfile_uris_started : 1;\n \tunsigned packfile_started : 1;\n@@ -269,7 +275,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\t\t     const struct string_list *uri_protocols)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n-\tstruct output_state output_state = { { 0 } };\n+\tstruct output_state *output_state = xcalloc(1, sizeof(struct output_state));\n \tchar progress[128];\n \tchar abort_msg[] = \"aborting due to possible repository \"\n \t\t\"corruption on the remote side.\";\n@@ -404,7 +410,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\t}\n \t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n \t\t\tint result = relay_pack_data(pack_objects.out,\n-\t\t\t\t\t\t     &output_state,\n+\t\t\t\t\t\t     output_state,\n \t\t\t\t\t\t     pack_data->use_sideband,\n \t\t\t\t\t\t     !!uri_protocols);\n \n@@ -438,11 +444,12 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t}\n \n \t/* flush the data */\n-\tif (output_state.used > 0) {\n-\t\tsend_client_data(1, output_state.buffer, output_state.used,\n+\tif (output_state->used > 0) {\n+\t\tsend_client_data(1, output_state->buffer, output_state->used,\n \t\t\t\t pack_data->use_sideband);\n \t\tfprintf(stderr, \"flushed.\\n\");\n \t}\n+\tfree(output_state);\n \tif (pack_data->use_sideband)\n \t\tpacket_flush(1);\n \treturn;\n-- \n2.33.0\n\n"},{"id":"444119","messageId":"CADMWQoOaPQaCQa5V+1Dujc6A=+f8rnv7MG+fP1mZi1kwm=d3fQ@mail.gmail.com","threadId":"57074","inReplyTo":"Ybi6SwndUHLs27bO@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-12-14T20:04:49Z","receivedAt":"2021-12-14T20:05:02Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"On Tue, Dec 14, 2021 at 4:37 PM Jeff King <peff@peff.net> wrote:\n>\n> It gets about 2.3GB/s with the tip of 'master' and 3.2GB/s with the\n> equivalent of your patch (using LARGE_PACKET_DATA_MAX). So definitely an\n> improvement.\n>\n> Without the cached case (so actually running pack-objects, albeit a\n> pretty quick one because of bitmaps and pack-reuse), the timings are\n> about the same (171MB/s versus 174MB/s, but really it's just pegging a\n> CPU running pack-objects). So it would be fine to just do this\n> unconditionally, I think.\n\nThank you for validating this!\n\n> Looking at strace, the other thing I notice is that we write() the\n> packet header separately in send_sideband(), which doubles the number of\n> syscalls. I hackily re-wrote this to use writev() instead (patch below),\n> but it doesn't seem to actually help much (maybe a curiosity to explore\n> further, but definitely not something to hold up your patch).\n\nThose double writes bug me too. Getting rid of them was one of the\nthings I liked about using stdio here. Curious now if writev will make\nan impact in my benchmarks. As your experiments indicate, the double\nwrites may not be a problem in real life. Either way, I also feel it\nis not something to rope into this patch set.\n"},{"id":"444123","messageId":"211214.86h7bac3vy.gmgdl@evledraar.gmail.com","threadId":"57074","inReplyTo":"20211214194626.33814-2-jacob@gitlab.com","subject":"Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-14T20:41:27Z","receivedAt":"2021-12-14T20:45:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 14 2021, Jacob Vosmaer wrote:\n\n> When serving a fetch, git upload-pack copies data from a git\n> pack-objects stdout pipe to its stdout. This commit increases the size\n> of the buffer used for that copying from 8192 to 65515, the maximum\n> sideband-64k packet size.\n>\n> Previously, this buffer was allocated on the stack. Because the new\n> buffer size is nearly 64KB, we switch this to a heap allocation.\n>\n> On GitLab.com we use GitLab's pack-objects cache which does writes of\n> 65515 bytes. Because of the default 8KB buffer size, propagating these\n> cache writes requires 8 pipe reads and 8 pipe writes from\n> git-upload-pack, and 8 pipe reads from Gitaly (our Git RPC service).\n> If we increase the size of the buffer to the maximum Git packet size,\n> we need only 1 pipe read and 1 pipe write in git-upload-pack, and 1\n> pipe read in Gitaly to transfer the same amount of data. In benchmarks\n> with a pure fetch and 100% cache hit rate workload we are seeing CPU\n> utilization reductions of over 30%.\n>\n> Signed-off-by: Jacob Vosmaer <jacob@gitlab.com>\n> ---\n>  upload-pack.c | 17 ++++++++++++-----\n>  1 file changed, 12 insertions(+), 5 deletions(-)\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index c78d55bc67..3b90fb69e6 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -194,7 +194,13 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n>  }\n>  \n>  struct output_state {\n> -\tchar buffer[8193];\n> +\t/*\n> +\t * We do writes no bigger than LARGE_PACKET_DATA_MAX - 1, because with\n> +\t * sideband-64k the band designator takes up 1 byte of space. Because\n> +\t * relay_pack_data keeps the last byte to itself, we make the buffer 1\n> +\t * byte bigger than the intended maximum write size.\n> +\t */\n> +\tchar buffer[(LARGE_PACKET_DATA_MAX - 1) + 1];\n\nSince X-1+1 = X shouldn't we just say X ? :)\n\nMaybe this fixup is better, maybe not:\n\t\n\tdiff --git a/upload-pack.c b/upload-pack.c\n\tindex 3b90fb69e6d..10849110229 100644\n\t--- a/upload-pack.c\n\t+++ b/upload-pack.c\n\t@@ -195,12 +195,12 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n\t \n\t struct output_state {\n\t \t/*\n\t-\t * We do writes no bigger than LARGE_PACKET_DATA_MAX - 1, because with\n\t-\t * sideband-64k the band designator takes up 1 byte of space. Because\n\t-\t * relay_pack_data keeps the last byte to itself, we make the buffer 1\n\t-\t * byte bigger than the intended maximum write size.\n\t+\t * We do writes no bigger than LARGE_PACKET_DATA_MAX - 1,\n\t+\t * because with * sideband-64k the band designator takes up 1\n\t+\t * byte of space (see relay_pack_data() below). So\n\t+\t * LARGE_PACKET_DATA_MAX ends up being the right size.\n\t \t */\n\t-\tchar buffer[(LARGE_PACKET_DATA_MAX - 1) + 1];\n\t+\tchar buffer[LARGE_PACKET_DATA_MAX];\n\t \tint used;\n\t \tunsigned packfile_uris_started : 1;\n\t \tunsigned packfile_started : 1;\n\n>  \tint used;\n>  \tunsigned packfile_uris_started : 1;\n>  \tunsigned packfile_started : 1;\n> @@ -269,7 +275,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n>  \t\t\t     const struct string_list *uri_protocols)\n>  {\n>  \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n> -\tstruct output_state output_state = { { 0 } };\n> +\tstruct output_state *output_state = xcalloc(1, sizeof(struct output_state));\n\nI don't know when/if we need to worry about 8k v.s. ~65k on the stack,\nespecially as recv_sideband() has:\n\n\tchar buf[LARGE_PACKET_MAX + 1];\n\nBut maybe our stack is already quite big here, I don't know...\n\n>  \tchar progress[128];\n>  \tchar abort_msg[] = \"aborting due to possible repository \"\n>  \t\t\"corruption on the remote side.\";\n> @@ -404,7 +410,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n>  \t\t}\n>  \t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n>  \t\t\tint result = relay_pack_data(pack_objects.out,\n> -\t\t\t\t\t\t     &output_state,\n> +\t\t\t\t\t\t     output_state,\n>  \t\t\t\t\t\t     pack_data->use_sideband,\n>  \t\t\t\t\t\t     !!uri_protocols);\n>  \n> @@ -438,11 +444,12 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n>  \t}\n>  \n>  \t/* flush the data */\n> -\tif (output_state.used > 0) {\n> -\t\tsend_client_data(1, output_state.buffer, output_state.used,\n> +\tif (output_state->used > 0) {\n> +\t\tsend_client_data(1, output_state->buffer, output_state->used,\n>  \t\t\t\t pack_data->use_sideband);\n>  \t\tfprintf(stderr, \"flushed.\\n\");\n>  \t}\n> +\tfree(output_state);\n>  \tif (pack_data->use_sideband)\n>  \t\tpacket_flush(1);\n>  \treturn;\n\nBut this looks fine either way. And yes, in reply to the question in the\ncover letter it's fine to ignore the memory leaks we have when we call\ndie().\n"},{"id":"444150","messageId":"YboYM7YKcRx1RuJY@coredump.intra.peff.net","threadId":"57074","inReplyTo":"20211214194626.33814-2-jacob@gitlab.com","subject":"Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-15T16:30:43Z","receivedAt":"2021-12-15T16:30:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 14, 2021 at 08:46:26PM +0100, Jacob Vosmaer wrote:\n\n> When serving a fetch, git upload-pack copies data from a git\n> pack-objects stdout pipe to its stdout. This commit increases the size\n> of the buffer used for that copying from 8192 to 65515, the maximum\n> sideband-64k packet size.\n> \n> Previously, this buffer was allocated on the stack. Because the new\n> buffer size is nearly 64KB, we switch this to a heap allocation.\n> \n> On GitLab.com we use GitLab's pack-objects cache which does writes of\n> 65515 bytes. Because of the default 8KB buffer size, propagating these\n> cache writes requires 8 pipe reads and 8 pipe writes from\n> git-upload-pack, and 8 pipe reads from Gitaly (our Git RPC service).\n> If we increase the size of the buffer to the maximum Git packet size,\n> we need only 1 pipe read and 1 pipe write in git-upload-pack, and 1\n> pipe read in Gitaly to transfer the same amount of data. In benchmarks\n> with a pure fetch and 100% cache hit rate workload we are seeing CPU\n> utilization reductions of over 30%.\n\nThanks, this patch looks good to me.\n\n>  struct output_state {\n> -\tchar buffer[8193];\n> +\t/*\n> +\t * We do writes no bigger than LARGE_PACKET_DATA_MAX - 1, because with\n> +\t * sideband-64k the band designator takes up 1 byte of space. Because\n> +\t * relay_pack_data keeps the last byte to itself, we make the buffer 1\n> +\t * byte bigger than the intended maximum write size.\n> +\t */\n> +\tchar buffer[(LARGE_PACKET_DATA_MAX - 1) + 1];\n\nI don't have an opinion on the \"-1 / +1\" thing mentioned elsewhere. What\nI care much more about is that the comment explains what is going on,\nand what you wrote here is easy to understand.\n\n> @@ -269,7 +275,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n>  \t\t\t     const struct string_list *uri_protocols)\n>  {\n>  \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n> -\tstruct output_state output_state = { { 0 } };\n> +\tstruct output_state *output_state = xcalloc(1, sizeof(struct output_state));\n\nI think this hunk is worth doing. As Ævar noted, we do sometimes have\npretty big stack variables elsewhere, so we might have been able to get\naway with it here.  But running out of stack can be unpredictable and\nannoying. Given how easy and low-cost it is to put it on the heap here,\nI'm glad to just do it preemptively.\n\n> @@ -438,11 +444,12 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n>  \t}\n>  \n>  \t/* flush the data */\n> -\tif (output_state.used > 0) {\n> -\t\tsend_client_data(1, output_state.buffer, output_state.used,\n> +\tif (output_state->used > 0) {\n> +\t\tsend_client_data(1, output_state->buffer, output_state->used,\n>  \t\t\t\t pack_data->use_sideband);\n>  \t\tfprintf(stderr, \"flushed.\\n\");\n>  \t}\n> +\tfree(output_state);\n>  \tif (pack_data->use_sideband)\n>  \t\tpacket_flush(1);\n>  \treturn;\n\nThere's a \"fail:\" label just below here. But since it does not return,\nand just calls die(), then I agree you don't need to worry about freeing\nin that case (and there are no other returns from the function). So this\nsingle free is sufficient.\n\n-Peff\n"},{"id":"444164","messageId":"xmqqa6h16414.fsf@gitster.g","threadId":"57074","inReplyTo":"20211214194626.33814-2-jacob@gitlab.com","subject":"Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-15T19:50:47Z","receivedAt":"2021-12-15T19:50:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Vosmaer <jacob@gitlab.com> writes:\n\n> When serving a fetch, git upload-pack copies data from a git\n> pack-objects stdout pipe to its stdout. This commit increases the size\n> of the buffer used for that copying from 8192 to 65515, the maximum\n> sideband-64k packet size.\n\nThanks.  I agree with others that extra configurability is not\nneeded in this case, and allocating this on the heap (as long as we\ncorrectly deallocate it when we are done) is the right thing.\n\nWill queue.  Thanks.\n"},{"id":"444165","messageId":"003001d7f1ee$48c88410$da598c30$@nexbridge.com","threadId":"57074","inReplyTo":"xmqqa6h16414.fsf@gitster.g","subject":"RE: [PATCH v2 1/1] upload-pack.c: increase output buffer size","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-12-15T19:59:29Z","receivedAt":"2021-12-15T19:59:50Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 15, 2021 2:51 PM, Junio C Hamano wrote:\n> Jacob Vosmaer <jacob@gitlab.com> writes:\n> \n> > When serving a fetch, git upload-pack copies data from a git\n> > pack-objects stdout pipe to its stdout. This commit increases the size\n> > of the buffer used for that copying from 8192 to 65515, the maximum\n> > sideband-64k packet size.\n> \n> Thanks.  I agree with others that extra configurability is not needed in\nthis\n> case, and allocating this on the heap (as long as we correctly deallocate\nit\n> when we are done) is the right thing.\n\nThis is likely to break NonStop as our sideband packet size is less than\n64K.\n-Randall\n\n"},{"id":"444166","messageId":"CADMWQoPUEMGvhg-gqZHvsModKGomTYOP7h0SGv=9CaLMJdEm0g@mail.gmail.com","threadId":"57074","inReplyTo":"003001d7f1ee$48c88410$da598c30$@nexbridge.com","subject":"Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-12-15T20:24:19Z","receivedAt":"2021-12-15T20:24:31Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"On Wed, Dec 15, 2021 at 8:59 PM <rsbecker@nexbridge.com> wrote:\n\n> This is likely to break NonStop as our sideband packet size is less than\n> 64K.\n\nCould you elaborate on what you expect to break?\n"},{"id":"444167","messageId":"003301d7f1f3$c1e24f20$45a6ed60$@nexbridge.com","threadId":"57074","inReplyTo":"CADMWQoPUEMGvhg-gqZHvsModKGomTYOP7h0SGv=9CaLMJdEm0g@mail.gmail.com","subject":"RE: [PATCH v2 1/1] upload-pack.c: increase output buffer size","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-12-15T20:38:40Z","receivedAt":"2021-12-15T20:38:59Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 15, 2021 3:24 PM, Jacob Vosmaer wrote:\n> To: rsbecker@nexbridge.com\n> Cc: Junio C Hamano <gitster@pobox.com>; git@vger.kernel.org;\n> avarab@gmail.com; peff@peff.net\n> Subject: Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size\n> \n> On Wed, Dec 15, 2021 at 8:59 PM <rsbecker@nexbridge.com> wrote:\n> \n> > This is likely to break NonStop as our sideband packet size is less\n> > than 64K.\n> \n> Could you elaborate on what you expect to break?\n\nThe maximum I/O size on the platform is 56Kb. Anything larger will fail. That's why we use xread and xwrite for non-buffered I/O.\n-Randall\n\n"},{"id":"444169","messageId":"CADMWQoMJ_BBzwSsaCBwLjaWuu=L2=f_nWSemR-pctBMpMTsgbA@mail.gmail.com","threadId":"57074","inReplyTo":"003301d7f1f3$c1e24f20$45a6ed60$@nexbridge.com","subject":"Re: [PATCH v2 1/1] upload-pack.c: increase output buffer size","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-12-15T20:45:19Z","receivedAt":"2021-12-15T20:45:33Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"On Wed, Dec 15, 2021 at 9:38 PM <rsbecker@nexbridge.com> wrote:\n>\n> The maximum I/O size on the platform is 56Kb. Anything larger will fail. That's why we use xread and xwrite for non-buffered I/O.\n\nThat sounds like it's orthogonal to what this patch is changing.\nRegardless of this patch, the actual writes are performed by\nwrite_or_die, which calls write_in_full. In turn, write_in_full calls\nxwrite however often is necessary. We are not making any assumptions\nanywhere about how large individual write syscalls turn out. Git\npackets use userspace buffers so we can read and write them no matter\nwhat the underlying IO sizes are.\n"},{"id":"444172","messageId":"003401d7f1fb$976e1be0$c64a53a0$@nexbridge.com","threadId":"57074","inReplyTo":"CADMWQoMJ_BBzwSsaCBwLjaWuu=L2=f_nWSemR-pctBMpMTsgbA@mail.gmail.com","subject":"RE: [PATCH v2 1/1] upload-pack.c: increase output buffer size","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-12-15T21:34:45Z","receivedAt":"2021-12-15T21:35:01Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 15, 2021 3:45 PM, Jacob Vosmaer wrote:\n> On Wed, Dec 15, 2021 at 9:38 PM <rsbecker@nexbridge.com> wrote:\n> >\n> > The maximum I/O size on the platform is 56Kb. Anything larger will fail.\n> That's why we use xread and xwrite for non-buffered I/O.\n> \n> That sounds like it's orthogonal to what this patch is changing.\n> Regardless of this patch, the actual writes are performed by write_or_die,\n> which calls write_in_full. In turn, write_in_full calls xwrite however often is\n> necessary. We are not making any assumptions anywhere about how large\n> individual write syscalls turn out. Git packets use userspace buffers so we can\n> read and write them no matter what the underlying IO sizes are.\n\nIn that case, it works for me. xread/xwrite know how to deal with the situation. Thanks for the clarification.\n-Randall\n\n"}]}