All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] upload-pack: buffer ref advertisement writes
@ 2021-08-24 14:02 Jacob Vosmaer
  2021-08-24 21:07 ` Taylor Blau
  0 siblings, 1 reply; 27+ messages in thread
From: Jacob Vosmaer @ 2021-08-24 14:02 UTC (permalink / raw)
  To: git; +Cc: Jacob Vosmaer

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.

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).

So having these buffers not only saves syscalls in upload-pack, it
also saves time in things that consume upload-pack's output.
---
 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);
+	}
 
 	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 {
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2021-09-02  9:19 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24 14:02 [PATCH 1/1] upload-pack: buffer ref advertisement writes Jacob Vosmaer
2021-08-24 21:07 ` Taylor Blau
2021-08-24 21:42   ` Junio C Hamano
2021-08-25  0:44     ` Jeff King
2021-08-26 10:02       ` Jacob Vosmaer
2021-08-26 10:06         ` [PATCH 1/2] pkt-line: add packet_fwrite Jacob Vosmaer
2021-08-26 10:06           ` [PATCH 2/2] upload-pack: use stdio in send_ref callbacks Jacob Vosmaer
2021-08-26 16:33             ` Junio C Hamano
2021-08-26 20:21               ` Junio C Hamano
2021-08-26 22:35               ` Taylor Blau
2021-08-26 23:24               ` Jeff King
2021-08-27 16:15                 ` Junio C Hamano
2021-08-31  9:34                   ` [PATCH v3 0/2] send_ref buffering Jacob Vosmaer
2021-08-31  9:34                     ` [PATCH v3 1/2] pkt-line: add stdio packet write functions Jacob Vosmaer
2021-08-31 10:37                       ` Jeff King
2021-08-31 18:13                       ` Junio C Hamano
2021-09-01 12:54                         ` [PATCH v4 0/2] send_ref buffering Jacob Vosmaer
2021-09-01 12:54                           ` [PATCH v4 1/2] pkt-line: add stdio packet write functions Jacob Vosmaer
2021-09-01 12:54                           ` [PATCH v4 2/2] upload-pack: use stdio in send_ref callbacks Jacob Vosmaer
2021-09-02  9:18                           ` [PATCH v4 0/2] send_ref buffering Jeff King
2021-08-31  9:34                     ` [PATCH v3 2/2] upload-pack: use stdio in send_ref callbacks Jacob Vosmaer
2021-08-31 10:25                     ` [PATCH v3 0/2] send_ref buffering Jeff King
2021-08-31 13:08                       ` Jacob Vosmaer
2021-08-31 17:44                         ` Jacob Vosmaer
2021-09-01  0:15                         ` Jeff King
2021-08-26 23:32             ` [PATCH 2/2] upload-pack: use stdio in send_ref callbacks Jeff King
2021-08-26 16:33           ` [PATCH 1/2] pkt-line: add packet_fwrite Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.