Skip to content

Optimization batch 11: avoid repeatedly detecting same renames #859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from
Closed

Optimization batch 11: avoid repeatedly detecting same renames #859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
129136a
t6423: rename file within directory that other side renamed
newren Apr 28, 2021
4246d01
Documentation/technical: describe remembering renames optimization
newren Apr 29, 2021
22b121a
fast-rebase: change assert() to BUG()
newren May 3, 2021
cfd8cbe
fast-rebase: write conflict state to working tree, index, and HEAD
newren Apr 28, 2021
f1c20da
t6429: testcases for remembering renames
newren Apr 28, 2021
304a276
merge-ort: add data structures for in-memory caching of rename detection
newren Jan 30, 2021
5a2f3b7
merge-ort: populate caches of rename detection results
newren Jan 30, 2021
e4a97b1
merge-ort: add code to check for whether cached renames can be reused
newren Jan 30, 2021
441e66f
merge-ort: avoid accidental API mis-use
newren Jan 31, 2021
472a539
merge-ort: preserve cached renames for the appropriate side
newren Jan 31, 2021
386fd24
merge-ort: add helper functions for using cached renames
newren Jan 31, 2021
886a171
merge-ort: handle interactions of caching and rename/rename(1to1) cases
newren May 1, 2021
76e2537
merge-ort, diffcore-rename: employ cached renames when possible
newren Jan 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
671 changes: 671 additions & 0 deletions Documentation/technical/remembering-renames.txt
View file
Open in desktop

Large diffs are not rendered by default.

