@matheusbsbs / matheus-bagliano / commits / f78cf97617

merge-ort: call diffcore_rename() directly

We want to pass additional information to diffcore_rename() (or some variant thereof) without plumbing that extra information through diff_tree_oid() and diffcore_std(). Further, since we will need to gather additional special information related to diffs and are walking the trees anyway in collect_merge_info(), it seems odd to have diff_tree_oid()/diffcore_std() repeat those tree walks. And there may be times where we can avoid traversing into a subtree in collect_merge_info() (based on additional information at our disposal), that the basic diff logic would be unable to take advantage of. For all these reasons, just create the add and delete pairs ourself and then call diffcore_rename() directly. This change is primarily about enabling future optimizations; the advantage of avoiding extra tree traversals is small compared to the cost of rename detection, and the advantage of avoiding the extra tree traversals is somewhat offset by the extra time spent in collect_merge_info() collecting the additional data anyway. However... For the testcases mentioned in commit 557ac0350d ("merge-ort: begin performance work; instrument with trace2_region_* calls", 2020-10-28), this change improves the performance as follows: Before After no-renames: 13.294 s ± 0.103 s 12.775 s ± 0.062 s mega-renames: 187.248 s ± 0.882 s 188.754 s ± 0.284 s just-one-mega: 5.557 s ± 0.017 s 5.599 s ± 0.019 s Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Feb 14, 2021 at 07:51 UTC f78cf976172fbad79a9ca4fc158f384fb868f177
1 file changed +59 -7
merge-ort.c
+59 -7
@@ -535,6 +535,23 @@ static void setup_path_info(struct merge_options *opt,
535 result->util = mi;
536 }
537
538 +static void add_pair(struct merge_options *opt,
539 + struct name_entry *names,
540 + const char *pathname,
541 + unsigned side,
542 + unsigned is_add /* if false, is_delete */)
543 +{
544 + struct diff_filespec *one, *two;
545 + struct rename_info *renames = &opt->priv->renames;
546 + int names_idx = is_add ? side : 0;
547 +
548 + one = alloc_filespec(pathname);
549 + two = alloc_filespec(pathname);
550 + fill_filespec(is_add ? two : one,
551 + &names[names_idx].oid, 1, names[names_idx].mode);
552 + diff_queue(&renames->pairs[side], one, two);
553 +}
554 +
555 static void collect_rename_info(struct merge_options *opt,
556 struct name_entry *names,
557 const char *dirname,
@@ -544,6 +561,7 @@ static void collect_rename_info(struct merge_options *opt,
561 unsigned match_mask)
562 {
563 struct rename_info *renames = &opt->priv->renames;
564 + unsigned side;
565
566 /* Update dirs_removed, as needed */
567 if (dirmask == 1 || dirmask == 3 || dirmask == 5) {
@@ -554,6 +572,21 @@ static void collect_rename_info(struct merge_options *opt,
572 if (sides & 2)
573 strset_add(&renames->dirs_removed[2], fullname);
574 }
575 +
576 + if (filemask == 0 || filemask == 7)
577 + return;
578 +
579 + for (side = MERGE_SIDE1; side <= MERGE_SIDE2; ++side) {
580 + unsigned side_mask = (1 << side);
581 +
582 + /* Check for deletion on side */
583 + if ((filemask & 1) && !(filemask & side_mask))
584 + add_pair(opt, names, fullname, side, 0 /* delete */);
585 +
586 + /* Check for addition on side */
587 + if (!(filemask & 1) && (filemask & side_mask))
588 + add_pair(opt, names, fullname, side, 1 /* add */);
589 + }
590 }
591
592 static int collect_merge_info_callback(int n,
@@ -2079,6 +2112,27 @@ static int process_renames(struct merge_options *opt,
2112 return clean_merge;
2113 }
2114
2115 +static void resolve_diffpair_statuses(struct diff_queue_struct *q)
2116 +{
2117 + /*
2118 + * A simplified version of diff_resolve_rename_copy(); would probably
2119 + * just use that function but it's static...
2120 + */
2121 + int i;
2122 + struct diff_filepair *p;
2123 +
2124 + for (i = 0; i < q->nr; ++i) {
2125 + p = q->queue[i];
2126 + p->status = 0; /* undecided */
2127 + if (!DIFF_FILE_VALID(p->one))
2128 + p->status = DIFF_STATUS_ADDED;
2129 + else if (!DIFF_FILE_VALID(p->two))
2130 + p->status = DIFF_STATUS_DELETED;
2131 + else if (DIFF_PAIR_RENAME(p))
2132 + p->status = DIFF_STATUS_RENAMED;
2133 + }
2134 +}
2135 +
2136 static int compare_pairs(const void *a_, const void *b_)
2137 {
2138 const struct diff_filepair *a = *((const struct diff_filepair **)a_);
@@ -2089,8 +2143,6 @@ static int compare_pairs(const void *a_, const void *b_)
2143
2144 /* Call diffcore_rename() to compute which files have changed on given side */
2145 static void detect_regular_renames(struct merge_options *opt,
2092 - struct tree *merge_base,
2093 - struct tree *side,
2146 unsigned side_index)
2147 {
2148 struct diff_options diff_opts;
@@ -2108,11 +2160,11 @@ static void detect_regular_renames(struct merge_options *opt,
2160 diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
2161 diff_setup_done(&diff_opts);
2162
2163 + diff_queued_diff = renames->pairs[side_index];
2164 trace2_region_enter("diff", "diffcore_rename", opt->repo);
2112 - diff_tree_oid(&merge_base->object.oid, &side->object.oid, "",
2113 - &diff_opts);
2114 - diffcore_std(&diff_opts);
2165 + diffcore_rename(&diff_opts);
2166 trace2_region_leave("diff", "diffcore_rename", opt->repo);
2167 + resolve_diffpair_statuses(&diff_queued_diff);
2168
2169 if (diff_opts.needed_rename_limit > renames->needed_limit)
2170 renames->needed_limit = diff_opts.needed_rename_limit;
@@ -2212,8 +2264,8 @@ static int detect_and_process_renames(struct merge_options *opt,
2264 memset(&combined, 0, sizeof(combined));
2265
2266 trace2_region_enter("merge", "regular renames", opt->repo);
2215 - detect_regular_renames(opt, merge_base, side1, MERGE_SIDE1);
2216 - detect_regular_renames(opt, merge_base, side2, MERGE_SIDE2);
2267 + detect_regular_renames(opt, MERGE_SIDE1);
2268 + detect_regular_renames(opt, MERGE_SIDE2);
2269 trace2_region_leave("merge", "regular renames", opt->repo);
2270
2271 trace2_region_enter("merge", "directory renames", opt->repo);