| Message ID | 20210824140259.89332-1-jacob@gitlab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] upload-pack: buffer ref advertisement writes | expand |
On Tue, Aug 24, 2021 at 04:02:59PM +0200, Jacob Vosmaer wrote: > In both protocol v0 and v2, upload-pack writes one pktline packet per > advertised ref to stdout. That means one or two write(2) syscalls per > ref. This is problematic if these writes become network sends with > high overhead. > > This change adds a strbuf buffer to the send_ref callbacks of the v0 > ref advertisement and v2's ls-refs. Instead of writing directly to > stdout, send_ref now writes into the buffer, and then checks if there > are more than 32768 bytes in the buffer. If so we flush the buffer to > stdout. Are there any consumers that rely on having information early (where buffering would be a detriment to them)? > To give an example: I set up a single-threaded loop that calls > ls-remote (with HTTP and protocol v2) on a local GitLab instance, on a > repository with 11K refs. When I switch from Git v2.32.0 to this > patch, I see a 50% reduction in CPU time for Git, and 75% for Gitaly > (GitLab's Git RPC service). Hmm. Seeing a reduction in CPU time is surprising to me: do you have an explanation for why the time-savings isn't coming purely from "system" (i.e., any blocking I/O)? > So having these buffers not only saves syscalls in upload-pack, it > also saves time in things that consume upload-pack's output. I don't think this explains it, since any time spent waiting on upload-pack output should be counted against the system bucket, not CPU time. > --- > ls-refs.c | 13 ++++++++++++- > upload-pack.c | 18 +++++++++++++++--- > 2 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/ls-refs.c b/ls-refs.c > index 88f6c3f60d..7b9d5813bf 100644 > --- a/ls-refs.c > +++ b/ls-refs.c > @@ -7,6 +7,8 @@ > #include "pkt-line.h" > #include "config.h" > > +#define OUT_WRITE_SIZE 32768 > + > static int config_read; > static int advertise_unborn; > static int allow_unborn; > @@ -65,6 +67,7 @@ struct ls_refs_data { > unsigned peel; > unsigned symrefs; > struct strvec prefixes; > + struct strbuf out; > unsigned unborn : 1; > }; > > @@ -105,7 +108,12 @@ static int send_ref(const char *refname, const struct object_id *oid, > } > > strbuf_addch(&refline, '\n'); > - packet_write(1, refline.buf, refline.len); > + > + packet_buf_write_len(&data->out, refline.buf, refline.len); > + if (data->out.len >= OUT_WRITE_SIZE) { > + write_or_die(1, data->out.buf, data->out.len); > + strbuf_reset(&data->out); > + } None of this looks wrong to me, but it might be nice to teach the packet writer a buffered mode that would handle this for us. It would be especially nice to bolt the final flush onto packet_flush(), since it is otherwise easy to forget to do. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: >> @@ -105,7 +108,12 @@ static int send_ref(const char *refname, const struct object_id *oid, >> } >> >> strbuf_addch(&refline, '\n'); >> - packet_write(1, refline.buf, refline.len); >> + >> + packet_buf_write_len(&data->out, refline.buf, refline.len); >> + if (data->out.len >= OUT_WRITE_SIZE) { >> + write_or_die(1, data->out.buf, data->out.len); >> + strbuf_reset(&data->out); This is somewhat unexpected. When one introduces size limit, it is more natural to make it upper bound and arrange the code more like if (currently buffered plus new data exceeds size limit) { flush the currently buffered data; if (new data alone exceeds size limit) flush the new data; else buffer the new data; } else { buffer the new data; } When a single packet exceeds the size limit, you'd end up making an oversize write() call anyway, but otherwise it would keep the write below the limit, not slightly above the limit, which makes it much easier to justify why the limit exists at the level it is set. Also, why is it 32k? Our jumbo packet limit is 65k, I think, and it may probably be easier to explain if we matched it. >> + } > > None of this looks wrong to me, but it might be nice to teach the > packet writer a buffered mode that would handle this for us. It would be > especially nice to bolt the final flush onto packet_flush(), since it is > otherwise easy to forget to do. FWIW, the packet-line part of the system was from the beginning written with an eye to allow buffering until _flush() comes; we may have added some buggy conversation path that deadlocks if we make the non-flush packets fully buffered, so there may need some fixes, but I do not expect the fallout would be too hard to diagnose. It may be worth trying that avenue first before piling on the user level buffering like this patch does. Thanks.
On Tue, Aug 24, 2021 at 02:42:20PM -0700, Junio C Hamano wrote: > > None of this looks wrong to me, but it might be nice to teach the > > packet writer a buffered mode that would handle this for us. It would be > > especially nice to bolt the final flush onto packet_flush(), since it is > > otherwise easy to forget to do. > > FWIW, the packet-line part of the system was from the beginning > written with an eye to allow buffering until _flush() comes; we may > have added some buggy conversation path that deadlocks if we make > the non-flush packets fully buffered, so there may need some fixes, > but I do not expect the fallout would be too hard to diagnose. > > It may be worth trying that avenue first before piling on the user > level buffering like this patch does. Yeah, I had the same thought. It also feels like this is a problem already solved by stdio. I.e., a lot of the packet_* functions can handle descriptors or strbufs. Why not "FILE *" handles? It would probably involve using the original descriptor _and_ the filehandle in some cases (e.g., ref advertisement over the handle, and then muxing pack-objects output straight to the descriptor). But that's OK as long we are sensible about flushing at the right moments. It may not be much less complex than just implementing buffering in the packet_* interfaces, though. The tricky part is likely to be the interface (not itself, but avoiding repetition between all the fd/strbuf/buffered variants). -Peff
> Are there any consumers that rely on having information early (where > buffering would be a detriment to them)? I think not. > Hmm. Seeing a reduction in CPU time is surprising to me: do you have an > explanation for why the time-savings isn't coming purely from "system" > (i.e., any blocking I/O)? Pipe writes only block in the IO sense if the pipe buffer is full. Otherwise they seem to spend their time in spinlocks and copying into the page buffer. In my benchmark, I don't believe the pipe buffer was ever full. I'm not an expert on the Linux kernel; you can see CPU flame graphs in https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1257. > Yeah, I had the same thought. It also feels like this is a problem > already solved by stdio. I.e., a lot of the packet_* functions can > handle descriptors or strbufs. Why not "FILE *" handles? My colleague Patrick Steinhardt (cc) made the same suggestion off-list. I'll post an alternative patch set to this thread. Jacob On Wed, Aug 25, 2021 at 2:44 AM Jeff King <peff@peff.net> wrote: > > On Tue, Aug 24, 2021 at 02:42:20PM -0700, Junio C Hamano wrote: > > > > None of this looks wrong to me, but it might be nice to teach the > > > packet writer a buffered mode that would handle this for us. It would be > > > especially nice to bolt the final flush onto packet_flush(), since it is > > > otherwise easy to forget to do. > > > > FWIW, the packet-line part of the system was from the beginning > > written with an eye to allow buffering until _flush() comes; we may > > have added some buggy conversation path that deadlocks if we make > > the non-flush packets fully buffered, so there may need some fixes, > > but I do not expect the fallout would be too hard to diagnose. > > > > It may be worth trying that avenue first before piling on the user > > level buffering like this patch does. > > Yeah, I had the same thought. It also feels like this is a problem > already solved by stdio. I.e., a lot of the packet_* functions can > handle descriptors or strbufs. Why not "FILE *" handles? > > It would probably involve using the original descriptor _and_ the > filehandle in some cases (e.g., ref advertisement over the handle, and > then muxing pack-objects output straight to the descriptor). But that's > OK as long we are sensible about flushing at the right moments. > > It may not be much less complex than just implementing buffering in the > packet_* interfaces, though. The tricky part is likely to be the > interface (not itself, but avoiding repetition between all the > fd/strbuf/buffered variants). > > -Peff
diff --git a/ls-refs.c b/ls-refs.c index 88f6c3f60d..7b9d5813bf 100644 --- a/ls-refs.c +++ b/ls-refs.c @@ -7,6 +7,8 @@ #include "pkt-line.h" #include "config.h" +#define OUT_WRITE_SIZE 32768 + static int config_read; static int advertise_unborn; static int allow_unborn; @@ -65,6 +67,7 @@ struct ls_refs_data { unsigned peel; unsigned symrefs; struct strvec prefixes; + struct strbuf out; unsigned unborn : 1; }; @@ -105,7 +108,12 @@ static int send_ref(const char *refname, const struct object_id *oid, } strbuf_addch(&refline, '\n'); - packet_write(1, refline.buf, refline.len); + + packet_buf_write_len(&data->out, refline.buf, refline.len); + if (data->out.len >= OUT_WRITE_SIZE) { + write_or_die(1, data->out.buf, data->out.len); + strbuf_reset(&data->out); + } strbuf_release(&refline); return 0; @@ -145,6 +153,7 @@ int ls_refs(struct repository *r, struct strvec *keys, memset(&data, 0, sizeof(data)); strvec_init(&data.prefixes); + strbuf_init(&data.out, OUT_WRITE_SIZE); ensure_config_read(); git_config(ls_refs_config, NULL); @@ -171,7 +180,9 @@ int ls_refs(struct repository *r, struct strvec *keys, strvec_push(&data.prefixes, ""); for_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v, send_ref, &data, 0); + write_or_die(1, data.out.buf, data.out.len); packet_flush(1); + strbuf_release(&data.out); strvec_clear(&data.prefixes); return 0; } diff --git a/upload-pack.c b/upload-pack.c index 297b76fcb4..15f9aab4f6 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -42,6 +42,8 @@ #define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \ NOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF) +#define SEND_REF_OUT_WRITE_SIZE 32768 + /* Enum for allowed unadvertised object request (UOR) */ enum allow_uor { /* Allow specifying sha1 if it is a ref tip. */ @@ -58,6 +60,7 @@ enum allow_uor { */ struct upload_pack_data { struct string_list symref; /* v0 only */ + struct strbuf send_ref_out; /* v0 only */ struct object_array want_obj; struct object_array have_obj; struct oid_array haves; /* v2 only */ @@ -126,6 +129,7 @@ static void upload_pack_data_init(struct upload_pack_data *data) struct string_list uri_protocols = STRING_LIST_INIT_DUP; struct object_array extra_edge_obj = OBJECT_ARRAY_INIT; struct string_list allowed_filters = STRING_LIST_INIT_DUP; + struct strbuf send_ref_out = STRBUF_INIT; memset(data, 0, sizeof(*data)); data->symref = symref; @@ -141,6 +145,7 @@ static void upload_pack_data_init(struct upload_pack_data *data) data->allow_filter_fallback = 1; data->tree_filter_max_depth = ULONG_MAX; packet_writer_init(&data->writer, 1); + data->send_ref_out = send_ref_out; data->keepalive = 5; data->advertise_sid = 0; @@ -158,6 +163,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data) object_array_clear(&data->extra_edge_obj); list_objects_filter_release(&data->filter_options); string_list_clear(&data->allowed_filters, 0); + strbuf_release(&data->send_ref_out); free((char *)data->pack_objects_hook); } @@ -1207,7 +1213,7 @@ static int send_ref(const char *refname, const struct object_id *oid, format_symref_info(&symref_info, &data->symref); format_session_id(&session_id, data); - packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n", + packet_buf_write(&data->send_ref_out, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n", oid_to_hex(oid), refname_nons, 0, capabilities, (data->allow_uor & ALLOW_TIP_SHA1) ? @@ -1223,11 +1229,15 @@ static int send_ref(const char *refname, const struct object_id *oid, strbuf_release(&symref_info); strbuf_release(&session_id); } else { - packet_write_fmt(1, "%s %s\n", oid_to_hex(oid), refname_nons); + packet_buf_write(&data->send_ref_out, "%s %s\n", oid_to_hex(oid), refname_nons); } capabilities = NULL; if (!peel_iterated_oid(oid, &peeled)) - packet_write_fmt(1, "%s %s^{}\n", oid_to_hex(&peeled), refname_nons); + packet_buf_write(&data->send_ref_out, "%s %s^{}\n", oid_to_hex(&peeled), refname_nons); + if (data->send_ref_out.len >= SEND_REF_OUT_WRITE_SIZE) { + write_or_die(1, data->send_ref_out.buf, data->send_ref_out.len); + strbuf_reset(&data->send_ref_out); + } return 0; } @@ -1346,8 +1356,10 @@ void upload_pack(struct upload_pack_options *options) if (options->advertise_refs || !data.stateless_rpc) { reset_timeout(data.timeout); + strbuf_grow(&data.send_ref_out, SEND_REF_OUT_WRITE_SIZE); head_ref_namespaced(send_ref, &data); for_each_namespaced_ref(send_ref, &data); + write_or_die(1, data.send_ref_out.buf, data.send_ref_out.len); advertise_shallow_grafts(1); packet_flush(1); } else {