22 changes: 18 additions & 4 deletions diffcore-rename.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,8 @@ static void update_dir_rename_counts(struct dir_rename_info *info,
static void initialize_dir_rename_info(struct dir_rename_info *info,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 5/3/21 10:12 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
...
> diff --git a/merge-ort.c b/merge-ort.c
> index 2fc98b803d1c..17dc3deb3c73 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -753,15 +753,48 @@ static void add_pair(struct merge_options *opt,
>  	struct rename_info *renames = &opt->priv->renames;
>  	int names_idx = is_add ? side : 0;
>  
> -	if (!is_add) {
> +	if (is_add) {
> +		if (strset_contains(&renames->cached_target_names[side],
> +				    pathname))
> +			return;
> +	} else {
>  		unsigned content_relevant = (match_mask == 0);
>  		unsigned location_relevant = (dir_rename_mask == 0x07);
>  
> +		/*
> +		 * If pathname is found in cached_irrelevant[side] due to
> +		 * previous pick but for this commit content is relevant,
> +		 * then we need to remove it from cached_irrelevant.
> +		 */
> +		if (content_relevant)
> +			/* strset_remove is no-op if strset doesn't have key */
> +			strset_remove(&renames->cached_irrelevant[side],
> +				      pathname);

I see, content can become relevant again.

...

> diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
> index f47d8924ee73..035edc40b1eb 100755
> --- a/t/t6429-merge-sequence-rename-caching.sh
> +++ b/t/t6429-merge-sequence-rename-caching.sh
> @@ -101,10 +101,10 @@ test_expect_success 'caching renames does not preclude finding new ones' '
>  # dramatic change in size of the file, but remembering the rename and
>  # reusing it is reasonable too.
>  #
> -# Rename detection (diffcore_rename_extended()) will run twice here; it is
> -# not needed on the topic side of history for either of the two commits
> -# being merged, but it is needed on the upstream side of history for each
> -# commit being picked.
> +# We do test here that we expect rename detection to only be run once total
> +# (the topic side of history doesn't need renames, and with caching we
> +# should be able to only run rename detection on the upstream side one
> +# time.)
>  test_expect_success 'cherry-pick both a commit and its immediate revert' '
>  	test_create_repo pick-commit-and-its-immediate-revert &&
>  	(
> @@ -140,11 +140,11 @@ test_expect_success 'cherry-pick both a commit and its immediate revert' '
>  		GIT_TRACE2_PERF="$(pwd)/trace.output" &&
>  		export GIT_TRACE2_PERF &&
>  
> -		test_might_fail test-tool fast-rebase --onto HEAD upstream~1 topic &&
> +		test-tool fast-rebase --onto HEAD upstream~1 topic &&

Here is a change of behavior, but it appears to be a good one!

>  		#git cherry-pick upstream~1..topic &&
>  
>  		grep region_enter.*diffcore_rename trace.output >calls &&
> -		test_line_count = 2 calls
> +		test_line_count = 1 calls
>  	)
...
> @@ -450,7 +459,7 @@ test_expect_success 'dir rename unneeded, then add new file to old dir' '
>  		#git cherry-pick upstream..topic &&
>  
>  		grep region_enter.*diffcore_rename trace.output >calls &&
> -		test_line_count = 3 calls &&
> +		test_line_count = 2 calls &&
>  
>  		git ls-files >tracked &&
>  		test_line_count = 5 tracked &&
> @@ -516,7 +525,7 @@ test_expect_success 'dir rename unneeded, then rename existing file into old dir
>  		#git cherry-pick upstream..topic &&
>  
>  		grep region_enter.*diffcore_rename trace.output >calls &&
> -		test_line_count = 4 calls &&
> +		test_line_count = 3 calls &&

I appreciate that this use of tracing demonstrates a change of
internal behavior.

>  		test_path_is_missing somefile &&
>  		test_path_is_missing olddir/newfile &&
> @@ -648,9 +657,8 @@ test_expect_success 'caching renames only on upstream side, part 1' '
>  # for the wrong side of history.
>  #
>  #
> -# This testcase should only need three calls to diffcore_rename_extended(),
> -# because there are no renames on the topic side of history for picking
> -# Topic_2.
> +# This testcase should only need two calls to diffcore_rename_extended(),
> +# both for the first merge, one for each side of history.
>  #
>  test_expect_success 'caching renames only on upstream side, part 2' '
>  	test_setup_topic_rename cache-renames-only-upstream-rename-file &&
> @@ -677,7 +685,7 @@ test_expect_success 'caching renames only on upstream side, part 2' '
>  		#git cherry-pick upstream..topic &&
>  
>  		grep region_enter.*diffcore_rename trace.output >calls &&
> -		test_line_count = 3 calls &&
> +		test_line_count = 2 calls &&

Same here.

As I was reading, I was also thinking that it would be good to
have some kind of tracing, if only a summary of how often we
relied upon the cached renames. That would present a mechanism
for the test cases to verify that the rename cache is behaving
as expected, but also provides a way to diagnose any issues that
might arise in the future by asking a user to reproduce a
problematic rebase/merge with a GIT_TRACE2* target. Such a
change would fit as a follow-up, and does not need to insert
into an already heavy patch.

I have now read all of the patches in this series to the level
I can. It's all very deep stuff, so the more we can rely on the
tests to show correctness, the better.

I appreciate the extra tests that you added, which increases my
confidence in the series.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Mon, May 17, 2021 at 7:23 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 5/3/21 10:12 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> ...
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 2fc98b803d1c..17dc3deb3c73 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -753,15 +753,48 @@ static void add_pair(struct merge_options *opt,
> >       struct rename_info *renames = &opt->priv->renames;
> >       int names_idx = is_add ? side : 0;
> >
> > -     if (!is_add) {
> > +     if (is_add) {
> > +             if (strset_contains(&renames->cached_target_names[side],
> > +                                 pathname))
> > +                     return;
> > +     } else {
> >               unsigned content_relevant = (match_mask == 0);
> >               unsigned location_relevant = (dir_rename_mask == 0x07);
> >
> > +             /*
> > +              * If pathname is found in cached_irrelevant[side] due to
> > +              * previous pick but for this commit content is relevant,
> > +              * then we need to remove it from cached_irrelevant.
> > +              */
> > +             if (content_relevant)
> > +                     /* strset_remove is no-op if strset doesn't have key */
> > +                     strset_remove(&renames->cached_irrelevant[side],
> > +                                   pathname);
>
> I see, content can become relevant again.
>
> ...
>
> > diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
> > index f47d8924ee73..035edc40b1eb 100755
> > --- a/t/t6429-merge-sequence-rename-caching.sh
> > +++ b/t/t6429-merge-sequence-rename-caching.sh
> > @@ -101,10 +101,10 @@ test_expect_success 'caching renames does not preclude finding new ones' '
> >  # dramatic change in size of the file, but remembering the rename and
> >  # reusing it is reasonable too.
> >  #
> > -# Rename detection (diffcore_rename_extended()) will run twice here; it is
> > -# not needed on the topic side of history for either of the two commits
> > -# being merged, but it is needed on the upstream side of history for each
> > -# commit being picked.
> > +# We do test here that we expect rename detection to only be run once total
> > +# (the topic side of history doesn't need renames, and with caching we
> > +# should be able to only run rename detection on the upstream side one
> > +# time.)
> >  test_expect_success 'cherry-pick both a commit and its immediate revert' '
> >       test_create_repo pick-commit-and-its-immediate-revert &&
> >       (
> > @@ -140,11 +140,11 @@ test_expect_success 'cherry-pick both a commit and its immediate revert' '
> >               GIT_TRACE2_PERF="$(pwd)/trace.output" &&
> >               export GIT_TRACE2_PERF &&
> >
> > -             test_might_fail test-tool fast-rebase --onto HEAD upstream~1 topic &&
> > +             test-tool fast-rebase --onto HEAD upstream~1 topic &&
>
> Here is a change of behavior, but it appears to be a good one!
>
> >               #git cherry-pick upstream~1..topic &&
> >
> >               grep region_enter.*diffcore_rename trace.output >calls &&
> > -             test_line_count = 2 calls
> > +             test_line_count = 1 calls
> >       )
> ...
> > @@ -450,7 +459,7 @@ test_expect_success 'dir rename unneeded, then add new file to old dir' '
> >               #git cherry-pick upstream..topic &&
> >
> >               grep region_enter.*diffcore_rename trace.output >calls &&
> > -             test_line_count = 3 calls &&
> > +             test_line_count = 2 calls &&
> >
> >               git ls-files >tracked &&
> >               test_line_count = 5 tracked &&
> > @@ -516,7 +525,7 @@ test_expect_success 'dir rename unneeded, then rename existing file into old dir
> >               #git cherry-pick upstream..topic &&
> >
> >               grep region_enter.*diffcore_rename trace.output >calls &&
> > -             test_line_count = 4 calls &&
> > +             test_line_count = 3 calls &&
>
> I appreciate that this use of tracing demonstrates a change of
> internal behavior.
>
> >               test_path_is_missing somefile &&
> >               test_path_is_missing olddir/newfile &&
> > @@ -648,9 +657,8 @@ test_expect_success 'caching renames only on upstream side, part 1' '
> >  # for the wrong side of history.
> >  #
> >  #
> > -# This testcase should only need three calls to diffcore_rename_extended(),
> > -# because there are no renames on the topic side of history for picking
> > -# Topic_2.
> > +# This testcase should only need two calls to diffcore_rename_extended(),
> > +# both for the first merge, one for each side of history.
> >  #
> >  test_expect_success 'caching renames only on upstream side, part 2' '
> >       test_setup_topic_rename cache-renames-only-upstream-rename-file &&
> > @@ -677,7 +685,7 @@ test_expect_success 'caching renames only on upstream side, part 2' '
> >               #git cherry-pick upstream..topic &&
> >
> >               grep region_enter.*diffcore_rename trace.output >calls &&
> > -             test_line_count = 3 calls &&
> > +             test_line_count = 2 calls &&
>
> Same here.
>
> As I was reading, I was also thinking that it would be good to
> have some kind of tracing, if only a summary of how often we
> relied upon the cached renames.

That's an interesting thought.  If there are renames that remain
cached, the code always uses them (whether we need the cached rename
or not doesn't matter since we already have it and it is cheap to
use), so a summary would probably be the only thing that would make
sense.

The easiest summary of how often we rely upon cached renames, is
checking if we have enough to avoid calling diffcore_rename_extended()
entirely.  That's precisely what the code above does that you are
responding to.

What more information could we get out?  I guess we could get ever so
slightly more information by tracking how many times we decide that
the cache from a previous merge can be re-used (by tracking the number
of times that cached_valid_side is set to 1 or 2 instead of 0).  Since
we sometimes have some cached renames but not enough to skip rename
detection (because source paths that were irrelevant in previous
commits are marked relevant for the current commit), this would be an
independent number from the region_enter-diffcore_rename count used
above.

> That would present a mechanism
> for the test cases to verify that the rename cache is behaving
> as expected

How so?  I did check something like that above with verifying I was
able to reduce the number of calls to diffcore_rename_extended() while
still getting the expected result (which can only be done if renames
are known).  The only additional thing I can think of we could check
would be a testcase where renames are cached and can be re-used but
it's not enough to avoid calling diffcore_rename_extended().  Did you
have something else in mind?

> but also provides a way to diagnose any issues that
> might arise in the future by asking a user to reproduce a
> problematic rebase/merge with a GIT_TRACE2* target. Such a
> change would fit as a follow-up, and does not need to insert
> into an already heavy patch.

I'm not sure what information could be recorded that would help
diagnose any such issues.  "9 out of 10 commits in your rebase reused
cached renames" just doesn't seem granular enough to help.  Is there
something else you were thinking of recording?


> I have now read all of the patches in this series to the level
> I can. It's all very deep stuff, so the more we can rely on the
> tests to show correctness, the better.
>
> I appreciate the extra tests that you added, which increases my
> confidence in the series.
>
> Thanks,
> -Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 5/19/21 8:36 PM, Elijah Newren wrote:
> On Mon, May 17, 2021 at 7:23 AM Derrick Stolee <stolee@gmail.com> wrote:>> As I was reading, I was also thinking that it would be good to
>> have some kind of tracing, if only a summary of how often we
>> relied upon the cached renames.
> 
> That's an interesting thought.  If there are renames that remain
> cached, the code always uses them (whether we need the cached rename
> or not doesn't matter since we already have it and it is cheap to
> use), so a summary would probably be the only thing that would make
> sense.
> 
> The easiest summary of how often we rely upon cached renames, is
> checking if we have enough to avoid calling diffcore_rename_extended()
> entirely.  That's precisely what the code above does that you are
> responding to.
> 
> What more information could we get out?  I guess we could get ever so
> slightly more information by tracking how many times we decide that
> the cache from a previous merge can be re-used (by tracking the number
> of times that cached_valid_side is set to 1 or 2 instead of 0).  Since
> we sometimes have some cached renames but not enough to skip rename
> detection (because source paths that were irrelevant in previous
> commits are marked relevant for the current commit), this would be an
> independent number from the region_enter-diffcore_rename count used
> above.
> 
>> That would present a mechanism
>> for the test cases to verify that the rename cache is behaving
>> as expected
> 
> How so?  I did check something like that above with verifying I was
> able to reduce the number of calls to diffcore_rename_extended() while
> still getting the expected result (which can only be done if renames
> are known).  The only additional thing I can think of we could check
> would be a testcase where renames are cached and can be re-used but
> it's not enough to avoid calling diffcore_rename_extended().  Did you
> have something else in mind?
> 
>> but also provides a way to diagnose any issues that
>> might arise in the future by asking a user to reproduce a
>> problematic rebase/merge with a GIT_TRACE2* target. Such a
>> change would fit as a follow-up, and does not need to insert
>> into an already heavy patch.
> 
> I'm not sure what information could be recorded that would help
> diagnose any such issues.  "9 out of 10 commits in your rebase reused
> cached renames" just doesn't seem granular enough to help.  Is there
> something else you were thinking of recording?

My initial thought was to include basic summary statistics, such
as "number of cached renames used" and "number of new renames" and
"number of invalidated cached renames" or something, summarized per
commit in the list. The information might not be a clear way to
find a root cause to a strange rebase, but it would help someone
looking for the root cause to know whether this rename cache is
involved or not.

As for the use in the test cases, we might be able to see the list
of trace2 results and extract the number of each type of statistic,
matching them to our expectations. That might make the tests too
fragile to future changes, though.

I'm commenting also on your v3 that we don't need to pursue the
trace2 idea right now, and can do it if we find it necessary to
diagnose any issues with the feature.

Thanks,
-Stolee

struct strintmap *relevant_sources,
struct strintmap *dirs_removed,
struct strmap *dir_rename_count)
struct strmap *dir_rename_count,
struct strmap *cached_pairs)
{
struct hashmap_iter iter;
struct strmap_entry *entry;
Expand Down Expand Up @@ -633,6 +634,17 @@ static void initialize_dir_rename_info(struct dir_rename_info *info,
rename_dst[i].p->two->path);
}

/* Add cached_pairs to counts */
strmap_for_each_entry(cached_pairs, &iter, entry) {
const char *old_name = entry->key;
const char *new_name = entry->value;
if (!new_name)
/* known delete; ignore it */
continue;

update_dir_rename_counts(info, dirs_removed, old_name, new_name);
}

/*
* Now we collapse
* dir_rename_count: old_directory -> {new_directory -> count}
Expand Down Expand Up @@ -1247,7 +1259,8 @@ static void handle_early_known_dir_renames(struct dir_rename_info *info,
void diffcore_rename_extended(struct diff_options *options,
struct strintmap *relevant_sources,
struct strintmap *dirs_removed,
struct strmap *dir_rename_count)
struct strmap *dir_rename_count,
struct strmap *cached_pairs)
{
int detect_rename = options->detect_rename;
int minimum_score = options->rename_score;
Expand Down Expand Up @@ -1363,7 +1376,8 @@ void diffcore_rename_extended(struct diff_options *options,
/* Preparation for basename-driven matching. */
trace2_region_enter("diff", "dir rename setup", options->repo);
initialize_dir_rename_info(&info, relevant_sources,
dirs_removed, dir_rename_count);
dirs_removed, dir_rename_count,
cached_pairs);
trace2_region_leave("diff", "dir rename setup", options->repo);

/* Utilize file basenames to quickly find renames. */
Expand Down Expand Up @@ -1560,5 +1574,5 @@ void diffcore_rename_extended(struct diff_options *options,

void diffcore_rename(struct diff_options *options)
{
diffcore_rename_extended(options, NULL, NULL, NULL);
diffcore_rename_extended(options, NULL, NULL, NULL, NULL);
}
3 changes: 2 additions & 1 deletion diffcore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ void diffcore_rename(struct diff_options *);
void diffcore_rename_extended(struct diff_options *options,
struct strintmap *relevant_sources,
struct strintmap *dirs_removed,
struct strmap *dir_rename_count);
struct strmap *dir_rename_count,
struct strmap *cached_pairs);
void diffcore_merge_broken(void);
void diffcore_pickaxe(struct diff_options *);
void diffcore_order(const char *orderfile);
Expand Down
Loading