Skip to content
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.

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

performance improvements and bug fixes #4793

Merged
merged 6 commits into from
Aug 30, 2024
Merged

Conversation

Byron
Copy link
Collaborator

@Byron Byron commented Aug 29, 2024

This PR is planned as follow-up to #4771, even though it chooses to pick other low-hanging fruit first.

This means, adding more instrumentation to find long-running functions or functions that run more than once in the same operation.
From there, one can avoid duplication, and optimize specific cases while keeping git2, at least at first.

Probably this will lead to this PR being merged early as it aims to deliver more small improvements while guiding the way towards
bigger improvements that would require gix, along with some upgrades to it which may take more time.

Tasks

Notes for the reviewer

  • It's not actually a perfect fix, as I saw this while testing:
    Screenshot 2024-08-29 at 22 50 43
    And actually, it turned out that this is a similar looking, but different, commit, so it's correct after all.
  • After unapplying, I saw that there are two duplicate commits which seem exactly the same (but have different patches). It was strange that the UI didn't show them combined anymore, maybe intentionally? Or maybe it's another implementation? It seems like it, so I didn't touch it.
    Screenshot 2024-08-29 at 22 52 34

Follow-Up

  • accelerate is_commit_integrated() - octopus merge with trees as results
    Screenshot 2024-08-29 at 18 48 46

Notes

  • When applying a branch in GitLab, it runs into the write-lock and operations block each other, with the last recalculation taking 20.4s (most of the time is waiting). Overall, this cascade of events seems very suboptimal.
    • I feel a lot of compute could be saved if the operation itself would emit whatever the UI needs, instead of relying on the watcher. The watcher could be deactivated/told to ignore what happens while the alteration is running. However, this also seems very complicated so it's probably better for now to make everything faster.
  • It seems like list_local_branches() (formerly list_remote_branches()) is used to build a combined branches view, similar what the new list_branches does. It's hard for me to see how it's used, and it am surprised this information is still used anywhere as the side-bar definitely uses the new branch_listing now.
  • It's clear that my tests of creating a vbranch from a local branch cause particularly bad performance when listing virtual branches due to their integration-commit checks.

Issues

These issues where created while processing this PR.

References

Copy link

vercel bot commented Aug 29, 2024

@Byron is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the rust Pull requests that update Rust code label Aug 29, 2024
@Byron Byron mentioned this pull request Aug 29, 2024
8 tasks
@krlvi
Copy link
Member

krlvi commented Aug 29, 2024

the list_remote_branches function is meant to be deprecated / removed entirely in favor of the new listing api that was already optimized. I think ideally we should figure out if there is anything this endpoint provides that is still needed and shift it somewhere else

@Byron Byron force-pushed the git2-to-gix branch 2 times, most recently from c39a948 to 81bd7ef Compare August 29, 2024 21:09
@Byron Byron marked this pull request as ready for review August 29, 2024 21:09
@Byron Byron requested review from krlvi and mtsgrd August 29, 2024 21:09
@Byron Byron changed the title Various performance improvements performance improvements and bug fixes Aug 29, 2024
Byron added 6 commits August 30, 2024 07:15
…iles.

The reason for this is that now the outcome of this can be re-used in `list_virtual_branches`,
which is sensitive to seeing the latest vbranch state which is not a given.

Calculating it makes sure there is no mismatch, and probably "can't be wrong".

Also note that `head_commit()` is still kept around as it's the idea to eventually
use it more.
Usually this makes the affected code simpler.
This is relevant when all commits are equal by tree, but seem changed
due to the added GitButler headers.

For added safety, we also compare by commit message, date and authors,
basically everything that isn't the headers.
@krlvi krlvi merged commit 2c7773a into gitbutlerapp:master Aug 30, 2024
16 of 17 checks passed
@mtsgrd
Copy link
Contributor

mtsgrd commented Aug 31, 2024

@Byron this seems to break amending of commits. If I have a virtual branch with an upstream and I amend the only commit it stays local and remote instead of becoming local (which can be force pushed).

@Byron
Copy link
Collaborator Author

Byron commented Aug 31, 2024

Thanks for letting me know! I think this means that a patch-id also has to be computed to detect such changes. And that would make this operation quite expensive even though I think it can easily be accelerated by using all cores, i.e. compute patches in parallel.
I'd be glad to do that if you think that fixing the initial shortcoming, i.e. creating a virtual branch from a remote and having its commits displayed as "Local and remote" instead of separately, is worth it. If not, a revert would be in order.
Please let me know and I will submit a PR with the respective kind of fix.

@mtsgrd
Copy link
Contributor

mtsgrd commented Aug 31, 2024

Hey thanks for responding so quickly. Iiuc we would want to display the list as if the branch was rebased, but it's difficult since the remote id's don't necessarily have change id's?

It feels like checking author, committer, and message is the right way to accomplish this, so I'm thinking the problem is in the front-end code. Instead of identifying the commit existing on both local and remote, it should be identified as local, but having been rebased. Let me see if I can fix this!

@mtsgrd
Copy link
Contributor

mtsgrd commented Sep 1, 2024

See #4807!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@gitbutler/desktop rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating a virtual branch from a local tracking branch that isn't from GitButler duplicates commits
3 participants