diff --git a/changelogs/unreleased/jv-git-patches.yml b/changelogs/unreleased/jv-git-patches.yml new file mode 100644 index 0000000000000000000000000000000000000000..ef4bf078d26f0b28c9c8ae7502d32918ba49f90e_Y2hhbmdlbG9ncy91bnJlbGVhc2VkL2p2LWdpdC1wYXRjaGVzLnltbA== --- /dev/null +++ b/changelogs/unreleased/jv-git-patches.yml @@ -0,0 +1,5 @@ +--- +title: Add Git refs performance patches +merge_request: 4977 +author: +type: performance diff --git a/config/patches/git/0001-builtin-pack-objects.c-avoid-iterating-all-refs.patch b/config/patches/git/0001-builtin-pack-objects.c-avoid-iterating-all-refs.patch new file mode 100644 index 0000000000000000000000000000000000000000..ef4bf078d26f0b28c9c8ae7502d32918ba49f90e_Y29uZmlnL3BhdGNoZXMvZ2l0LzAwMDEtYnVpbHRpbi1wYWNrLW9iamVjdHMuYy1hdm9pZC1pdGVyYXRpbmctYWxsLXJlZnMucGF0Y2g= --- /dev/null +++ b/config/patches/git/0001-builtin-pack-objects.c-avoid-iterating-all-refs.patch @@ -0,0 +1,60 @@ +From 14c49dd3d6f3bc2968d369181d5d3de002abe0c1 Mon Sep 17 00:00:00 2001 +From: Jacob Vosmaer <jacob@gitlab.com> +Date: Wed, 20 Jan 2021 13:45:14 +0100 +Subject: [PATCH 1/4] builtin/pack-objects.c: avoid iterating all refs + +In git-pack-objects, we iterate over all the tags if the --include-tag +option is passed on the command line. For some reason this uses +for_each_ref which is expensive if the repo has many refs. We should +use for_each_tag_ref instead. + +Because the add_ref_tag callback will now only visit tags we +simplified it a bit. + +The motivation for this change is that we observed performance issues +with a repository on gitlab.com that has 500,000 refs but only 2,000 +tags. The fetch traffic on that repo is dominated by CI, and when we +changed CI to fetch with 'git fetch --no-tags' we saw a dramatic +change in the CPU profile of git-pack-objects. This lead us to this +particular ref walk. More details in: +https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/746#note_483546598 + +Signed-off-by: Jacob Vosmaer <jacob@gitlab.com> +Reviewed-by: Taylor Blau <me@ttaylorr.com> +Signed-off-by: Junio C Hamano <gitster@pobox.com> +--- + builtin/pack-objects.c | 8 +++----- + 1 file changed, 3 insertions(+), 5 deletions(-) + +diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c +index 5617c01b5a..9a4f3561c1 100644 +--- a/builtin/pack-objects.c ++++ b/builtin/pack-objects.c +@@ -2804,13 +2804,11 @@ static void add_tag_chain(const struct object_id *oid) + } + } + +-static int add_ref_tag(const char *path, const struct object_id *oid, int flag, void *cb_data) ++static int add_ref_tag(const char *tag, const struct object_id *oid, int flag, void *cb_data) + { + struct object_id peeled; + +- if (starts_with(path, "refs/tags/") && /* is a tag? */ +- !peel_ref(path, &peeled) && /* peelable? */ +- obj_is_packed(&peeled)) /* object packed? */ ++ if (!peel_ref(tag, &peeled) && obj_is_packed(&peeled)) + add_tag_chain(oid); + return 0; + } +@@ -3741,7 +3739,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) + } + cleanup_preferred_base(); + if (include_tag && nr_result) +- for_each_ref(add_ref_tag, NULL); ++ for_each_tag_ref(add_ref_tag, NULL); + stop_progress(&progress_state); + trace2_region_leave("pack-objects", "enumerate-objects", + the_repository); +-- +2.30.0 + diff --git a/config/patches/git/0002-refs-expose-for_each_fullref_in_prefixes.patch b/config/patches/git/0002-refs-expose-for_each_fullref_in_prefixes.patch new file mode 100644 index 0000000000000000000000000000000000000000..ef4bf078d26f0b28c9c8ae7502d32918ba49f90e_Y29uZmlnL3BhdGNoZXMvZ2l0LzAwMDItcmVmcy1leHBvc2UtZm9yX2VhY2hfZnVsbHJlZl9pbl9wcmVmaXhlcy5wYXRjaA== --- /dev/null +++ b/config/patches/git/0002-refs-expose-for_each_fullref_in_prefixes.patch @@ -0,0 +1,247 @@ +From e561a23f1625f898208f10dbba8a69efd405360a Mon Sep 17 00:00:00 2001 +From: Taylor Blau <me@ttaylorr.com> +Date: Wed, 20 Jan 2021 11:04:21 -0500 +Subject: [PATCH 2/4] refs: expose 'for_each_fullref_in_prefixes' + +This function was used in the ref-filter.c code to find the longest +common prefix of among a set of refspecs, and then to iterate all of the +references that descend from that prefix. + +A future patch will want to use that same code from ls-refs.c, so +prepare by exposing and moving it to refs.c. Since there is nothing +specific to the ref-filter code here (other than that it was previously +the only caller of this function), this really belongs in the more +generic refs.h header. + +The code moved in this patch is identical before and after, with the one +exception of renaming some arguments to be consistent with other +functions exposed in refs.h. + +Signed-off-by: Taylor Blau <me@ttaylorr.com> +Signed-off-by: Junio C Hamano <gitster@pobox.com> +--- + ref-filter.c | 74 ++------------------------------------------ + refs.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++ + refs.h | 9 ++++++ + 3 files changed, 98 insertions(+), 72 deletions(-) + +diff --git a/ref-filter.c b/ref-filter.c +index c62f6b4822..dac7eab94a 100644 +--- a/ref-filter.c ++++ b/ref-filter.c +@@ -1921,64 +1921,6 @@ static int filter_pattern_match(struct ref_filter *filter, const char *refname) + return match_pattern(filter, refname); + } + +-static int qsort_strcmp(const void *va, const void *vb) +-{ +- const char *a = *(const char **)va; +- const char *b = *(const char **)vb; +- +- return strcmp(a, b); +-} +- +-static void find_longest_prefixes_1(struct string_list *out, +- struct strbuf *prefix, +- const char **patterns, size_t nr) +-{ +- size_t i; +- +- for (i = 0; i < nr; i++) { +- char c = patterns[i][prefix->len]; +- if (!c || is_glob_special(c)) { +- string_list_append(out, prefix->buf); +- return; +- } +- } +- +- i = 0; +- while (i < nr) { +- size_t end; +- +- /* +- * Set "end" to the index of the element _after_ the last one +- * in our group. +- */ +- for (end = i + 1; end < nr; end++) { +- if (patterns[i][prefix->len] != patterns[end][prefix->len]) +- break; +- } +- +- strbuf_addch(prefix, patterns[i][prefix->len]); +- find_longest_prefixes_1(out, prefix, patterns + i, end - i); +- strbuf_setlen(prefix, prefix->len - 1); +- +- i = end; +- } +-} +- +-static void find_longest_prefixes(struct string_list *out, +- const char **patterns) +-{ +- struct strvec sorted = STRVEC_INIT; +- struct strbuf prefix = STRBUF_INIT; +- +- strvec_pushv(&sorted, patterns); +- QSORT(sorted.v, sorted.nr, qsort_strcmp); +- +- find_longest_prefixes_1(out, &prefix, sorted.v, sorted.nr); +- +- strvec_clear(&sorted); +- strbuf_release(&prefix); +-} +- + /* + * This is the same as for_each_fullref_in(), but it tries to iterate + * only over the patterns we'll care about. Note that it _doesn't_ do a full +@@ -1989,10 +1931,6 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter, + void *cb_data, + int broken) + { +- struct string_list prefixes = STRING_LIST_INIT_DUP; +- struct string_list_item *prefix; +- int ret; +- + if (!filter->match_as_path) { + /* + * in this case, the patterns are applied after +@@ -2016,16 +1954,8 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter, + return for_each_fullref_in("", cb, cb_data, broken); + } + +- find_longest_prefixes(&prefixes, filter->name_patterns); +- +- for_each_string_list_item(prefix, &prefixes) { +- ret = for_each_fullref_in(prefix->string, cb, cb_data, broken); +- if (ret) +- break; +- } +- +- string_list_clear(&prefixes, 0); +- return ret; ++ return for_each_fullref_in_prefixes(NULL, filter->name_patterns, ++ cb, cb_data, broken); + } + + /* +diff --git a/refs.c b/refs.c +index fa01153151..c5509eeebf 100644 +--- a/refs.c ++++ b/refs.c +@@ -1527,6 +1527,93 @@ int for_each_rawref(each_ref_fn fn, void *cb_data) + return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data); + } + ++static int qsort_strcmp(const void *va, const void *vb) ++{ ++ const char *a = *(const char **)va; ++ const char *b = *(const char **)vb; ++ ++ return strcmp(a, b); ++} ++ ++static void find_longest_prefixes_1(struct string_list *out, ++ struct strbuf *prefix, ++ const char **patterns, size_t nr) ++{ ++ size_t i; ++ ++ for (i = 0; i < nr; i++) { ++ char c = patterns[i][prefix->len]; ++ if (!c || is_glob_special(c)) { ++ string_list_append(out, prefix->buf); ++ return; ++ } ++ } ++ ++ i = 0; ++ while (i < nr) { ++ size_t end; ++ ++ /* ++ * Set "end" to the index of the element _after_ the last one ++ * in our group. ++ */ ++ for (end = i + 1; end < nr; end++) { ++ if (patterns[i][prefix->len] != patterns[end][prefix->len]) ++ break; ++ } ++ ++ strbuf_addch(prefix, patterns[i][prefix->len]); ++ find_longest_prefixes_1(out, prefix, patterns + i, end - i); ++ strbuf_setlen(prefix, prefix->len - 1); ++ ++ i = end; ++ } ++} ++ ++static void find_longest_prefixes(struct string_list *out, ++ const char **patterns) ++{ ++ struct strvec sorted = STRVEC_INIT; ++ struct strbuf prefix = STRBUF_INIT; ++ ++ strvec_pushv(&sorted, patterns); ++ QSORT(sorted.v, sorted.nr, qsort_strcmp); ++ ++ find_longest_prefixes_1(out, &prefix, sorted.v, sorted.nr); ++ ++ strvec_clear(&sorted); ++ strbuf_release(&prefix); ++} ++ ++int for_each_fullref_in_prefixes(const char *namespace, ++ const char **patterns, ++ each_ref_fn fn, void *cb_data, ++ unsigned int broken) ++{ ++ struct string_list prefixes = STRING_LIST_INIT_DUP; ++ struct string_list_item *prefix; ++ struct strbuf buf = STRBUF_INIT; ++ int ret = 0, namespace_len; ++ ++ find_longest_prefixes(&prefixes, patterns); ++ ++ if (namespace) ++ strbuf_addstr(&buf, namespace); ++ namespace_len = buf.len; ++ ++ for_each_string_list_item(prefix, &prefixes) { ++ strbuf_addstr(&buf, prefix->string); ++ ret = for_each_fullref_in(buf.buf, fn, cb_data, broken); ++ if (ret) ++ break; ++ strbuf_setlen(&buf, namespace_len); ++ } ++ ++ string_list_clear(&prefixes, 0); ++ strbuf_release(&buf); ++ return ret; ++} ++ + static int refs_read_special_head(struct ref_store *ref_store, + const char *refname, struct object_id *oid, + struct strbuf *referent, unsigned int *type) +diff --git a/refs.h b/refs.h +index 6695518156..8e84f14211 100644 +--- a/refs.h ++++ b/refs.h +@@ -347,6 +347,15 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix, + int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, + unsigned int broken); + ++/** ++ * iterate all refs in "patterns" by partitioning patterns into disjoint sets ++ * and iterating the longest-common prefix of each set. ++ * ++ * callers should be prepared to ignore references that they did not ask for. ++ */ ++int for_each_fullref_in_prefixes(const char *namespace, const char **patterns, ++ each_ref_fn fn, void *cb_data, ++ unsigned int broken); + /** + * iterate refs from the respective area. + */ +-- +2.30.0 + diff --git a/config/patches/git/0003-ls-refs.c-initialize-prefixes-before-using-it.patch b/config/patches/git/0003-ls-refs.c-initialize-prefixes-before-using-it.patch new file mode 100644 index 0000000000000000000000000000000000000000..ef4bf078d26f0b28c9c8ae7502d32918ba49f90e_Y29uZmlnL3BhdGNoZXMvZ2l0LzAwMDMtbHMtcmVmcy5jLWluaXRpYWxpemUtcHJlZml4ZXMtYmVmb3JlLXVzaW5nLWl0LnBhdGNo --- /dev/null +++ b/config/patches/git/0003-ls-refs.c-initialize-prefixes-before-using-it.patch @@ -0,0 +1,43 @@ +From 5dd7555a496cdcffa5834429f93f17c66e5b914a Mon Sep 17 00:00:00 2001 +From: Jacob Vosmaer <jacob@gitlab.com> +Date: Wed, 20 Jan 2021 11:04:25 -0500 +Subject: [PATCH 3/4] ls-refs.c: initialize 'prefixes' before using it + +Correctly initialize the "prefixes" strvec using strvec_init() instead +of simply zeroing it via the earlier memset(). + +There's no way to trigger a crash, since the first 'ref-prefix' command +will initialize the strvec via the 'ALLOC_GROW' in 'strvec_push_nodup()' +(the alloc and nr variables are already zero'd, so the call to +ALLOC_GROW is valid). + +If no "ref-prefix" command was given, then the call to +'ls-refs.c:ref_match()' will abort early after it reads the zero in +'prefixes->nr'. Likewise, strvec_clear() will only call free() on the +array, which is NULL, so we're safe there, too. + +But, all of this is dangerous and requires more reasoning than it would +if we simply called 'strvec_init()', so do that. + +Signed-off-by: Jacob Vosmaer <jacob@gitlab.com> +Signed-off-by: Taylor Blau <me@ttaylorr.com> +Signed-off-by: Junio C Hamano <gitster@pobox.com> +--- + ls-refs.c | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/ls-refs.c b/ls-refs.c +index a1e0b473e4..367597d447 100644 +--- a/ls-refs.c ++++ b/ls-refs.c +@@ -90,6 +90,7 @@ int ls_refs(struct repository *r, struct strvec *keys, + struct ls_refs_data data; + + memset(&data, 0, sizeof(data)); ++ strvec_init(&data.prefixes); + + git_config(ls_refs_config, NULL); + +-- +2.30.0 + diff --git a/config/patches/git/0004-ls-refs.c-traverse-prefixes-of-disjoint-ref-prefix-s.patch b/config/patches/git/0004-ls-refs.c-traverse-prefixes-of-disjoint-ref-prefix-s.patch new file mode 100644 index 0000000000000000000000000000000000000000..ef4bf078d26f0b28c9c8ae7502d32918ba49f90e_Y29uZmlnL3BhdGNoZXMvZ2l0LzAwMDQtbHMtcmVmcy5jLXRyYXZlcnNlLXByZWZpeGVzLW9mLWRpc2pvaW50LXJlZi1wcmVmaXgtcy5wYXRjaA== --- /dev/null +++ b/config/patches/git/0004-ls-refs.c-traverse-prefixes-of-disjoint-ref-prefix-s.patch @@ -0,0 +1,60 @@ +From 7fdc5adb77c9eca4a71727891678a4409208fb47 Mon Sep 17 00:00:00 2001 +From: Taylor Blau <me@ttaylorr.com> +Date: Wed, 20 Jan 2021 11:04:30 -0500 +Subject: [PATCH 4/4] ls-refs.c: traverse prefixes of disjoint "ref-prefix" + sets + +ls-refs performs a single revision walk over the whole ref namespace, +and sends ones that match with one of the given ref prefixes down to the +user. + +This can be expensive if there are many refs overall, but the portion of +them covered by the given prefixes is small by comparison. + +To attempt to reduce the difference between the number of refs +traversed, and the number of refs sent, only traverse references which +are in the longest common prefix of the given prefixes. This is very +reminiscent of the approach taken in b31e2680c4 (ref-filter.c: find +disjoint pattern prefixes, 2019-06-26) which does an analogous thing for +multi-patterned 'git for-each-ref' invocations. + +The callback 'send_ref' is resilient to ignore extra patterns by +discarding any arguments which do not begin with at least one of the +specified prefixes. + +Similarly, the code introduced in b31e2680c4 is resilient to stop early +at metacharacters, but we only pass strict prefixes here. At worst we +would return too many results, but the double checking done by send_ref +will throw away anything that doesn't start with something in the prefix +list. + +Finally, if no prefixes were provided, then implicitly add the empty +string (which will match all references) since this matches the existing +behavior (see the "no restrictions" comment in "ls-refs.c:ref_match()"). + +Original-patch-by: Jacob Vosmaer <jacob@gitlab.com> +Signed-off-by: Taylor Blau <me@ttaylorr.com> +Signed-off-by: Junio C Hamano <gitster@pobox.com> +--- + ls-refs.c | 5 ++++- + 1 file changed, 4 insertions(+), 1 deletion(-) + +diff --git a/ls-refs.c b/ls-refs.c +index 367597d447..eaaa36d0df 100644 +--- a/ls-refs.c ++++ b/ls-refs.c +@@ -110,7 +110,10 @@ int ls_refs(struct repository *r, struct strvec *keys, + die(_("expected flush after ls-refs arguments")); + + head_ref_namespaced(send_ref, &data); +- for_each_namespaced_ref(send_ref, &data); ++ if (!data.prefixes.nr) ++ strvec_push(&data.prefixes, ""); ++ for_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v, ++ send_ref, &data, 0); + packet_flush(1); + strvec_clear(&data.prefixes); + return 0; +-- +2.30.0 + diff --git a/config/software/git.rb b/config/software/git.rb index 04ed1248286ead3d7dc8c69a6087057194c411a5_Y29uZmlnL3NvZnR3YXJlL2dpdC5yYg==..ef4bf078d26f0b28c9c8ae7502d32918ba49f90e_Y29uZmlnL3NvZnR3YXJlL2dpdC5yYg== 100644 --- a/config/software/git.rb +++ b/config/software/git.rb @@ -59,6 +59,14 @@ # # Patch files should be in config/patches/git/ + # Fast-track patches from + # https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/806, which + # are expected to land in 2.31.0. + patch source: '0001-builtin-pack-objects.c-avoid-iterating-all-refs.patch' + patch source: '0002-refs-expose-for_each_fullref_in_prefixes.patch' + patch source: '0003-ls-refs.c-initialize-prefixes-before-using-it.patch' + patch source: '0004-ls-refs.c-traverse-prefixes-of-disjoint-ref-prefix-s.patch' + block do File.open(File.join(project_dir, 'config.mak'), 'a') do |file| file.print <<-EOH