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
4 changes: 4 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
24 changes: 14 additions & 10 deletions crates/gitbutler-branch-actions/src/branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ use core::fmt;
use gitbutler_branch::{
Branch as GitButlerBranch, BranchId, BranchIdentity, ReferenceExtGix, Target,
};
use gitbutler_command_context::{CommandContext, GixRepositoryExt};
use gitbutler_command_context::CommandContext;
use gitbutler_diff::DiffByPathMap;
use gitbutler_project::access::WorktreeReadPermission;
use gitbutler_reference::normalize_branch_name;
use gitbutler_repo::{GixRepositoryExt, 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
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ impl BranchManager<'_> {
vb_state.set_branch(branch.clone())?;
}

let wd_tree = self.ctx.repository().get_wd_tree()?;
let wd_tree = self.ctx.repository().create_wd_tree()?;

let branch_tree = repo
.find_tree(branch.tree)
Expand Down
14 changes: 10 additions & 4 deletions crates/gitbutler-branch-actions/src/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,22 @@ 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};

const WORKSPACE_HEAD: &str = "Workspace Head";
const GITBUTLER_INTEGRATION_COMMIT_TITLE: &str = "GitButler Integration Commit";

// Creates and returns a merge commit of all active branch heads.
//
// This is the base against which we diff the working directory to understand
// what files have been modified.
/// Creates and returns a merge commit of all active branch heads.
///
/// This is the base against which we diff the working directory to understand
/// what files have been modified.
///
/// 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.
Comment on lines +27 to +29
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 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.

#[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
41 changes: 29 additions & 12 deletions crates/gitbutler-branch-actions/src/status.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
use std::{collections::HashMap, path::PathBuf, vec};

use crate::integration::get_workspace_head;
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 +17,7 @@ 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 tracing::instrument;

/// Represents the uncommitted status of the applied virtual branches in the workspace.
pub struct VirtualBranchesStatus {
Expand All @@ -27,21 +27,38 @@ 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)?;
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(|| {
// TODO(ST): Ideally, we can avoid calling `get_workspace_head()` as everyone who modifies
// any of its inputs will update the intragration commit right away.
// It's for another day though - right now the integration commit may be slightly stale.
let integration_commit_id = get_workspace_head(ctx)?;
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
Loading
Loading