Optimization batch 8: use file basenames even more #844
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
newren
force-pushed
the
temporary/ort-perf-batch-7
branch
from
4dada11
to
581dca0
Compare
newren
force-pushed
the
ort-perf-batch-8
branch
2 times, most recently
from
3bfbd40
to
f3cda8d
Compare
newren
force-pushed
the
temporary/ort-perf-batch-7
branch
from
581dca0
to
1d941c3
Compare
newren
force-pushed
the
ort-perf-batch-8
branch
2 times, most recently
from
c53920b
to
ac1fea5
Compare
newren
force-pushed
the
temporary/ort-perf-batch-7
branch
from
1d941c3
to
a0e75d8
Compare
newren
force-pushed
the
ort-perf-batch-8
branch
3 times, most recently
from
d266fa7
to
e546ae2
Compare
newren
force-pushed
the
temporary/ort-perf-batch-7
branch
from
fc72d24
to
fedb3d3
Compare
newren
force-pushed
the
ort-perf-batch-8
branch
3 times, most recently
from
81bcfc4
to
46774dc
Compare
newren
force-pushed
the
temporary/ort-perf-batch-7
branch
from
fedb3d3
to
3c9d4fe
Compare
newren
force-pushed
the
ort-perf-batch-8
branch
from
46774dc
to
8b2286c
Compare
newren
force-pushed
the
temporary/ort-perf-batch-7
branch
from
3c9d4fe
to
aeca14f
Compare
newren
force-pushed
the
ort-perf-batch-8
branch
from
8b2286c
to
1df498b
Compare
|
/submit |
|
Submitted as pull.844.git.1613289544.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
newren
force-pushed
the
ort-perf-batch-8
branch
from
1df498b
to
805c101
Compare
|
/submit |
|
Submitted as pull.844.v2.git.1614123848.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): |
|
This patch series was integrated into seen via git@de7e639. |
|
There was a status update about the branch Rename detection rework continues. Will cook in 'next'. |
|
This patch series was integrated into seen via git@891cf66. |
|
This patch series was integrated into seen via git@7d6738c. |
A previous commit noted that it is very common for people to move files across directories while keeping their filename the same. The last few commits took advantage of this and showed that we can accelerate rename detection significantly using basenames; since files with the same basename serve as likely rename candidates, we can check those first and remove them from the rename candidate pool if they are sufficiently similar. Unfortunately, the previous optimization was limited by the fact that the remaining basenames after exact rename detection are not always unique. Many repositories have hundreds of build files with the same name (e.g. Makefile, .gitignore, build.gradle, etc.), and may even have hundreds of source files with the same name. (For example, the linux kernel has 100 setup.c, 87 irq.c, and 112 core.c files. A repository at $DAYJOB has a lot of ObjectFactory.java and Plugin.java files). For these files with non-unique basenames, we are faced with the task of attempting to determine or guess which directory they may have been relocated to. Such a task is precisely the job of directory rename detection. However, there are two catches: (1) the directory rename detection code has traditionally been part of the merge machinery rather than diffcore-rename.c, and (2) directory rename detection currently runs after regular rename detection is complete. The 1st catch is just an implementation issue that can be overcome by some code shuffling. The 2nd requires us to add a further approximation: we only have access to exact renames at this point, so we need to do directory rename detection based on just exact renames. In some cases we won't have exact renames, in which case this extra optimization won't apply. We also choose to not apply the optimization unless we know that the underlying directory was removed, which will require extra data to be passed in to diffcore_rename_extended(). Also, even if we get a prediction about which directory a file may have relocated to, we will still need to check to see if there is a file in the predicted directory, and then compare the two files to see if they meet the higher min_basename_score threshold required for marking the two files as renames. This commit introduces an idx_possible_rename() function which will do this directory rename detection for us and give us the index within rename_dst of the resulting filename. For now, this function is hardcoded to return -1 (not found) and just hooks up how its results would be used once we have a more complete implementation in place. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com>
Add a new struct dir_rename_info with various values we need inside our idx_possible_rename() function introduced in the previous commit. Add a basic implementation for this function showing how we plan to use the variables, but which will just return early with a value of -1 (not found) when those variables are not set up. Future commits will do the work necessary to set up those other variables so that idx_possible_rename() does not always return -1. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com>
Compute a mapping of full filename to the index within rename_dst where that filename is found, and store it in idx_map. idx_possible_rename() needs this to quickly finding an array entry in rename_dst given the pathname. While at it, add placeholder initializations for dir_rename_count and dir_rename_guess; these will be more fully populated in subsequent commits. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com>
Move the computation of dir_rename_count from merge-ort.c to diffcore-rename.c, making slight adjustments to the data structures based on the move. While the diffstat looks large, viewing this commit with --color-moved makes it clear that only about 20 lines changed. With this patch, the computation of dir_rename_count is still only done after inexact rename detection, but subsequent commits will add a preliminary computation of dir_rename_count after exact rename detection, followed by some updates after inexact rename detection. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com>
As we adjust the usage of dir_rename_count we want to have a function for clearing, or partially clearing it out. Add a partial_clear_dir_rename_count() function for this purpose. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com>
This continues the migration of the directory rename detection code into diffcore-rename, now taking the simple step of combining it with the dir_rename_info struct. Future commits will then make dir_rename_counts be computed in stages, and add computation of dir_rename_guess. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com>
When diffcore_rename_extended() is passed a NULL dir_rename_count, we will still want to create a temporary one for use by find_basename_matches(), but have it fully deallocated before diffcore_rename_extended() returns. However, when diffcore_rename_extended() is passed a dir_rename_count, we want to fill that strmap with appropriate values and return it. However, for our interim purposes we may also add entries corresponding to directories that cannot have been renamed due to still existing on both sides. Extend cleanup_dir_rename_info() to handle these two different cases, cleaning up the relevant bits of information for each case. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com>
Compute dir_rename_counts based just on exact renames to start, as that can provide us useful information in find_basename_matches(). This is done by moving the code from compute_dir_rename_counts() into initialize_dir_rename_info(), resulting in it being computed earlier and based just on exact renames. Since that's an incomplete result, we augment the counts via calling update_dir_rename_counts() after each basename-guide and inexact rename detection match is found. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com>
We are using dir_rename_counts to count the number of other directories that files within a directory moved to. We only need this information for directories that disappeared, though, so we can return early from update_dir_rename_counts() for other paths. If dirs_removed is passed to diffcore_rename_extended(), then it provides the relevant bits of information for us to limit this counting to relevant dirs. If dirs_removed is not passed, we would need to compute some replacement in order to do this limiting. Introduce a new info->relevant_source_dirs variable for this purpose, even though at this stage we will only set it to dirs_removed for simplicity. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com>
dir_rename_counts has a mapping of a mapping, in particular, it has
old_dir => { new_dir => count }
We want a simple mapping of
old_dir => new_dir
based on which new_dir had the highest count for a given old_dir.
Compute this and store it in dir_rename_guess.
This is the final piece of the puzzle needed to make our guesses at
which directory files have been moved to when basenames aren't unique.
For the testcases mentioned in commit 557ac03 ("merge-ort: begin
performance work; instrument with trace2_region_* calls", 2020-10-28),
this change improves the performance as follows:
Before After
no-renames: 12.775 s ± 0.062 s 12.596 s ± 0.061 s
mega-renames: 188.754 s ± 0.284 s 130.465 s ± 0.259 s
just-one-mega: 5.599 s ± 0.019 s 3.958 s ± 0.010 s
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
newren
force-pushed
the
temporary/ort-perf-batch-7
branch
from
aeca14f
to
f78cf97
Compare
newren
force-pushed
the
ort-perf-batch-8
branch
from
4be565c
to
fbc4316
Compare
|
This patch series was integrated into seen via git@b382178. |
|
There was a status update about the branch Rename detection rework continues. Will cook in 'next'. |
|
This patch series was integrated into seen via git@3f6755e. |
|
This patch series was integrated into seen via git@feb8f96. |
|
This patch series was integrated into seen via git@a467314. |
|
There was a status update about the branch Rename detection rework continues. Will merge to 'master'. |
|
This patch series was integrated into seen via git@fb339f8. |
|
This patch series was integrated into seen via git@dd4048d. |
|
This patch series was integrated into next via git@dd4048d. |
|
This patch series was integrated into master via git@dd4048d. |
|
Closed via dd4048d. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series depends on en/diffcore-rename (a concatenation of what I
was calling ort-perf-batch-6 and ort-perf-batch-7).
Changes since v3:
cc: Derrick Stolee stolee@gmail.com
cc: Elijah Newren newren@gmail.com
cc: Junio C Hamano gitster@pobox.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com