From d0e52c1728bdab27f7ca61ee0d4ff91055646bae Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 6 Nov 2024 16:16:58 -0500 Subject: [PATCH 1/4] t6120: demonstrate weakness in disjoint-root handling Commit 30b1c7ad9d (describe: don't abort too early when searching tags, 2020-02-26) tried to fix a problem that happens when there are disjoint histories: to accurately compare the counts for different tags, we need to keep walking the history longer in order to find a common base. But its fix misses a case: we may still bail early if we hit the max_candidates limit, producing suboptimal output. You can see this in action by adding "--candidates=2" to the tests; we'll stop traversing as soon as we see the second tag and will produce the wrong answer. I hit this in practice while trying to teach git-describe not to keep looking for candidates after we've seen all tags in the repo (effectively adding --candidates=2, since these toy repos have only two tags each). This is probably fixable by continuing to walk after hitting the max-candidates limit, all the way down to a common ancestor of all candidates. But it's not clear in practice what the preformance implications would be (it would depend on how long the branches that hold the candidates are). So I'm punting on that for now, but I'd like to adjust the tests to be more resilient, and to document the findings. So this patch: 1. Adds an extra tag at the bottom of history. This shouldn't change the output, but does mean we are more resilient to low values of --candidates (e.g., if we start reducing it to the total number of tags). This is arguably closer to the real world anyway, where you're not going to have just 2 tags, but an arbitrarily long history going back in time, possibly with multiple irrelevant tags in it (I called the new tag "H" here for "history"). 2. Run the same tests with --candidates=2, which shows that even with the current code they can fail if we end the traversal early. That leaves a trail for anybody interested in trying to improve the behavior. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t6120-describe.sh | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 05ed2510d96868..69689d2f3634b8 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -19,6 +19,7 @@ TEST_PASSES_SANITIZE_LEAK=true check_describe () { indir= && + outcome=success && while test $# != 0 do case "$1" in @@ -26,6 +27,9 @@ check_describe () { indir="$2" shift ;; + --expect-failure) + outcome=failure + ;; *) break ;; @@ -36,7 +40,7 @@ check_describe () { expect="$1" shift describe_opts="$@" - test_expect_success "describe $describe_opts" ' + test_expect_${outcome} "describe $describe_opts" ' git ${indir:+ -C "$indir"} describe $describe_opts >raw && sed -e "s/-g[0-9a-f]*\$/-gHASH/" actual && echo "$expect" >expect && @@ -617,7 +621,7 @@ test_expect_success 'name-rev --annotate-stdin works with commitGraph' ' # B # o -# \ +# H \ # o-----o---o----x # A # @@ -627,6 +631,7 @@ test_expect_success 'setup: describe commits with disjoint bases' ' cd disjoint1 && echo o >> file && git add file && git commit -m o && + git tag H -a -m H && echo A >> file && git add file && git commit -m A && git tag A -a -m A && echo o >> file && git add file && git commit -m o && @@ -639,8 +644,9 @@ test_expect_success 'setup: describe commits with disjoint bases' ' ' check_describe -C disjoint1 "A-3-gHASH" HEAD +check_describe -C disjoint1 --expect-failure "A-3-gHASH" --candidates=2 HEAD -# B +# H B # o---o---o------------. # \ # o---o---x @@ -658,6 +664,7 @@ test_expect_success 'setup: describe commits with disjoint bases 2' ' git checkout --orphan branch && echo o >> file2 && git add file2 && GIT_COMMITTER_DATE="2020-01-01 15:00" git commit -m o && echo o >> file2 && git add file2 && GIT_COMMITTER_DATE="2020-01-01 15:01" git commit -m o && + git tag H -a -m H && echo B >> file2 && git add file2 && GIT_COMMITTER_DATE="2020-01-01 15:02" git commit -m B && git tag B -a -m B && git merge --no-ff --allow-unrelated-histories main -m x @@ -665,6 +672,7 @@ test_expect_success 'setup: describe commits with disjoint bases 2' ' ' check_describe -C disjoint2 "B-3-gHASH" HEAD +check_describe -C disjoint2 --expect-failure "B-3-gHASH" --candidates=2 HEAD test_expect_success 'setup misleading taggerdates' ' GIT_COMMITTER_DATE="2006-12-12 12:31" git tag -a -m "another tag" newer-tag-older-commit unique-file~1 From bb0830c6820bf25cdf4722c63a3ff06470e18b0e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 6 Nov 2024 16:17:08 -0500 Subject: [PATCH 2/4] t/perf: add tests for git-describe We don't have a perf script for git-describe, despite it often being accused of slowness. Let's add a few simple tests to start with. Rather than use the existing tags from our test repo, we'll make our own so that we have a known quantity and position. We'll add a "new" tag near the tip of HEAD, and an "old" one that is at the very bottom. And then our tests are: 1. Describing HEAD naively requires walking all the way down to the old tag as we collect candidates. This gives us a baseline for what "slow" looks like. 2. Doing the same with --candidates=1 can potentially be fast, because we can quie after finding "new". But we don't, and it's also slow. 3. Likewise we should be able to quit when there are no more tags to find. This can happen naturally if a repo has few tags, but also if you restrict the set of tags with --match. Here are the results running against linux.git. Note that I have a commit-graph built for the repo, so "slow" here is ~700ms. Without a commit graph it's more like 9s! Test HEAD -------------------------------------------------------------- 6100.2: describe HEAD 0.70(0.66+0.04) 6100.3: describe HEAD with one max candidate 0.70(0.66+0.04) 6100.4: describe HEAD with one tag 0.70(0.64+0.06) Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/perf/p6100-describe.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100755 t/perf/p6100-describe.sh diff --git a/t/perf/p6100-describe.sh b/t/perf/p6100-describe.sh new file mode 100755 index 00000000000000..069f91ce493a70 --- /dev/null +++ b/t/perf/p6100-describe.sh @@ -0,0 +1,30 @@ +#!/bin/sh + +test_description='performance of git-describe' +. ./perf-lib.sh + +test_perf_default_repo + +# clear out old tags and give us a known state +test_expect_success 'set up tags' ' + git for-each-ref --format="delete %(refname)" refs/tags >to-delete && + git update-ref --stdin Date: Wed, 6 Nov 2024 16:17:14 -0500 Subject: [PATCH 3/4] describe: stop digging for max_candidates+1 By default, describe considers only 10 candidate matches, and stops traversing when we have enough. This makes things much faster in a large repository, where collecting all candidates requires walking all the way down to the root (or at least to the oldest tag). This goes all the way back to 8713ab3079 (Improve git-describe performance by reducing revision listing., 2007-01-13). However, we don't stop immediately when we have enough candidates. We keep traversing and only bail when we find one more candidate that we're ignoring. Usually this is not too expensive, if the tags are sprinkled evenly throughout history. But if you are unlucky, you might hit the max candidate quickly, and then have a huge swath of history before finding the next one. Our p6100 test has exactly this unlucky case: with a max of "1", we find a recent tag quickly and then have to go all the way to the root to find the old tag that will be discarded. A more interesting real-world case is: git describe --candidates=1 --match=v6.12-rc4 HEAD in the linux.git repo. There we restrict the set of tags to a single one, so there is no older candidate to find at all! But despite --candidates=1, we keep traversing to the root only to find nothing. So why do we keep traversing after hitting thet max? There are two reasons I can see: 1. In theory the extra information that there was another candidate could be useful, and we record it in the gave_up_on variable. But we only show this information with --debug. 2. After finding the candidate, there's more processing we do in our loop. The most important of this is propagating the "within" flags to our parent commits, and putting them in the commit_list we'll use for finish_depth_computation(). That function continues the traversal until we've counted all commits reachable from the starting point but not reachable from our best candidate tag (so essentially counting "$tag..$start", but avoiding re-walking over the bits we've seen). If we break immediately without putting those commits into the list, our depth computation will be wrong (in the worst case we'll count all the way down to the root, not realizing those commits are included in our tag). But we don't need to find a new candidate for (2). As soon as we finish the loop iteration where we hit max_candidates, we can then quit on the next iteration. This should produce the same output as the original code (which could, after all, find a candidate on the very next commit anyway) but ends the traversal with less pointless digging. We still have to set "gave_up_on"; we've popped it off the list and it has to go back. An alternative would be to re-order the loop so that it never gets popped, but it's perhaps still useful to show in the --debug output, so we need to know it anyway. We do have to adjust the --debug output since it's now just a commit where we stopped traversing, and not the max+1th candidate. p6100 shows the speedup using linux.git: Test HEAD^ HEAD --------------------------------------------------------------------------------------- 6100.2: describe HEAD 0.70(0.63+0.06) 0.71(0.66+0.04) +1.4% 6100.3: describe HEAD with one max candidate 0.70(0.64+0.05) 0.01(0.00+0.00) -98.6% 6100.4: describe HEAD with one tag 0.70(0.67+0.03) 0.70(0.63+0.06) +0.0% Reported-by: Josh Poimboeuf Helped-by: Rasmus Villemoes Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/describe.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/builtin/describe.c b/builtin/describe.c index 7330a77b38ea7e..69f2d942be8203 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -366,6 +366,12 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) struct commit_name **slot; seen_commits++; + + if (match_cnt == max_candidates) { + gave_up_on = c; + break; + } + slot = commit_names_peek(&commit_names, c); n = slot ? *slot : NULL; if (n) { @@ -381,10 +387,6 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) if (n->prio == 2) annotated_cnt++; } - else { - gave_up_on = c; - break; - } } for (cur_match = 0; cur_match < match_cnt; cur_match++) { struct possible_tag *t = &all_matches[cur_match]; @@ -470,9 +472,8 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) fprintf(stderr, _("traversed %lu commits\n"), seen_commits); if (gave_up_on) { fprintf(stderr, - _("more than %i tags found; listed %i most recent\n" - "gave up search at %s\n"), - max_candidates, max_candidates, + _("found %i tags; gave up search at %s\n"), + max_candidates, oid_to_hex(&gave_up_on->object.oid)); } } From b8150bfee1f8d609e3d1f38eaa98cfb64a1c2ca5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 6 Nov 2024 16:17:17 -0500 Subject: [PATCH 4/4] describe: stop traversing when we run out of names When trying to describe a commit, we'll traverse from the commit, collecting candidate tags that point to its ancestors. But once we've seen all of the tags in the repo, there's no point in traversing further. There's nothing left to find! For a default "git describe", this isn't usually a big problem. In a large repo you'll probably have multiple tags, so we'll eventually find 10 candidates (the default for max_candidates) and stop there. And in a small repo, it's quick to traverse to the root. But you can imagine a large repo with few tags. Or, as we saw in a real world case, explicitly limiting the set of matches like this (on linux.git): git describe --match=v6.12-rc4 HEAD which goes all the way to the root before realizing that no, there are no other tags under consideration besides the one we fed via --match. If we add in "--candidates=1" there, it's much faster (at least as of the previous commit). But we should be able to speed this up without the user asking for it. After expanding all matching tags, we know the total number of names. We could just stop the traversal there, but as hinted at above we already have a mechanism for doing that: the max_candidate limit. So we can just reduce that limit to match the number of possible candidates. Our p6100 test shows this off: Test HEAD^ HEAD --------------------------------------------------------------------------------------- 6100.2: describe HEAD 0.71(0.65+0.06) 0.72(0.68+0.04) +1.4% 6100.3: describe HEAD with one max candidate 0.01(0.00+0.00) 0.01(0.00+0.00) +0.0% 6100.4: describe HEAD with one tag 0.72(0.66+0.05) 0.01(0.00+0.00) -98.6% Now we are fast automatically, just as if --candidates=1 were supplied by the user. Reported-by: Josh Poimboeuf Helped-by: Rasmus Villemoes Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/describe.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/describe.c b/builtin/describe.c index 69f2d942be8203..8ec3be87df0a2a 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -667,6 +667,8 @@ int cmd_describe(int argc, NULL); if (!hashmap_get_size(&names) && !always) die(_("No names found, cannot describe anything.")); + if (hashmap_get_size(&names) < max_candidates) + max_candidates = hashmap_get_size(&names); if (argc == 0) { if (broken) {