@matheusbsbs / matheus-bagliano / commits / 5a3743da32

merge-ort: replace string_list_df_name_compare with faster alternative

Gathering accumulated times from trace2 output on the mega-renames testcase, I saw the following timings (where I'm only showing a few lines to highlight the portions of interest): 10.120 : label:incore_nonrecursive 4.462 : ..label:process_entries 3.143 : ....label:process_entries setup 2.988 : ......label:plist special sort 1.305 : ....label:processing 2.604 : ..label:collect_merge_info 2.018 : ..label:merge_start 1.018 : ..label:renames In the above output, note that the 4.462 seconds for process_entries was split as 3.143 seconds for "process_entries setup" and 1.305 seconds for "processing" (and a little time for other stuff removed from the highlight). Most of the "process_entries setup" time was spent on "plist special sort" which corresponds to the following code: trace2_region_enter("merge", "plist special sort", opt->repo); plist.cmp = string_list_df_name_compare; string_list_sort(&plist); trace2_region_leave("merge", "plist special sort", opt->repo); In other words, in a merge strategy that would be invoked by passing "-sort" to either rebase or merge, sorting an array takes more time than anything else. Serves me right for naming my merge strategy this way. Rewrite the comparison function in a way that does not require finding out the lengths of the strings when comparing them. While at it, tweak the code for our specific case -- no need to handle a variety of modes, for example. The combination of these changes reduced the time spent in "plist special sort" by ~25% in the mega-renames case. 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: 5.622 s ± 0.059 s 5.235 s ± 0.042 s mega-renames: 10.127 s ± 0.073 s 9.419 s ± 0.107 s just-one-mega: 500.3 ms ± 3.8 ms 480.1 ms ± 3.9 ms Signed-off-by: Elijah Newren <newren@gmail.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Jun 8, 2021 at 16:11 UTC 5a3743da326b2e041c65ac12ba0d04a1fed45b58
1 file changed +47 -20
merge-ort.c
+47 -20
@@ -2746,31 +2746,58 @@ simple_cleanup:
2746
2747 /*** Function Grouping: functions related to process_entries() ***/
2748
2749 -static int string_list_df_name_compare(const char *one, const char *two)
2749 +static int sort_dirs_next_to_their_children(const char *one, const char *two)
2750 {
2751 - int onelen = strlen(one);
2752 - int twolen = strlen(two);
2751 + unsigned char c1, c2;
2752 +
2753 /*
2754 - * Here we only care that entries for D/F conflicts are
2755 - * adjacent, in particular with the file of the D/F conflict
2756 - * appearing before files below the corresponding directory.
2757 - * The order of the rest of the list is irrelevant for us.
2754 + * Here we only care that entries for directories appear adjacent
2755 + * to and before files underneath the directory. We can achieve
2756 + * that by pretending to add a trailing slash to every file and
2757 + * then sorting. In other words, we do not want the natural
2758 + * sorting of
2759 + * foo
2760 + * foo.txt
2761 + * foo/bar
2762 + * Instead, we want "foo" to sort as though it were "foo/", so that
2763 + * we instead get
2764 + * foo.txt
2765 + * foo
2766 + * foo/bar
2767 + * To achieve this, we basically implement our own strcmp, except that
2768 + * if we get to the end of either string instead of comparing NUL to
2769 + * another character, we compare '/' to it.
2770 + *
2771 + * If this unusual "sort as though '/' were appended" perplexes
2772 + * you, perhaps it will help to note that this is not the final
2773 + * sort. write_tree() will sort again without the trailing slash
2774 + * magic, but just on paths immediately under a given tree.
2775 *
2759 - * To achieve this, we sort with df_name_compare and provide
2760 - * the mode S_IFDIR so that D/F conflicts will sort correctly.
2761 - * We use the mode S_IFDIR for everything else for simplicity,
2762 - * since in other cases any changes in their order due to
2763 - * sorting cause no problems for us.
2776 + * The reason to not use df_name_compare directly was that it was
2777 + * just too expensive (we don't have the string lengths handy), so
2778 + * it was reimplemented.
2779 */
2765 - int cmp = df_name_compare(one, onelen, S_IFDIR,
2766 - two, twolen, S_IFDIR);
2780 +
2781 /*
2768 - * Now that 'foo' and 'foo/bar' compare equal, we have to make sure
2769 - * that 'foo' comes before 'foo/bar'.
2782 + * NOTE: This function will never be called with two equal strings,
2783 + * because it is used to sort the keys of a strmap, and strmaps have
2784 + * unique keys by construction. That simplifies our c1==c2 handling
2785 + * below.
2786 */
2771 - if (cmp)
2772 - return cmp;
2773 - return onelen - twolen;
2787 +
2788 + while (*one && (*one == *two)) {
2789 + one++;
2790 + two++;
2791 + }
2792 +
2793 + c1 = *one ? *one : '/';
2794 + c2 = *two ? *two : '/';
2795 +
2796 + if (c1 == c2) {
2797 + /* Getting here means one is a leading directory of the other */
2798 + return (*one) ? 1 : -1;
2799 + } else
2800 + return c1 - c2;
2801 }
2802
2803 static int read_oid_strbuf(struct merge_options *opt,
@@ -3489,7 +3516,7 @@ static void process_entries(struct merge_options *opt,
3516 trace2_region_leave("merge", "plist copy", opt->repo);
3517
3518 trace2_region_enter("merge", "plist special sort", opt->repo);
3492 - plist.cmp = string_list_df_name_compare;
3519 + plist.cmp = sort_dirs_next_to_their_children;
3520 string_list_sort(&plist);
3521 trace2_region_leave("merge", "plist special sort", opt->repo);
3522