"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
fix got_opentemp() overhead of got-read-pack
To:
gameoftrees@openbsd.org
Date:
Fri, 31 Dec 2021 19:02:31 +0100

Download raw body.

Thread
Turns out the problem where gotadmin pack spends about 30% of its time
opening temporary files can be fixed independently from using mmap.

With this patch we open temporary files used during delta application
just once, instead of opening new temporary files during every request
for a blob object or a raw object.

This makes gotadmin pack significantly faster, and will likely also
speed up other tasks which need to load a lot of blobs.

ok?

diff c0df59665de91324eeab1808c6c4e41343f21789 /home/stsp/src/got
blob - ce62cbf5e948bfee2b29b0f0524208b1a15a93e9
file + lib/got_lib_privsep.h
--- lib/got_lib_privsep.h
+++ lib/got_lib_privsep.h
@@ -546,7 +546,7 @@ const struct got_error *got_privsep_send_tag(struct im
 const struct got_error *got_privsep_recv_tag(struct got_tag_object **,
     struct imsgbuf *);
 const struct got_error *got_privsep_init_pack_child(struct imsgbuf *,
-    struct got_pack *, struct got_packidx *);
+    struct got_pack *, struct got_packidx *, int, int);
 const struct got_error *got_privsep_send_packed_obj_req(struct imsgbuf *, int,
     struct got_object_id *);
 const struct got_error *got_privsep_send_packed_raw_obj_req(struct imsgbuf *,
blob - a4cf395476aee75f57a0281a2184b91a09e360e3
file + lib/object.c
--- lib/object.c
+++ lib/object.c
@@ -173,51 +173,18 @@ request_packed_object_raw(uint8_t **outbuf, off_t *siz
 	const struct got_error *err = NULL;
 	struct imsgbuf *ibuf = pack->privsep_child->ibuf;
 	int outfd_child;
-	int basefd, accumfd; /* temporary files for delta application */
 
-	basefd = got_opentempfd();
-	if (basefd == -1)
-		return got_error_from_errno("got_opentempfd");
-
-	accumfd = got_opentempfd();
-	if (accumfd == -1) {
-		close(basefd);
-		return got_error_from_errno("got_opentempfd");
-	}
-
 	outfd_child = dup(outfd);
-	if (outfd_child == -1) {
-		err = got_error_from_errno("dup");
-		close(basefd);
-		close(accumfd);
-		return err;
-	}
+	if (outfd_child == -1)
+		return got_error_from_errno("dup");
 
 	err = got_privsep_send_packed_raw_obj_req(ibuf, idx, id);
 	if (err) {
-		close(basefd);
-		close(accumfd);
 		close(outfd_child);
 		return err;
 	}
 
 	err = got_privsep_send_raw_obj_outfd(ibuf, outfd_child);
-	if (err) {
-		close(basefd);
-		close(accumfd);
-		return err;
-	}
-
-
-	err = got_privsep_send_tmpfd(pack->privsep_child->ibuf,
-	    basefd);
-	if (err) {
-		close(accumfd);
-		return err;
-	}
-
-	err = got_privsep_send_tmpfd(pack->privsep_child->ibuf,
-	    accumfd);
 	if (err)
 		return err;
 
@@ -247,6 +214,7 @@ start_pack_privsep_child(struct got_pack *pack, struct
 	int imsg_fds[2];
 	pid_t pid;
 	struct imsgbuf *ibuf;
+	int basefd = -1, accumfd = -1;
 
 	ibuf = calloc(1, sizeof(*ibuf));
 	if (ibuf == NULL)
@@ -282,7 +250,19 @@ start_pack_privsep_child(struct got_pack *pack, struct
 	imsg_init(ibuf, imsg_fds[0]);
 	pack->privsep_child->ibuf = ibuf;
 
-	err = got_privsep_init_pack_child(ibuf, pack, packidx);
+	/* Create temporary files used during delta application. */
+	basefd = got_opentempfd();
+	if (basefd == -1) {
+		err = got_error_from_errno("got_opentempfd");
+		goto done;
+	}
+	accumfd = got_opentempfd();
+	if (accumfd == -1) {
+		err = got_error_from_errno("got_opentempfd");
+		goto done;
+	}
+
+	err = got_privsep_init_pack_child(ibuf, pack, packidx, basefd, accumfd);
 	if (err) {
 		const struct got_error *child_err;
 		err = got_privsep_send_stop(pack->privsep_child->imsg_fd);
@@ -296,6 +276,10 @@ done:
 		free(ibuf);
 		free(pack->privsep_child);
 		pack->privsep_child = NULL;
+		if (basefd != -1)
+			close(basefd);
+		if (accumfd != -1)
+			close(accumfd);
 	}
 	return err;
 }
@@ -1192,15 +1176,7 @@ request_packed_blob(uint8_t **outbuf, size_t *size, si
 {
 	const struct got_error *err = NULL;
 	int outfd_child;
-	int basefd, accumfd; /* temporary files for delta application */
 
-	basefd = got_opentempfd();
-	if (basefd == -1)
-		return got_error_from_errno("got_opentempfd");
-	accumfd = got_opentempfd();
-	if (accumfd == -1)
-		return got_error_from_errno("got_opentempfd");
-
 	outfd_child = dup(outfd);
 	if (outfd_child == -1)
 		return got_error_from_errno("dup");
@@ -1212,23 +1188,9 @@ request_packed_blob(uint8_t **outbuf, size_t *size, si
 	err = got_privsep_send_blob_outfd(pack->privsep_child->ibuf,
 	    outfd_child);
 	if (err) {
-		close(basefd);
-		close(accumfd);
 		return err;
 	}
 
-	err = got_privsep_send_tmpfd(pack->privsep_child->ibuf,
-	    basefd);
-	if (err) {
-		close(accumfd);
-		return err;
-	}
-
-	err = got_privsep_send_tmpfd(pack->privsep_child->ibuf,
-	    accumfd);
-	if (err)
-		return err;
-
 	err = got_privsep_recv_blob(outbuf, size, hdrlen,
 	    pack->privsep_child->ibuf);
 	if (err)
blob - 5e229ab44b13c2e63ed9807f70837898eeda1e43
file + lib/pack_create.c
--- lib/pack_create.c
+++ lib/pack_create.c
@@ -249,7 +249,6 @@ encode_delta(struct got_pack_meta *m, struct got_raw_o
 	return NULL;
 }
 
-
 static const struct got_error *
 pick_deltas(struct got_pack_meta **meta, int nmeta, int nours,
     FILE *delta_cache, struct got_repository *repo,
blob - 6cf7e9a2ee72ca78311d0d9a16088324b574dfd6
file + lib/privsep.c
--- lib/privsep.c
+++ lib/privsep.c
@@ -1972,7 +1972,7 @@ got_privsep_recv_tag(struct got_tag_object **tag, stru
 
 const struct got_error *
 got_privsep_init_pack_child(struct imsgbuf *ibuf, struct got_pack *pack,
-    struct got_packidx *packidx)
+    struct got_packidx *packidx, int basefd, int accumfd)
 {
 	const struct got_error *err = NULL;
 	struct got_imsg_packidx ipackidx;
@@ -2008,6 +2008,16 @@ got_privsep_init_pack_child(struct imsgbuf *ibuf, stru
 		return err;
 	}
 
+	err = got_privsep_send_tmpfd(ibuf, basefd);
+	if (err) {
+		close(accumfd);
+		return err;
+	}
+
+	err = got_privsep_send_tmpfd(ibuf, accumfd);
+	if (err)
+		return err;
+
 	return flush_imsg(ibuf);
 }
 
blob - 8bbfe87a70dfd1f3c936dd572b1ab0dcf33aff61
file + libexec/got-read-pack/got-read-pack.c
--- libexec/got-read-pack/got-read-pack.c
+++ libexec/got-read-pack/got-read-pack.c
@@ -290,12 +290,13 @@ done:
 
 static const struct got_error *
 blob_request(struct imsg *imsg, struct imsgbuf *ibuf, struct got_pack *pack,
-    struct got_packidx *packidx, struct got_object_cache *objcache)
+    struct got_packidx *packidx, struct got_object_cache *objcache,
+    FILE *basefile, FILE *accumfile)
 {
 	const struct got_error *err = NULL;
 	struct got_imsg_packed_object iobj;
 	struct got_object *obj = NULL;
-	FILE *outfile = NULL, *basefile = NULL, *accumfile = NULL;
+	FILE *outfile = NULL;
 	struct got_object_id id;
 	size_t datalen;
 	uint64_t blob_size;
@@ -320,12 +321,6 @@ blob_request(struct imsg *imsg, struct imsgbuf *ibuf, 
 	err = receive_file(&outfile, ibuf, GOT_IMSG_BLOB_OUTFD);
 	if (err)
 		goto done;
-	err = receive_file(&basefile, ibuf, GOT_IMSG_TMPFD);
-	if (err)
-		goto done;
-	err = receive_file(&accumfile, ibuf, GOT_IMSG_TMPFD);
-	if (err)
-		goto done;
 
 	if (obj->flags & GOT_OBJ_FLAG_DELTIFIED) {
 		err = got_pack_get_max_delta_object_size(&blob_size, obj, pack);
@@ -348,10 +343,6 @@ done:
 	free(buf);
 	if (outfile && fclose(outfile) == EOF && err == NULL)
 		err = got_error_from_errno("fclose");
-	if (basefile && fclose(basefile) == EOF && err == NULL)
-		err = got_error_from_errno("fclose");
-	if (accumfile && fclose(accumfile) == EOF && err == NULL)
-		err = got_error_from_errno("fclose");
 	got_object_close(obj);
 	if (err && err->code != GOT_ERR_PRIVSEP_PIPE)
 		got_privsep_send_error(ibuf, err);
@@ -775,12 +766,12 @@ done:
 static const struct got_error *
 raw_object_request(struct imsg *imsg, struct imsgbuf *ibuf,
     struct got_pack *pack, struct got_packidx *packidx,
-    struct got_object_cache *objcache)
+    struct got_object_cache *objcache, FILE *basefile, FILE *accumfile)
 {
 	const struct got_error *err = NULL;
 	uint8_t *buf = NULL;
 	uint64_t size = 0;
-	FILE *outfile = NULL, *basefile = NULL, *accumfile = NULL;
+	FILE *outfile = NULL;
 	struct got_imsg_packed_object iobj;
 	struct got_object *obj;
 	struct got_object_id id;
@@ -805,12 +796,6 @@ raw_object_request(struct imsg *imsg, struct imsgbuf *
 	err = receive_file(&outfile, ibuf, GOT_IMSG_RAW_OBJECT_OUTFD);
 	if (err)
 		return err;
-	err = receive_file(&basefile, ibuf, GOT_IMSG_TMPFD);
-	if (err)
-		goto done;
-	err = receive_file(&accumfile, ibuf, GOT_IMSG_TMPFD);
-	if (err)
-		goto done;
 
 	if (obj->flags & GOT_OBJ_FLAG_DELTIFIED) {
 		err = got_pack_get_max_delta_object_size(&size, obj, pack);
@@ -833,10 +818,6 @@ done:
 	free(buf);
 	if (outfile && fclose(outfile) == EOF && err == NULL)
 		err = got_error_from_errno("fclose");
-	if (basefile && fclose(basefile) == EOF && err == NULL)
-		err = got_error_from_errno("fclose");
-	if (accumfile && fclose(accumfile) == EOF && err == NULL)
-		err = got_error_from_errno("fclose");
 	got_object_close(obj);
 	if (err && err->code != GOT_ERR_PRIVSEP_PIPE)
 		got_privsep_send_error(ibuf, err);
@@ -999,6 +980,7 @@ main(int argc, char *argv[])
 	struct got_packidx *packidx = NULL;
 	struct got_pack *pack = NULL;
 	struct got_object_cache objcache;
+	FILE *basefile = NULL, *accumfile = NULL;
 
 	//static int attached;
 	//while (!attached) sleep(1);
@@ -1035,6 +1017,18 @@ main(int argc, char *argv[])
 		return 1;
 	}
 
+	err = receive_file(&basefile, &ibuf, GOT_IMSG_TMPFD);
+	if (err) {
+		got_privsep_send_error(&ibuf, err);
+		return 1;
+	}
+
+	err = receive_file(&accumfile, &ibuf, GOT_IMSG_TMPFD);
+	if (err) {
+		got_privsep_send_error(&ibuf, err);
+		return 1;
+	}
+
 	for (;;) {
 		imsg.fd = -1;
 
@@ -1060,7 +1054,7 @@ main(int argc, char *argv[])
 			break;
 		case GOT_IMSG_PACKED_RAW_OBJECT_REQUEST:
 			err = raw_object_request(&imsg, &ibuf, pack, packidx,
-			    &objcache);
+			    &objcache, basefile, accumfile);
 			break;
 		case GOT_IMSG_COMMIT_REQUEST:
 			err = commit_request(&imsg, &ibuf, pack, packidx,
@@ -1072,7 +1066,7 @@ main(int argc, char *argv[])
 			break;
 		case GOT_IMSG_BLOB_REQUEST:
 			err = blob_request(&imsg, &ibuf, pack, packidx,
-			    &objcache);
+			    &objcache, basefile, accumfile);
 			break;
 		case GOT_IMSG_TAG_REQUEST:
 			err = tag_request(&imsg, &ibuf, pack, packidx,
@@ -1100,6 +1094,10 @@ main(int argc, char *argv[])
 		got_pack_close(pack);
 	got_object_cache_close(&objcache);
 	imsg_clear(&ibuf);
+	if (basefile && fclose(basefile) == EOF && err == NULL)
+		err = got_error_from_errno("fclose");
+	if (accumfile && fclose(accumfile) == EOF && err == NULL)
+		err = got_error_from_errno("fclose");
 	if (err) {
 		if (!sigint_received && err->code != GOT_ERR_PRIVSEP_PIPE) {
 			fprintf(stderr, "%s: %s\n", getprogname(), err->msg);