diffcore-rename: accelerate rename_dst setup

register_rename_src() simply references the passed pair inside rename_src. In contrast, add_rename_dst() did something entirely different for rename_dst. Instead of copying the passed pair, it made a copy of the second diff_filespec from the passed pair, referenced it, and then set the diff_rename_dst.pair field to NULL. Later, when a pairing is found, record_rename_pair() allocated a full diff_filepair via diff_queue() and pointed its src and dst fields at the appropriate diff_filespecs. This contrast between register_rename_src() for the rename_src data structure and add_rename_dst() for the rename_dst data structure is oddly inconsistent and requires more memory and work than necessary. Let's just reference the original diff_filepair in rename_dst as-is, just as we do with rename_src. Add a new rename_dst.is_rename field, since the rename_dst.p field is never NULL unlike the old rename_dst.pair field. Taking advantage of this change and the fact that same-named paths will be adjacent, we can get rid of the sorting of the array and most of the lookups on it, allowing us to instead just append as we go. However, there is one remaining reason to still keep locate_rename_dst(): handling broken pairs (i.e. when break detection is on). Those are somewhat rare, but we can set up a simple strintmap to get the map between the source and the index. Doing that allows us to still have a fast lookup without sorting the rename_dst array. Since the sorting had been done in a weakly quadratic manner, when many renames are involved this time could add up. There is still a strcmp() in add_rename_dst() that I have left in place to make it easier to verify that the algorithm has the same results. This strcmp() is there to check for duplicate destination entries (which was the easiest way at the time to avoid segfaults in the diffcore-rename code when trees had multiple entries at a given path). The underlying double free()s are no longer an issue with the new algorithm, but that can be addressed in a subsequent commit. This patch is being submitted in a different order than its original development, but in a large rebase of many commits with lots of renames and with several optimizations to inexact rename detection, both setup time and write back to output queue time from diffcore_rename() were sizeable chunks of overall runtime. This patch accelerated the setup time by about 65%, and final write back to the output queue time by about 50%, resulting in an overall drop of 3.5% on the execution time of rebasing a few dozen patches. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Dec 11, 2020 at 09:08 UTC 9db2ac56168e58f856b001a93ad369affe18879f
1 file changed +65 -83
diffcore-rename.c
+65 -83
@@ -9,63 +9,48 @@
9 #include "hashmap.h"
10 #include "progress.h"
11 #include "promisor-remote.h"
12 +#include "strmap.h"
13
14 /* Table of rename/copy destinations */
15
16 static struct diff_rename_dst {
16 - struct diff_filespec *two;
17 - struct diff_filepair *pair;
17 + struct diff_filepair *p;
18 + struct diff_filespec *filespec_to_free;
19 + int is_rename; /* false -> just a create; true -> rename or copy */
20 } *rename_dst;
21 static int rename_dst_nr, rename_dst_alloc;
22 +/* Mapping from break source pathname to break destination index */
23 +static struct strintmap *break_idx = NULL;
24
21 -static int find_rename_dst(struct diff_filespec *two)
22 -{
23 - int first, last;
24 -
25 - first = 0;
26 - last = rename_dst_nr;
27 - while (last > first) {
28 - int next = first + ((last - first) >> 1);
29 - struct diff_rename_dst *dst = &(rename_dst[next]);
30 - int cmp = strcmp(two->path, dst->two->path);
31 - if (!cmp)
32 - return next;
33 - if (cmp < 0) {
34 - last = next;
35 - continue;
36 - }
37 - first = next+1;
38 - }
39 - return -first - 1;
40 -}
41 -
42 -static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two)
25 +static struct diff_rename_dst *locate_rename_dst(struct diff_filepair *p)
26 {
44 - int ofs = find_rename_dst(two);
45 - return ofs < 0 ? NULL : &rename_dst[ofs];
27 + /* Lookup by p->ONE->path */
28 + int idx = break_idx ? strintmap_get(break_idx, p->one->path) : -1;
29 + return (idx == -1) ? NULL : &rename_dst[idx];
30 }
31
32 /*
33 * Returns 0 on success, -1 if we found a duplicate.
34 */
51 -static int add_rename_dst(struct diff_filespec *two)
35 +static int add_rename_dst(struct diff_filepair *p)
36 {
53 - int first = find_rename_dst(two);
54 -
55 - if (first >= 0)
37 + /*
38 + * If we have multiple entries at the same path in the destination
39 + * tree (an invalid tree, to be sure), turn off rename detection
40 + * entirely. Once upon a time, diffcore-rename had double free()
41 + * issues due to such duplicate paths, resulting in segfaults. See
42 + * commit 4d6be03b95 ("diffcore-rename: avoid processing duplicate
43 + * destinations", 2015-02-26).
44 + */
45 + if (rename_dst_nr > 0 &&
46 + !strcmp(rename_dst[rename_dst_nr-1].p->two->path, p->two->path))
47 return -1;
57 - first = -first - 1;
48
59 - /* insert to make it at "first" */
49 ALLOC_GROW(rename_dst, rename_dst_nr + 1, rename_dst_alloc);
50 + rename_dst[rename_dst_nr].p = p;
51 + rename_dst[rename_dst_nr].filespec_to_free = NULL;
52 + rename_dst[rename_dst_nr].is_rename = 0;
53 rename_dst_nr++;
62 - if (first < rename_dst_nr)
63 - MOVE_ARRAY(rename_dst + first + 1, rename_dst + first,
64 - rename_dst_nr - first - 1);
65 - rename_dst[first].two = alloc_filespec(two->path);
66 - fill_filespec(rename_dst[first].two, &two->oid, two->oid_valid,
67 - two->mode);
68 - rename_dst[first].pair = NULL;
54 return 0;
55 }
56
@@ -89,6 +74,14 @@ static void register_rename_src(struct diff_filepair *p)
74 !strcmp(rename_src[rename_src_nr-1].p->one->path, p->one->path))
75 return;
76
77 + if (p->broken_pair) {
78 + if (!break_idx) {
79 + break_idx = xmalloc(sizeof(*break_idx));
80 + strintmap_init(break_idx, -1);
81 + }
82 + strintmap_set(break_idx, p->one->path, rename_dst_nr);
83 + }
84 +
85 ALLOC_GROW(rename_src, rename_src_nr + 1, rename_src_alloc);
86 rename_src[rename_src_nr].p = p;
87 rename_src[rename_src_nr].score = p->score;
@@ -128,14 +121,14 @@ static void prefetch(void *prefetch_options)
121 struct oid_array to_fetch = OID_ARRAY_INIT;
122
123 for (i = 0; i < rename_dst_nr; i++) {
131 - if (rename_dst[i].pair)
124 + if (rename_dst[i].p->renamed_pair)
125 /*
126 * The loop in diffcore_rename() will not need these
127 * blobs, so skip prefetching.
128 */
129 continue; /* already found exact match */
130 diff_add_if_missing(options->repo, &to_fetch,
138 - rename_dst[i].two);
131 + rename_dst[i].p->two);
132 }
133 for (i = 0; i < rename_src_nr; i++) {
134 if (options->skip_unmodified &&
@@ -245,26 +238,24 @@ static int estimate_similarity(struct repository *r,
238
239 static void record_rename_pair(int dst_index, int src_index, int score)
240 {
248 - struct diff_filespec *src, *dst;
249 - struct diff_filepair *dp;
241 + struct diff_filepair *src = rename_src[src_index].p;
242 + struct diff_filepair *dst = rename_dst[dst_index].p;
243
251 - if (rename_dst[dst_index].pair)
244 + if (dst->renamed_pair)
245 die("internal error: dst already matched.");
246
254 - src = rename_src[src_index].p->one;
255 - src->rename_used++;
256 - src->count++;
247 + src->one->rename_used++;
248 + src->one->count++;
249
258 - dst = rename_dst[dst_index].two;
259 - dst->count++;
250 + rename_dst[dst_index].filespec_to_free = dst->one;
251 + rename_dst[dst_index].is_rename = 1;
252
261 - dp = diff_queue(NULL, src, dst);
262 - dp->renamed_pair = 1;
263 - if (!strcmp(src->path, dst->path))
264 - dp->score = rename_src[src_index].score;
253 + dst->one = src->one;
254 + dst->renamed_pair = 1;
255 + if (!strcmp(dst->one->path, dst->two->path))
256 + dst->score = rename_src[src_index].score;
257 else
266 - dp->score = score;
267 - rename_dst[dst_index].pair = dp;
258 + dst->score = score;
259 }
260
261 /*
@@ -310,7 +301,7 @@ static int find_identical_files(struct hashmap *srcs,
301 struct diff_options *options)
302 {
303 int renames = 0;
313 - struct diff_filespec *target = rename_dst[dst_index].two;
304 + struct diff_filespec *target = rename_dst[dst_index].p->two;
305 struct file_similarity *p, *best = NULL;
306 int i = 100, best_score = -1;
307 unsigned int hash = hash_filespec(options->repo, target);
@@ -476,7 +467,7 @@ static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, i
467 (mx[i].score < minimum_score))
468 break; /* there is no more usable pair. */
469 dst = &rename_dst[mx[i].dst];
479 - if (dst->pair)
470 + if (dst->is_rename)
471 continue; /* already done, either exact or fuzzy. */
472 if (!copies && rename_src[mx[i].src].p->one->rename_used)
473 continue;
@@ -511,7 +502,7 @@ void diffcore_rename(struct diff_options *options)
502 else if (!options->flags.rename_empty &&
503 is_empty_blob_oid(&p->two->oid))
504 continue;
514 - else if (add_rename_dst(p->two) < 0) {
505 + else if (add_rename_dst(p) < 0) {
506 warning("skipping rename detection, detected"
507 " duplicate destination '%s'",
508 p->two->path);
@@ -586,10 +577,10 @@ void diffcore_rename(struct diff_options *options)
577 mx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_destinations),
578 sizeof(*mx));
579 for (dst_cnt = i = 0; i < rename_dst_nr; i++) {
589 - struct diff_filespec *two = rename_dst[i].two;
580 + struct diff_filespec *two = rename_dst[i].p->two;
581 struct diff_score *m;
582
592 - if (rename_dst[i].pair)
583 + if (rename_dst[i].is_rename)
584 continue; /* dealt with exact match already. */
585
586 m = &mx[dst_cnt * NUM_CANDIDATE_PER_DST];
@@ -646,22 +637,8 @@ void diffcore_rename(struct diff_options *options)
637 diff_q(&outq, p);
638 }
639 else if (!DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {
649 - /*
650 - * Creation
651 - *
652 - * We would output this create record if it has
653 - * not been turned into a rename/copy already.
654 - */
655 - struct diff_rename_dst *dst = locate_rename_dst(p->two);
656 - if (dst && dst->pair) {
657 - diff_q(&outq, dst->pair);
658 - pair_to_free = p;
659 - }
660 - else
661 - /* no matching rename/copy source, so
662 - * record this as a creation.
663 - */
664 - diff_q(&outq, p);
640 + /* Creation */
641 + diff_q(&outq, p);
642 }
643 else if (DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two)) {
644 /*
@@ -682,8 +659,10 @@ void diffcore_rename(struct diff_options *options)
659 */
660 if (DIFF_PAIR_BROKEN(p)) {
661 /* broken delete */
685 - struct diff_rename_dst *dst = locate_rename_dst(p->one);
686 - if (dst && dst->pair)
662 + struct diff_rename_dst *dst = locate_rename_dst(p);
663 + if (!dst)
664 + BUG("tracking failed somehow; failed to find associated dst for broken pair");
665 + if (dst->is_rename)
666 /* counterpart is now rename/copy */
667 pair_to_free = p;
668 }
@@ -693,16 +672,14 @@ void diffcore_rename(struct diff_options *options)
672 pair_to_free = p;
673 }
674
696 - if (pair_to_free)
697 - ;
698 - else
675 + if (!pair_to_free)
676 diff_q(&outq, p);
677 }
678 else if (!diff_unmodified_pair(p))
679 /* all the usual ones need to be kept */
680 diff_q(&outq, p);
681 else
705 - /* no need to keep unmodified pairs */
682 + /* no need to keep unmodified pairs; FIXME: remove earlier? */
683 pair_to_free = p;
684
685 if (pair_to_free)
@@ -715,11 +692,16 @@ void diffcore_rename(struct diff_options *options)
692 diff_debug_queue("done collapsing", q);
693
694 for (i = 0; i < rename_dst_nr; i++)
718 - free_filespec(rename_dst[i].two);
695 + if (rename_dst[i].filespec_to_free)
696 + free_filespec(rename_dst[i].filespec_to_free);
697
698 FREE_AND_NULL(rename_dst);
699 rename_dst_nr = rename_dst_alloc = 0;
700 FREE_AND_NULL(rename_src);
701 rename_src_nr = rename_src_alloc = 0;
702 + if (break_idx) {
703 + strintmap_clear(break_idx);
704 + FREE_AND_NULL(break_idx);
705 + }
706 return;
707 }