Replace merge-tree in more places
#5416
Conversation
This will help measuring the performance changes during apply and unapply operations.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
While at it, check make it a thing to check for clean merges easily.
It's definitely a debugging and profiling tool, nothing more.
Note that this at best is done for getting better merge results, it's not significant for performance.
Unfortunately it doesn't seem to be called from the UI anymore, just in tests, so there isn't any benchmark results.
|
|
||
| let mut opts = DiffOptions::new(); | ||
| opts.include_untracked(true); | ||
| opts.ignore_submodules(true); |
There was a problem hiding this comment.
gitoxide has no option to ignore submodules, but it would conflict on them if both change to different values. It's planned to have similar resolution facilities like Git has, but it's not implemented yet.
| }) | ||
| .for_each_to_obtain_tree(&wd_tree, |change| -> Result<_> { | ||
| match change { | ||
| Change::Addition { location, .. } => { |
There was a problem hiding this comment.
I am not 100% sure that this is what was intended in the git2 implementation, or why that is.
In the GItLab repo, I see 464 files changed in a discard hunk snapshot, but that was 4.6k if any location is used. Maybe this is why this 'add-only' filter is applied here. The UI gets quite slow displaying this.
| | Change::Modification { .. } | ||
| | Change::Rewrite { .. } => {} | ||
| } | ||
| if let Some(counts) = change |
There was a problem hiding this comment.
This is the expensive operation, of 3s, half goes into calculating line numbers. If performance ever becomes a bigger problem, these diffs should be avoided and be done on demand entirely.
| @@ -15,6 +15,21 @@ impl TestingRepository { | |||
| pub fn open() -> Self { | |||
| let tempdir = tempdir().unwrap(); | |||
| let repository = git2::Repository::init_opts(tempdir.path(), &init_opts()).unwrap(); | |||
| // TODO(ST): remove this once `gix::Repository::index_or_load_from_tree_or_empty()` | |||
There was a problem hiding this comment.
gitoxide will soon get an upgrade so this won't be an issue in future. For now it's easiest to cater to it and avoid an unborn HEAD.
The
gitoxidemerge-tree implementation is faster and better in quality, so it should be used where applicable. This PR attempts to find these places and make the switch.The goal is also to provide performance metrics, even though it's not expected we will see great numbers here as merge-tree is a smaller portion of the total work to be done.
Follow-up on #5411 .
Tasks
Use
merge-treein…gixfor nicer APIgixto the one frommainNotes for the Reviewer
resolve_index()have been postponed for now, as it's probably easiest to implement this directly in thegitoxidemerge tree function, for achieving the best possible quality. In theory, it might already work doing a post-process on the conflict entries, but that would be harder to get right.gitbutler-cherry-pickandgitbutler-edit-modethat rely on the index quite a bit, and even checkout an index. But it seems that it doesn't rely on the index to show merge-conflicts.Follow-Up
gixmerge trees APIs, see tree-merge follow-up GitoxideLabs/gitoxide#1651gix-mergein all the remaining places for performance and merge-qualityApply and Unapply
Before
When applying, merge-trees takes a whopping 72ms. It's clearly not the bottleneck, with a lot of time being spent in checkout.
When unapplying, it now clocks in at 139ms, so once again isn't going to change anything when replaced. Checking out files takes a long time, comparatively.
After
Like expected, there isn't much of a relevant change, with both merges during unapply and apply clocking in to ~90ms. Yes, it seems a bit faster, but is still far from flexing any muscle.
Unapply Ownership
Before
A tiny portion of 66ms of the 4s runtime is related to merging trees. Since it's the same repository (GitLab) the after should look very similar.
After
It's basically the same at 64ms.
Set Base Branch / Back to Integration
Before
112ms
After
93ms
Oplog Listing
Before
Listing takes 22s, and one full diff takes 7s, which is duplicated for an additional stats. Merging of trees takes 1.93s.
After replacing merge-trees
Merging trees now take 585ms, 3.3x of what was before.
After using
gitoxidefor the diffingThe whole operation is down to 7.3s, a 3x improvement! This is also due to not duplicating work.
It's noticeable how the TOML deserialisation takes 2s, which is a lot in comparison to the total runtime. But like I mentioned on Discord, I think there is ways to speed this up by not precomputing everything, instead of parallelising.
Checkout Branch Trees
Before
Merging in total takes about 300ms with two trivial branches. Like noticed before, writing the index to a tree is very expensive.
After
Now the same task takes 140ms for merging and writing the tree. Notably, writing the tree now takes 6ms , 29x faster than having to write a whole index.
Upstream Integration Statuses
Before
As often, most time is spent elsewhere (and duplicated in
stack_series()), but that aside we are looking at 1.09s for merging trees in that function.After
Now it clocks in at 640ms, at 1.7x.