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

list virtual branches performance #4771

Merged
merged 11 commits into from
Aug 29, 2024
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,8 @@ codegen-units = 1 # Compile crates one after another so the compiler can optimiz
lto = true # Enables link to optimizations
opt-level = "s" # Optimize for binary size
debug = true # Enable debug symbols, for profiling

[profile.bench]
codegen-units = 256
lto = false
opt-level = 3
23 changes: 23 additions & 0 deletions DEVELOPMENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,29 @@ The app writes logs into:
1. `stdout` in development mode
2. The Tauri [logs](https://tauri.app/v1/api/js/path/#platform-specific) directory

One can get performance log when launching the application locally as follows:

```bash
GITBUTLER_PERFORMANCE_LOG=1 LOG_LEVEL=debug pnpm tauri dev
```

For more realistic performance logging, use local release builds with `--release`.

```bash
GITBUTLER_PERFORMANCE_LOG=1 LOG_LEVEL=debug pnpm tauri dev --release
```

Since release builds are configured for public releases, they are very slow to compile.
Speed them up by sourcing the following file.

```bash
export CARGO_PROFILE_RELEASE_DEBUG=0
export CARGO_PROFILE_RELEASE_INCREMENTAL=false
export CARGO_PROFILE_RELEASE_LTO=false
export CARGO_PROFILE_RELEASE_CODEGEN_UNITS=256
export CARGO_PROFILE_RELEASE_OPT_LEVEL=2
```

### Tokio

We are also collecting tokio's runtime tracing information that could be viewed using [tokio-console](https://github.com/tokio-rs/console#tokio-console-prototypes):
Expand Down
55 changes: 40 additions & 15 deletions crates/gitbutler-branch-actions/src/actions.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
use super::r#virtual as branch;
use crate::branch::get_uncommited_files_raw;
use crate::{
base::{
get_base_branch_data, set_base_branch, set_target_push_remote, update_base_branch,
BaseBranch,
},
branch::get_uncommited_files,
branch_manager::BranchManagerExt,
file::RemoteBranchFile,
remote::{get_branch_data, list_remote_branches, RemoteBranch, RemoteBranchData},
VirtualBranchesExt,
};
use anyhow::{Context, Result};
use gitbutler_branch::{
BranchCreateRequest, BranchId, BranchOwnershipClaims, BranchUpdateRequest, ChangeReference,
};
use gitbutler_command_context::CommandContext;
use gitbutler_diff::DiffByPathMap;
use gitbutler_operating_modes::assure_open_workspace_mode;
use gitbutler_oplog::{
entry::{OperationKind, SnapshotDetails},
Expand All @@ -13,19 +27,6 @@ use gitbutler_reference::{ReferenceName, Refname, RemoteRefname};
use gitbutler_repo::{credentials::Helper, RepoActionsExt, RepositoryExt};
use tracing::instrument;

use super::r#virtual as branch;
use crate::{
base::{
get_base_branch_data, set_base_branch, set_target_push_remote, update_base_branch,
BaseBranch,
},
branch::get_uncommited_files,
branch_manager::BranchManagerExt,
file::RemoteBranchFile,
remote::{get_branch_data, list_remote_branches, RemoteBranch, RemoteBranchData},
VirtualBranchesExt,
};

#[derive(Clone, Copy, Default)]
pub struct VirtualBranchActions;

Expand Down Expand Up @@ -81,6 +82,24 @@ impl VirtualBranchActions {
.map_err(Into::into)
}

pub fn list_virtual_branches_cached(
&self,
project: &Project,
worktree_changes: Option<DiffByPathMap>,
) -> Result<(Vec<branch::VirtualBranch>, Vec<gitbutler_diff::FileDiff>)> {
let ctx = open_with_verify(project)?;

assure_open_workspace_mode(&ctx)
.context("Listing virtual branches requires open workspace mode")?;

branch::list_virtual_branches_cached(
&ctx,
project.exclusive_worktree_access().write_permission(),
worktree_changes,
)
.map_err(Into::into)
}

pub fn create_virtual_branch(
&self,
project: &Project,
Expand Down Expand Up @@ -569,11 +588,17 @@ impl VirtualBranchActions {

pub fn get_uncommited_files(&self, project: &Project) -> Result<Vec<RemoteBranchFile>> {
let context = CommandContext::open(project)?;

let guard = project.exclusive_worktree_access();

get_uncommited_files(&context, guard.read_permission())
}

/// Like [`get_uncommited_files()`], but returns a type that can be re-used with
/// [`crate::list_virtual_branches()`].
pub fn get_uncommited_files_reusable(&self, project: &Project) -> Result<DiffByPathMap> {
let context = CommandContext::open(project)?;
let guard = project.exclusive_worktree_access();
get_uncommited_files_raw(&context, guard.read_permission())
}
}

fn open_with_verify(project: &Project) -> Result<CommandContext> {
Expand Down
22 changes: 13 additions & 9 deletions crates/gitbutler-branch-actions/src/branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ use gitbutler_branch::{
Branch as GitButlerBranch, BranchId, BranchIdentity, ReferenceExtGix, Target,
};
use gitbutler_command_context::{CommandContext, GixRepositoryExt};
use gitbutler_diff::DiffByPathMap;
use gitbutler_project::access::WorktreeReadPermission;
use gitbutler_reference::normalize_branch_name;
use gitbutler_repo::RepositoryExt;
use gitbutler_serde::BStringForFrontend;
use gix::object::tree::diff::Action;
use gix::prelude::ObjectIdExt;
Expand All @@ -22,19 +24,21 @@ use std::{
vec,
};

pub(crate) fn get_uncommited_files_raw(
context: &CommandContext,
_permission: &WorktreeReadPermission,
) -> Result<DiffByPathMap> {
let repository = context.repository();
let head_commit = repository.head_commit()?;
gitbutler_diff::workdir(repository, &head_commit.id())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

.context("Failed to list uncommited files")
}

pub(crate) fn get_uncommited_files(
context: &CommandContext,
_permission: &WorktreeReadPermission,
) -> Result<Vec<RemoteBranchFile>> {
let repository = context.repository();
let head_commit = repository
.head()
.context("Failed to get head")?
.peel_to_commit()
.context("Failed to get head commit")?;

let files = gitbutler_diff::workdir(repository, &head_commit.id())
.context("Failed to list uncommited files")?
let files = get_uncommited_files_raw(context, _permission)?
.into_iter()
.map(|(path, file)| {
let binary = file.hunks.iter().any(|h| h.binary);
Expand Down
2 changes: 2 additions & 0 deletions crates/gitbutler-branch-actions/src/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use gitbutler_commit::commit_ext::CommitExt;
use gitbutler_error::error::Marker;
use gitbutler_project::access::WorktreeWritePermission;
use gitbutler_repo::{LogUntil, RepoActionsExt, RepositoryExt};
use tracing::instrument;

use crate::{branch_manager::BranchManagerExt, conflicts, VirtualBranchesExt};

Expand All @@ -22,6 +23,7 @@ const GITBUTLER_INTEGRATION_COMMIT_TITLE: &str = "GitButler Integration Commit";
//
// This is the base against which we diff the working directory to understand
// what files have been modified.
#[instrument(level = tracing::Level::DEBUG, skip(ctx))]
pub(crate) fn get_workspace_head(ctx: &CommandContext) -> Result<git2::Oid> {
let vb_state = ctx.project().virtual_branches();
let target = vb_state
Expand Down
42 changes: 30 additions & 12 deletions crates/gitbutler-branch-actions/src/status.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
use std::{collections::HashMap, path::PathBuf, vec};

use crate::{
conflicts::RepoConflictsExt,
file::{virtual_hunks_into_virtual_files, VirtualBranchFile},
hunk::{file_hunks_from_diffs, HunkLock, VirtualBranchHunk},
BranchManagerExt, VirtualBranchesExt,
};
use anyhow::{bail, Context, Result};
use git2::Tree;
use gitbutler_branch::{
Expand All @@ -10,14 +16,8 @@ use gitbutler_command_context::CommandContext;
use gitbutler_diff::{diff_files_into_hunks, GitHunk, Hunk, HunkHash};
use gitbutler_operating_modes::assure_open_workspace_mode;
use gitbutler_project::access::WorktreeWritePermission;

use crate::{
conflicts::RepoConflictsExt,
file::{virtual_hunks_into_virtual_files, VirtualBranchFile},
hunk::{file_hunks_from_diffs, HunkLock, VirtualBranchHunk},
integration::get_workspace_head,
BranchManagerExt, VirtualBranchesExt,
};
use gitbutler_repo::RepositoryExt;
use tracing::instrument;

/// Represents the uncommitted status of the applied virtual branches in the workspace.
pub struct VirtualBranchesStatus {
Expand All @@ -27,21 +27,39 @@ pub struct VirtualBranchesStatus {
pub skipped_files: Vec<gitbutler_diff::FileDiff>,
}

pub fn get_applied_status(
ctx: &CommandContext,
perm: Option<&mut WorktreeWritePermission>,
) -> Result<VirtualBranchesStatus> {
get_applied_status_cached(ctx, perm, None)
}

/// Returns branches and their associated file changes, in addition to a list
/// of skipped files.
/// `worktree_changes` are all changed files against the current `HEAD^{tree}` and index
/// against the current working tree directory, and it's used to avoid double-computing
/// this expensive information.
// TODO(kv): make this side effect free
pub fn get_applied_status(
#[instrument(level = tracing::Level::DEBUG, skip(ctx, perm, worktree_changes))]
pub fn get_applied_status_cached(
ctx: &CommandContext,
perm: Option<&mut WorktreeWritePermission>,
worktree_changes: Option<gitbutler_diff::DiffByPathMap>,
) -> Result<VirtualBranchesStatus> {
assure_open_workspace_mode(ctx).context("ng applied status requires open workspace mode")?;
let integration_commit = get_workspace_head(ctx)?;
// TODO(ST): this was `get_workspace_head()`, which is slow and ideally, we don't dynamically
// calculate which should already be 'fixed' - why do we have the integration branch
// if we can't assume it's in the right state? So ideally, we assure that the code
// that affects the integration branch also updates it?
let integration_commit_id = ctx.repository().head_commit()?.id();
Byron marked this conversation as resolved.
Show resolved Hide resolved
let mut virtual_branches = ctx
.project()
.virtual_branches()
.list_branches_in_workspace()?;
let base_file_diffs = gitbutler_diff::workdir(ctx.repository(), &integration_commit.to_owned())
.context("failed to diff workdir")?;
let base_file_diffs = worktree_changes.map(Ok).unwrap_or_else(|| {
gitbutler_diff::workdir(ctx.repository(), &integration_commit_id.to_owned())
.context("failed to diff workdir")
})?;

let mut skipped_files: Vec<gitbutler_diff::FileDiff> = Vec::new();
for file_diff in base_file_diffs.values() {
Expand Down
39 changes: 25 additions & 14 deletions crates/gitbutler-branch-actions/src/virtual.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ use std::{
vec,
};

use crate::{
branch_manager::BranchManagerExt,
commit::{commit_to_vbranch_commit, VirtualBranchCommit},
conflicts::{self, RepoConflictsExt},
file::VirtualBranchFile,
hunk::VirtualBranchHunk,
integration::get_workspace_head,
remote::{branch_to_remote_branch, RemoteBranch},
status::{get_applied_status, get_applied_status_cached},
Get, VirtualBranchesExt,
};
use anyhow::{anyhow, bail, Context, Result};
use bstr::ByteSlice;
use git2_hooks::HookResult;
Expand All @@ -27,18 +38,7 @@ use gitbutler_repo::{
};
use gitbutler_time::time::now_since_unix_epoch_ms;
use serde::Serialize;

use crate::{
branch_manager::BranchManagerExt,
commit::{commit_to_vbranch_commit, VirtualBranchCommit},
conflicts::{self, RepoConflictsExt},
file::VirtualBranchFile,
hunk::VirtualBranchHunk,
integration::get_workspace_head,
remote::{branch_to_remote_branch, RemoteBranch},
status::get_applied_status,
Get, VirtualBranchesExt,
};
use tracing::instrument;

// this struct is a mapping to the view `Branch` type in Typescript
// found in src-tauri/src/routes/repo/[project_id]/types.ts
Expand Down Expand Up @@ -254,12 +254,23 @@ fn resolve_old_applied_state(

Ok(())
}

pub fn list_virtual_branches(
ctx: &CommandContext,
perm: &mut WorktreeWritePermission,
) -> Result<(Vec<VirtualBranch>, Vec<gitbutler_diff::FileDiff>)> {
list_virtual_branches_cached(ctx, perm, None)
}

/// `worktree_changes` are all changed files against the current `HEAD^{tree}` and index
/// against the current working tree directory, and it's used to avoid double-computing
/// this expensive information.
#[instrument(level = tracing::Level::DEBUG, skip(ctx, perm, worktree_changes))]
pub fn list_virtual_branches_cached(
ctx: &CommandContext,
// TODO(ST): this should really only shared access, but there is some internals
// that conditionally write things.
perm: &mut WorktreeWritePermission,
worktree_changes: Option<gitbutler_diff::DiffByPathMap>,
) -> Result<(Vec<VirtualBranch>, Vec<gitbutler_diff::FileDiff>)> {
assure_open_workspace_mode(ctx)
.context("Listing virtual branches requires open workspace mode")?;
Expand All @@ -273,7 +284,7 @@ pub fn list_virtual_branches(
.get_default_target()
.context("failed to get default target")?;

let status = get_applied_status(ctx, Some(perm))?;
let status = get_applied_status_cached(ctx, Some(perm), worktree_changes)?;
let max_selected_for_changes = status
.branches
.iter()
Expand Down
11 changes: 9 additions & 2 deletions crates/gitbutler-branch-actions/tests/extra/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ fn commit_id_can_be_generated_or_specified() -> Result<()> {
#[test]
fn merge_vbranch_upstream_clean_rebase() -> Result<()> {
let suite = Suite::default();
let Case { ctx, project, .. } = &suite.new_case();
let Case { ctx, project, .. } = &mut suite.new_case();

// create a commit and set the target
let file_path = Path::new("test.txt");
Expand Down Expand Up @@ -829,8 +829,14 @@ fn merge_vbranch_upstream_clean_rebase() -> Result<()> {

// create the branch
let (branches, _) = list_virtual_branches(ctx, guard.write_permission())?;
assert_eq!(branches.len(), 1);
let branch1 = &branches[0];
assert_eq!(branch1.files.len(), 1);
assert_eq!(
branch1.files.len(),
1 + 1,
"'test' (modified compared to index) and 'test2' (untracked).\
This is actually correct when looking at the git repository"
);
assert_eq!(branch1.commits.len(), 1);
// assert_eq!(branch1.upstream.as_ref().unwrap().commits.len(), 1);

Expand Down Expand Up @@ -967,6 +973,7 @@ fn merge_vbranch_upstream_conflict() -> Result<()> {
)?;

// make gb see the conflict resolution
gitbutler_branch_actions::update_gitbutler_integration(&vb_state, ctx)?;
let (branches, _) = list_virtual_branches(ctx, guard.write_permission())?;
assert!(branches[0].conflicted);

Expand Down
Loading
Loading