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