From: Jeff King <peff@peff.net>
To: Jacob Vosmaer <jacob@gitlab.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable
Date: Tue, 14 Dec 2021 10:37:47 -0500 [thread overview]
Message-ID: <Ybi6SwndUHLs27bO@coredump.intra.peff.net> (raw)
In-Reply-To: <20211213132345.26310-2-jacob@gitlab.com>
On Mon, Dec 13, 2021 at 02:23:45PM +0100, Jacob Vosmaer wrote:
> The current size of the buffer is 8192+1. The '+1' is a technical
> detail orthogonal to this change. On GitLab.com we use GitLab's
> pack-objects cache which does writes of 65515 bytes. Because of the
> default 8KB buffer size, propagating these cache writes requires 8
> pipe reads and 8 pipe writes from git-upload-pack, and 8 pipe reads
> from Gitaly (our Git RPC service). If we increase the size of the
> buffer to the maximum Git packet size, we need only 1 pipe read and 1
> pipe write in git-upload-pack, and 1 pipe read in Gitaly to transfer
> the same amount of data. In benchmarks with a pure fetch and 100%
> cache hit rate workload we are seeing CPU utilization reductions of
> over 30%.
I was curious to reproduce this locally, so I came up with:
(
printf "003fwant %s side-band-64k" $(git rev-parse HEAD)
printf 0000
echo '0009done'
) |
git -c uploadpack.packobjectsHook='cat objects/pack/pack-*.pack; :' upload-pack . |
pv >/dev/null
which hackily simulates the server side of your "cached" case. :) I ran
it on a fully-packed clone of linux.git.
It gets about 2.3GB/s with the tip of 'master' and 3.2GB/s with the
equivalent of your patch (using LARGE_PACKET_DATA_MAX). So definitely an
improvement.
Without the cached case (so actually running pack-objects, albeit a
pretty quick one because of bitmaps and pack-reuse), the timings are
about the same (171MB/s versus 174MB/s, but really it's just pegging a
CPU running pack-objects). So it would be fine to just do this
unconditionally, I think.
Looking at strace, the other thing I notice is that we write() the
packet header separately in send_sideband(), which doubles the number of
syscalls. I hackily re-wrote this to use writev() instead (patch below),
but it doesn't seem to actually help much (maybe a curiosity to explore
further, but definitely not something to hold up your patch).
-Peff
---
diff --git a/sideband.c b/sideband.c
index 85bddfdcd4..d0945507a3 100644
--- a/sideband.c
+++ b/sideband.c
@@ -5,6 +5,11 @@
#include "help.h"
#include "pkt-line.h"
+/* hack; should go in git-compat-util, and should provide compat
+ * wrapper around regular write()
+ */
+#include <sys/uio.h>
+
struct keyword_entry {
/*
* We use keyword as config key so it should be a single alphanumeric word.
@@ -257,6 +262,7 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma
while (sz) {
unsigned n;
+ struct iovec iov[2];
char hdr[5];
n = sz;
@@ -265,12 +271,17 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma
if (0 <= band) {
xsnprintf(hdr, sizeof(hdr), "%04x", n + 5);
hdr[4] = band;
- write_or_die(fd, hdr, 5);
+ iov[0].iov_base = hdr;
+ iov[0].iov_len = 5;
} else {
xsnprintf(hdr, sizeof(hdr), "%04x", n + 4);
- write_or_die(fd, hdr, 4);
+ iov[0].iov_base = hdr;
+ iov[0].iov_len = 4;
}
- write_or_die(fd, p, n);
+ iov[1].iov_base = (void *)p;
+ iov[1].iov_len = n;
+ /* should check for errors, but also short writes and EINTR, etc */
+ writev(fd, iov, 2);
p += n;
sz -= n;
}
diff --git a/upload-pack.c b/upload-pack.c
index c78d55bc67..111de8c60c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -194,7 +194,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
}
struct output_state {
- char buffer[8193];
+ char buffer[LARGE_PACKET_DATA_MAX];
int used;
unsigned packfile_uris_started : 1;
unsigned packfile_started : 1;
next prev parent reply other threads:[~2021-12-14 15:37 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-13 13:23 [PATCH 0/1] Make upload-pack pack write size configurable Jacob Vosmaer
2021-12-13 13:23 ` [PATCH 1/1] upload-pack.c: make output buffer " Jacob Vosmaer
2021-12-14 12:08 ` Ævar Arnfjörð Bjarmason
2021-12-14 15:08 ` Jeff King
2021-12-14 19:46 ` [PATCH v2 0/1] upload-pack.c: increase output buffer size Jacob Vosmaer
2021-12-14 19:46 ` [PATCH v2 1/1] " Jacob Vosmaer
2021-12-14 20:41 ` Ævar Arnfjörð Bjarmason
2021-12-15 16:30 ` Jeff King
2021-12-15 19:50 ` Junio C Hamano
2021-12-15 19:59 ` rsbecker
2021-12-15 20:24 ` Jacob Vosmaer
2021-12-15 20:38 ` rsbecker
2021-12-15 20:45 ` Jacob Vosmaer
2021-12-15 21:34 ` rsbecker
2021-12-14 15:37 ` Jeff King [this message]
2021-12-14 20:04 ` [PATCH 1/1] upload-pack.c: make output buffer size configurable Jacob Vosmaer
2021-12-14 15:12 ` [PATCH 0/1] Make upload-pack pack write " Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ybi6SwndUHLs27bO@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=jacob@gitlab.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).