list virtual branches performance #4771
Merged
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
|
@Byron is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. |
This can be useful for those who seek extra information, and while it's dominating refresh times.
It calls `list_virtual_branches()` which kind of has the effect of a status call.
There we don't necessarily need every last bit of optimization like one would want in release profiles, but build performance is of the essence for developer productivity.
The worktree diff is essentially a 'git status' with a given tree, and that can be expressed with a diff of the workdir, the index and a tree. This comes at the expense of not being able to control which file sizes are diffed.
```` GITBUTLER_PERFORMANCE_LOG=1 LOG_LEVEL=debug pnpm tauri dev ```` Additionally, some noise was removed by turning a warning into a trace.
…nches. That way, the head is static which allows to re-use the worktree status across multiple related calls. Also, add a way to quickly get the `head_commit()` from a `git2::Repository`. Note that this may cause breakage in the app, as now it will see the actual head, not a computed one, but that should also help us to find places that didn't properly update their state.
Also make sure that important `gix` crates are always compiled with optimization for proper performance. This is particularly useful when optimizing performance as not everything has to be done in release mode. However, it also causes CI to be slower as it compiles some dependencies longer. The problem realy is that `tauri dev` doesn't support custom profiles, so there is only `dev` and `release`.
It's used for emission of changed files, but also for listing
branch information.
Make sure we only run the worktree-status once to save time.
This also unifies the acquisition of the `HEAD^{commit}`,
which one day will be a natural feature once `gix::Repository`
is used.
Byron
commented
Aug 28, 2024
Byron
force-pushed
the
git2-to-gix
branch
2 times, most recently
from
c44d090
to
c86300b
Compare
This was lost previously when switching it over to a read-only implementation. Implementing it with an ignore list will take time, 400ms in the GitLab repository, but it's not slower than it was before and it is always preferred to not dump objects into the ODB unnecessarily.
That way, the application will not run into unexpected cases.
krlvi
approved these changes
Aug 29, 2024
Byron
commented
Aug 29, 2024
There was a problem hiding this comment.
I left a few more notes on this merged PR, @Caleb-T-Owens and @krlvi, just for your information.
| ) -> Result<DiffByPathMap> { | ||
| let repository = context.repository(); | ||
| let head_commit = repository.head_commit()?; | ||
| gitbutler_diff::workdir(repository, &head_commit.id()) |
There was a problem hiding this comment.
I think this is the only place where we drive gitbutler_diff::workdir() with the (possibly stale) HEAD commit, and I do wonder if that's always correct.
However, I also believe that the HEAD integration commit should not be able to go stale in the first place, and would rather want to see that fixed.
It's something that isn't attempted here just yet though.
Comment on lines
+27
to
+29
| /// This should be used to update the `gitbutler/workspace` ref with, which is usually | ||
| /// done from [`update_gitbutler_integration()`], after any of its input changes. | ||
| /// This is namely the conflicting state, or any head of the virtual branches. |
There was a problem hiding this comment.
I added the list of dependencies that I see are used here in the hopes that is raises awareness in functions that alter these inputs.
In theory, these should adjust the integration commit to match, so eventually all read-only code can rely on HEAD to be valid.
2 tasks
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.
An early PR to capture the research and first steps towards making
list_virtual_branchesfaster by introducinggix.Refactoring might happen here as well.
Tasks
list_virtual_branchesfor CLIlist_virtual_branchesby profiling: gitbutler_diff:workdir()diff::workdir()be implemented without writing objects (but also, without skipping files probably?) ingit2?diff::workdir()call when responding to filesystem events (reuse the data instead) - makes it 800ms fasterdiff::workdir()get_workspace_head()is required?Notes for the Reviewer
recalculate_everything()is about twice as fast in this PR as it is on Master.index.add_all(*).diff::workdir()is read-only, but a little slower as it now has to find big files to skip first (in order to be mergeable). Overall it's still an improvement, but wantsgix(in follow-up)Follow-Ups
gixfordiff::workdir()gixforget_worktree_head()- merge-treesgixforcreate_wd_treetree_from_worktree()function that does the job correctly and quickly.Shortcomings of current implementation
index::add(*)Performance Results
Everything was taken from the GitLab repository, with a
--releasebuild.workspace big files
Before
731ms with
git2.After
389ms for traversing 78k files, a 1.88x improvement. It's faster as it won't have to do an actual status, it just finds untracked files. Of course, ideally one day there is no need to exclude untracked large files as untracked files as virtual branches won't 'soak up' the worktree like they do now.
duplicate workdir call
Before
After
Final Result
On
master, unfortunately, this takes about 1,9s as well, which is largely due toworkdirnow actually being faster there. The overhead of doing theignore_large_files_in_diffs()call is significant, and it eats up all wins.However, this is the basis
gixto come in and do a status much faster, which is when everything will be consistently faster once again.It's notable though that technically,
diff::workdiris now 25% slower at least, and all that so it won't add everything to the worktree.And I am quite aware that there are many locations that call
create_wd_tree()which does exactly that, but I suppose it has to start somewhere.Performance
list_virual_branches()on the GitLab repo takes 1.16s, of which 802ms are spent on the workdir diff.worktree::add(*)is most time-consuming, but it's the dirwalk that really takes time.