rate-limiting of gotadmin progress output

Some time ago naddy pointed out that 'gotadmin cleanup' produces a huge amount of progress output which easily overwhelms slow terminals. The same is true for 'gotadmin pack'. The patch below uses a similar approach to the patch naddy suggested at the time. (naddy's patch was not applied because of naddy's own cosmetic concerns about his patch.) With a thin API layer on top of clock_gettime() and timespecs, progress updates are rate-limited to every 500 msec, which seems fine to me. I would have no problem with tweaking this timeout if needed. In my test case the volume of progress output from 'gotadmin pack -a' is reduced from 182Kb to 10Kb. The output volume of 'gotadmin cleanup' goes down from 736Kb to 2Kb. The run time of 'gotadmin cleanup' improves from 18 seconds to 12 seconds. I am leaving other progress reports alone for now. We can easily introduce more use of this new rate-limiting API over time. ok? diff 0f639468088c75c028b204d850ac5b1e5052dd1f 19f1646bd8e01a81d9c170af20efecd6c5a57ee7 blob - 7e44f914b454fcd320fd03610f840109df3f7539 blob + 8b431936ed43a5398c391d18637ae6dbc0f5348f --- got/Makefile +++ got/Makefile @@ -13,7 +13,7 @@ SRCS= got.c blame.c commit_graph.c delta.c diff.c \ diff_myers.c diff_output.c diff_output_plain.c \ diff_output_unidiff.c diff_output_edscript.c \ diff_patience.c send.c deltify.c pack_create.c dial.c \ - bloom.c murmurhash2.c + bloom.c murmurhash2.c ratelimit.c MAN = ${PROG}.1 got-worktree.5 git-repository.5 got.conf.5 blob - 47d9d105cc92076d9e33ac2abdf1adf89ab25dc9 blob + 781133bbc9837ad999231c521ae9da3239c0232b --- gotadmin/Makefile +++ gotadmin/Makefile @@ -8,7 +8,7 @@ SRCS= gotadmin.c \ inflate.c lockfile.c object.c object_cache.c object_create.c \ object_idset.c object_parse.c opentemp.c pack.c pack_create.c \ path.c privsep.c reference.c repository.c repository_admin.c \ - worktree_open.c sha1.c bloom.c murmurhash2.c + worktree_open.c sha1.c bloom.c murmurhash2.c ratelimit.c MAN = ${PROG}.1 CPPFLAGS = -I${.CURDIR}/../include -I${.CURDIR}/../lib blob - /dev/null blob + 9b95dcd8f24e8a9a1ad8900a16ece71c03e03a94 (mode 644) --- /dev/null +++ lib/got_lib_ratelimit.h @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2022 Stefan Sperling <stsp@openbsd.org> + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +struct got_ratelimit { + struct timespec last; + struct timespec interval; +}; + +void got_ratelimit_init(struct got_ratelimit *, time_t, unsigned int); +const struct got_error *got_ratelimit_check(int *, struct got_ratelimit *); blob - 5e229ab44b13c2e63ed9807f70837898eeda1e43 blob + 2e421bd245bb3702600302716d83053e5785c51a --- lib/pack_create.c +++ lib/pack_create.c @@ -20,6 +20,7 @@ #include <sys/tree.h> #include <sys/uio.h> #include <sys/stat.h> +#include <sys/time.h> #include <stdint.h> #include <imsg.h> @@ -27,6 +28,7 @@ #include <stdlib.h> #include <string.h> #include <sha1.h> +#include <time.h> #include <limits.h> #include <zlib.h> @@ -47,6 +49,7 @@ #include "got_lib_pack.h" #include "got_lib_privsep.h" #include "got_lib_repository.h" +#include "got_lib_ratelimit.h" #ifndef MIN #define MIN(_a,_b) ((_a) < (_b) ? (_a) : (_b)) @@ -249,12 +252,30 @@ encode_delta(struct got_pack_meta *m, struct got_raw_o return NULL; } +static const struct got_error * +report_progress(got_pack_progress_cb progress_cb, void *progress_arg, + struct got_ratelimit *rl, off_t packfile_size, int ncommits, + int nobj_total, int obj_deltify, int nobj_written) +{ + const struct got_error *err; + int elapsed; + if (progress_cb == NULL) + return NULL; + + err = got_ratelimit_check(&elapsed, rl); + if (err || !elapsed) + return err; + + return progress_cb(progress_arg, packfile_size, ncommits, + nobj_total, obj_deltify, nobj_written); +} + static const struct got_error * pick_deltas(struct got_pack_meta **meta, int nmeta, int nours, FILE *delta_cache, struct got_repository *repo, got_pack_progress_cb progress_cb, void *progress_arg, - got_cancel_cb cancel_cb, void *cancel_arg) + struct got_ratelimit *rl, got_cancel_cb cancel_cb, void *cancel_arg) { const struct got_error *err = NULL; struct got_pack_meta *m = NULL, *base = NULL; @@ -271,11 +292,10 @@ pick_deltas(struct got_pack_meta **meta, int nmeta, in if (err) break; } - if (progress_cb) { - err = progress_cb(progress_arg, 0L, nours, nmeta, i, 0); - if (err) - goto done; - } + err = report_progress(progress_cb, progress_arg, rl, + 0L, nours, nmeta, i, 0); + if (err) + goto done; m = meta[i]; if (m->obj_type == GOT_OBJ_TYPE_COMMIT || @@ -923,7 +943,7 @@ read_meta(struct got_pack_meta ***meta, int *nmeta, struct got_object_id **theirs, int ntheirs, struct got_object_id **ours, int nours, struct got_repository *repo, int loose_obj_only, got_pack_progress_cb progress_cb, void *progress_arg, - got_cancel_cb cancel_cb, void *cancel_arg) + struct got_ratelimit *rl, got_cancel_cb cancel_cb, void *cancel_arg) { const struct got_error *err = NULL; struct got_object_id **ids = NULL; @@ -964,12 +984,10 @@ read_meta(struct got_pack_meta ***meta, int *nmeta, loose_obj_only, cancel_cb, cancel_arg); if (err) goto done; - if (progress_cb) { - err = progress_cb(progress_arg, 0L, nours, - v.nmeta, 0, 0); - if (err) - goto done; - } + err = report_progress(progress_cb, progress_arg, rl, + 0L, nours, v.nmeta, 0, 0); + if (err) + goto done; } for (i = 0; i < ntheirs; i++) { @@ -990,12 +1008,10 @@ read_meta(struct got_pack_meta ***meta, int *nmeta, loose_obj_only, cancel_cb, cancel_arg); if (err) goto done; - if (progress_cb) { - err = progress_cb(progress_arg, 0L, nours, - v.nmeta, 0, 0); - if (err) - goto done; - } + err = report_progress(progress_cb, progress_arg, rl, + 0L, nours, v.nmeta, 0, 0); + if (err) + goto done; } for (i = 0; i < nobj; i++) { @@ -1003,12 +1019,12 @@ read_meta(struct got_pack_meta ***meta, int *nmeta, loose_obj_only, cancel_cb, cancel_arg); if (err) goto done; - if (progress_cb) { - err = progress_cb(progress_arg, 0L, nours, - v.nmeta, 0, 0); - if (err) - goto done; - } + if (err) + goto done; + err = report_progress(progress_cb, progress_arg, rl, + 0L, nours, v.nmeta, 0, 0); + if (err) + goto done; } for (i = 0; i < nours; i++) { @@ -1029,14 +1045,17 @@ read_meta(struct got_pack_meta ***meta, int *nmeta, loose_obj_only, cancel_cb, cancel_arg); if (err) goto done; - if (progress_cb) { - err = progress_cb(progress_arg, 0L, nours, - v.nmeta, 0, 0); - if (err) - goto done; - } + err = report_progress(progress_cb, progress_arg, rl, + 0L, nours, v.nmeta, 0, 0); + if (err) + goto done; } + if (progress_cb) { + err = progress_cb(progress_arg, 0L, nours, v.nmeta, 0, 0); + if (err) + goto done; + } done: for (i = 0; i < nobj; i++) { free(ids[i]); @@ -1136,6 +1155,7 @@ genpack(uint8_t *pack_sha1, FILE *packfile, FILE *delt struct got_pack_meta **meta, int nmeta, int nours, int use_offset_deltas, struct got_repository *repo, got_pack_progress_cb progress_cb, void *progress_arg, + struct got_ratelimit *rl, got_cancel_cb cancel_cb, void *cancel_arg) { const struct got_error *err = NULL; @@ -1167,12 +1187,10 @@ genpack(uint8_t *pack_sha1, FILE *packfile, FILE *delt goto done; qsort(meta, nmeta, sizeof(struct got_pack_meta *), write_order_cmp); for (i = 0; i < nmeta; i++) { - if (progress_cb) { - err = progress_cb(progress_arg, packfile_size, nours, - nmeta, nmeta, i); - if (err) - goto done; - } + err = report_progress(progress_cb, progress_arg, rl, + packfile_size, nours, nmeta, nmeta, i); + if (err) + goto done; m = meta[i]; m->off = ftello(packfile); err = got_object_raw_open(&raw, &outfd, repo, &m->id); @@ -1281,10 +1299,12 @@ genpack(uint8_t *pack_sha1, FILE *packfile, FILE *delt err = got_ferror(packfile, GOT_ERR_IO); packfile_size += SHA1_DIGEST_LENGTH; packfile_size += sizeof(struct got_packfile_hdr); - err = progress_cb(progress_arg, packfile_size, nours, - nmeta, nmeta, nmeta); - if (err) - goto done; + if (progress_cb) { + err = progress_cb(progress_arg, packfile_size, nours, + nmeta, nmeta, nmeta); + if (err) + goto done; + } done: if (delta_file && fclose(delta_file) == EOF && err == NULL) err = got_error_from_errno("fclose"); @@ -1307,9 +1327,13 @@ got_pack_create(uint8_t *packsha1, FILE *packfile, struct got_pack_meta **meta; int nmeta; FILE *delta_cache = NULL; + struct got_ratelimit rl; + got_ratelimit_init(&rl, 0, 500); + err = read_meta(&meta, &nmeta, theirs, ntheirs, ours, nours, repo, - loose_obj_only, progress_cb, progress_arg, cancel_cb, cancel_arg); + loose_obj_only, progress_cb, progress_arg, &rl, + cancel_cb, cancel_arg); if (err) return err; @@ -1326,7 +1350,7 @@ got_pack_create(uint8_t *packsha1, FILE *packfile, if (nmeta > 0) { err = pick_deltas(meta, nmeta, nours, delta_cache, repo, - progress_cb, progress_arg, cancel_cb, cancel_arg); + progress_cb, progress_arg, &rl, cancel_cb, cancel_arg); if (err) goto done; if (fseeko(delta_cache, 0L, SEEK_SET) == -1) { @@ -1336,7 +1360,7 @@ got_pack_create(uint8_t *packsha1, FILE *packfile, } err = genpack(packsha1, packfile, delta_cache, meta, nmeta, nours, 1, - repo, progress_cb, progress_arg, cancel_cb, cancel_arg); + repo, progress_cb, progress_arg, &rl, cancel_cb, cancel_arg); if (err) goto done; done: blob - /dev/null blob + 219e7905e4765ae964a3ab7a1e8145d1ce344c2f (mode 644) --- /dev/null +++ lib/ratelimit.c @@ -0,0 +1,55 @@ +/* + * Copyright (c) 2022 Stefan Sperling <stsp@openbsd.org> + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include <sys/time.h> + +#include <stdio.h> +#include <limits.h> +#include <string.h> +#include <time.h> + +#include "got_lib_ratelimit.h" + +#include "got_error.h" + +void +got_ratelimit_init(struct got_ratelimit *rl, time_t interval_sec, + unsigned int interval_msec) +{ + memset(rl, 0, sizeof(*rl)); + rl->interval.tv_sec = interval_sec; + rl->interval.tv_nsec = interval_msec * 1000000UL; +} + +const struct got_error * +got_ratelimit_check(int *elapsed, struct got_ratelimit *rl) +{ + struct timespec now, delta; + + if (clock_gettime(CLOCK_MONOTONIC, &now) == -1) + return got_error_from_errno("clock_gettime"); + + if (timespecisset(&rl->last)) { + timespecsub(&now, &rl->last, &delta); + *elapsed = timespeccmp(&delta, &rl->interval, >=) ? 1 : 0; + } else + *elapsed = 1; + + if (*elapsed) + rl->last = now; + + return NULL; +} blob - 4e1253bcafe34044a40c52250b6a96a552b4b609 blob + 40af887d57530bc64cde9dbfdd26426592ae93c3 --- lib/repository_admin.c +++ lib/repository_admin.c @@ -54,6 +54,7 @@ #include "got_lib_pack_create.h" #include "got_lib_sha1.h" #include "got_lib_lockfile.h" +#include "got_lib_ratelimit.h" #ifndef nitems #define nitems(_a) (sizeof((_a)) / sizeof((_a)[0])) @@ -598,9 +599,27 @@ done: } static const struct got_error * +report_cleanup_progress(got_cleanup_progress_cb progress_cb, + void *progress_arg, struct got_ratelimit *rl, + int nloose, int ncommits, int npurged) +{ + const struct got_error *err; + int elapsed; + + if (progress_cb == NULL) + return NULL; + + err = got_ratelimit_check(&elapsed, rl); + if (err || !elapsed) + return err; + + return progress_cb(progress_arg, nloose, ncommits, npurged); +} + +static const struct got_error * get_loose_object_ids(struct got_object_idset **loose_ids, off_t *ondisk_size, got_cleanup_progress_cb progress_cb, void *progress_arg, - struct got_repository *repo) + struct got_ratelimit *rl, struct got_repository *repo) { const struct got_error *err = NULL; char *path_objects = NULL, *path = NULL; @@ -688,13 +707,11 @@ get_loose_object_ids(struct got_object_idset **loose_i err = got_object_idset_add(*loose_ids, &id, NULL); if (err) goto done; - if (progress_cb) { - err = progress_cb(progress_arg, - got_object_idset_num_elements(*loose_ids), - -1, -1); - if (err) - goto done; - } + err = report_cleanup_progress(progress_cb, + progress_arg, rl, + got_object_idset_num_elements(*loose_ids), -1, -1); + if (err) + goto done; } if (closedir(dir) != 0) { @@ -878,7 +895,8 @@ static const struct got_error * load_commit_or_tag(struct got_object_idset *loose_ids, int *ncommits, int *npacked, struct got_object_idset *traversed_ids, struct got_object_id *id, struct got_repository *repo, - got_cleanup_progress_cb progress_cb, void *progress_arg, int nloose, + got_cleanup_progress_cb progress_cb, void *progress_arg, + struct got_ratelimit *rl, int nloose, got_cancel_cb cancel_cb, void *cancel_arg) { const struct got_error *err; @@ -986,11 +1004,10 @@ load_commit_or_tag(struct got_object_idset *loose_ids, if (commit || tag) (*ncommits)++; /* scanned tags are counted as commits */ - if (progress_cb) { - err = progress_cb(progress_arg, nloose, *ncommits, -1); - if (err) - break; - } + err = report_cleanup_progress(progress_cb, progress_arg, rl, + nloose, *ncommits, -1); + if (err) + break; if (commit) { /* Find parent commits to scan. */ @@ -1024,6 +1041,7 @@ struct purge_loose_object_arg { struct got_repository *repo; got_cleanup_progress_cb progress_cb; void *progress_arg; + struct got_ratelimit *rl; int nloose; int ncommits; int npurged; @@ -1075,10 +1093,10 @@ purge_loose_object(struct got_object_id *id, void *dat a->npurged++; a->size_purged += sb.st_size; - if (a->progress_cb) { - err = a->progress_cb(a->progress_arg, a->nloose, - a->ncommits, a->npurged); - } + err = report_cleanup_progress(a->progress_cb, a->progress_arg, + a->rl, a->nloose, a->ncommits, a->npurged); + if (err) + goto done; } done: if (fd != -1 && close(fd) == -1 && err == NULL) @@ -1104,15 +1122,17 @@ got_repo_purge_unreferenced_loose_objects(struct got_r struct got_reflist_entry *re; struct purge_loose_object_arg arg; time_t max_mtime = 0; + struct got_ratelimit rl; TAILQ_INIT(&refs); + got_ratelimit_init(&rl, 0, 500); *size_before = 0; *size_after = 0; *npacked = 0; err = get_loose_object_ids(&loose_ids, size_before, - progress_cb, progress_arg, repo); + progress_cb, progress_arg, &rl, repo); if (err) return err; nloose = got_object_idset_num_elements(loose_ids); @@ -1153,23 +1173,17 @@ got_repo_purge_unreferenced_loose_objects(struct got_r for (i = 0; i < nreferenced; i++) { struct got_object_id *id = referenced_ids[i]; err = load_commit_or_tag(loose_ids, &ncommits, npacked, - traversed_ids, id, repo, progress_cb, progress_arg, nloose, - cancel_cb, cancel_arg); + traversed_ids, id, repo, progress_cb, progress_arg, &rl, + nloose, cancel_cb, cancel_arg); if (err) goto done; } - /* Produce a final progress report in case no objects can be purged. */ - if (got_object_idset_num_elements(loose_ids) == 0 && progress_cb) { - err = progress_cb(progress_arg, nloose, ncommits, 0); - if (err) - goto done; - } - /* Any remaining loose objects are unreferenced and can be purged. */ arg.repo = repo; arg.progress_arg = progress_arg; arg.progress_cb = progress_cb; + arg.rl = &rl; arg.nloose = nloose; arg.npurged = 0; arg.size_purged = 0; @@ -1181,6 +1195,14 @@ got_repo_purge_unreferenced_loose_objects(struct got_r if (err) goto done; *size_after = *size_before - arg.size_purged; + + /* Produce a final progress report. */ + if (progress_cb) { + err = progress_cb(progress_arg, nloose, ncommits, + got_object_idset_num_elements(loose_ids)); + if (err) + goto done; + } done: got_object_idset_free(loose_ids); got_object_idset_free(traversed_ids);

2022-01-04 20:38 Stefan Sperling:
rate-limiting of gotadmin progress output
- 2022-01-04 21:47 Tracey Emery:
  rate-limiting of gotadmin progress output