From 8f942bf647c62ba4de9598ded569daacb125229e Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 30 May 2024 09:37:11 -0700 Subject: [PATCH] Use repository mutex more sparingly. Don't hold it while running git status. (#12489) Previously, each git `Repository` object was held inside of a mutex. This was needed because libgit2's Repository object is (as one would expect) not thread safe. But now, the two longest-running git operations that Zed performs, (`status` and `blame`) do not use libgit2 - they invoke the `git` executable. For these operations, it's not necessary to hold a lock on the repository. In this PR, I've moved our mutex usage so that it only wraps the libgit2 calls, not our `git` subprocess spawns. The main user-facing impact of this is that the UI is much more responsive when initially opening a project with a very large git repository (e.g. `chromium`, `webkit`, `linux`). Release Notes: - Improved Zed's responsiveness when initially opening a project containing a very large git repository. --- crates/editor/src/editor.rs | 2 -- crates/fs/src/fs.rs | 35 +++++++++----------- crates/git/src/repository.rs | 58 ++++++++++++++++++--------------- crates/project/src/project.rs | 15 +++------ crates/vcs_menu/src/lib.rs | 5 +-- crates/worktree/src/worktree.rs | 33 ++++++++++--------- 6 files changed, 69 insertions(+), 79 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 2e2db78021..da0b381f4c 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -9998,11 +9998,9 @@ impl Editor { const REMOTE_NAME: &str = "origin"; let origin_url = repo - .lock() .remote_url(REMOTE_NAME) .ok_or_else(|| anyhow!("remote \"{REMOTE_NAME}\" not found"))?; let sha = repo - .lock() .head_sha() .ok_or_else(|| anyhow!("failed to read HEAD SHA"))?; diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index bf66f2b0c5..7bfefd695d 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -12,19 +12,13 @@ use std::os::unix::fs::MetadataExt; use async_tar::Archive; use futures::{future::BoxFuture, AsyncRead, Stream, StreamExt}; use git::repository::{GitRepository, RealGitRepository}; -use git2::Repository as LibGitRepository; -use parking_lot::Mutex; use rope::Rope; - -#[cfg(any(test, feature = "test-support"))] -use smol::io::AsyncReadExt; use smol::io::AsyncWriteExt; -use std::io::Write; -use std::sync::Arc; use std::{ - io, + io::{self, Write}, path::{Component, Path, PathBuf}, pin::Pin, + sync::Arc, time::{Duration, SystemTime}, }; use tempfile::{NamedTempFile, TempDir}; @@ -36,6 +30,10 @@ use collections::{btree_map, BTreeMap}; #[cfg(any(test, feature = "test-support"))] use git::repository::{FakeGitRepositoryState, GitFileStatus}; #[cfg(any(test, feature = "test-support"))] +use parking_lot::Mutex; +#[cfg(any(test, feature = "test-support"))] +use smol::io::AsyncReadExt; +#[cfg(any(test, feature = "test-support"))] use std::ffi::OsStr; #[async_trait::async_trait] @@ -83,7 +81,7 @@ pub trait Fs: Send + Sync { latency: Duration, ) -> Pin>>>; - fn open_repo(&self, abs_dot_git: &Path) -> Option>>; + fn open_repo(&self, abs_dot_git: &Path) -> Option>; fn is_fake(&self) -> bool; async fn is_case_sensitive(&self) -> Result; #[cfg(any(test, feature = "test-support"))] @@ -506,16 +504,13 @@ impl Fs for RealFs { }))) } - fn open_repo(&self, dotgit_path: &Path) -> Option>> { - LibGitRepository::open(dotgit_path) - .log_err() - .map::>, _>(|libgit_repository| { - Arc::new(Mutex::new(RealGitRepository::new( - libgit_repository, - self.git_binary_path.clone(), - self.git_hosting_provider_registry.clone(), - ))) - }) + fn open_repo(&self, dotgit_path: &Path) -> Option> { + let repo = git2::Repository::open(dotgit_path).log_err()?; + Some(Arc::new(RealGitRepository::new( + repo, + self.git_binary_path.clone(), + self.git_hosting_provider_registry.clone(), + ))) } fn is_fake(&self) -> bool { @@ -1489,7 +1484,7 @@ impl Fs for FakeFs { })) } - fn open_repo(&self, abs_dot_git: &Path) -> Option>> { + fn open_repo(&self, abs_dot_git: &Path) -> Option> { let state = self.state.lock(); let entry = state.read_path(abs_dot_git).unwrap(); let mut entry = entry.lock(); diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 9221176b02..fc598e0c9d 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -14,8 +14,6 @@ use std::{ use sum_tree::MapSeekTarget; use util::ResultExt; -pub use git2::Repository as LibGitRepository; - #[derive(Clone, Debug, Hash, PartialEq)] pub struct Branch { pub is_head: bool, @@ -24,7 +22,7 @@ pub struct Branch { pub unix_timestamp: Option, } -pub trait GitRepository: Send { +pub trait GitRepository: Send + Sync { fn reload_index(&self); /// Loads a git repository entry's contents. @@ -58,19 +56,19 @@ impl std::fmt::Debug for dyn GitRepository { } pub struct RealGitRepository { - pub repository: LibGitRepository, + pub repository: Mutex, pub git_binary_path: PathBuf, hosting_provider_registry: Arc, } impl RealGitRepository { pub fn new( - repository: LibGitRepository, + repository: git2::Repository, git_binary_path: Option, hosting_provider_registry: Arc, ) -> Self { Self { - repository, + repository: Mutex::new(repository), git_binary_path: git_binary_path.unwrap_or_else(|| PathBuf::from("git")), hosting_provider_registry, } @@ -79,13 +77,13 @@ impl RealGitRepository { impl GitRepository for RealGitRepository { fn reload_index(&self) { - if let Ok(mut index) = self.repository.index() { + if let Ok(mut index) = self.repository.lock().index() { _ = index.read(false); } } fn load_index_text(&self, relative_file_path: &Path) -> Option { - fn logic(repo: &LibGitRepository, relative_file_path: &Path) -> Result> { + fn logic(repo: &git2::Repository, relative_file_path: &Path) -> Result> { const STAGE_NORMAL: i32 = 0; let index = repo.index()?; @@ -101,7 +99,7 @@ impl GitRepository for RealGitRepository { Ok(Some(String::from_utf8(content)?)) } - match logic(&self.repository, relative_file_path) { + match logic(&self.repository.lock(), relative_file_path) { Ok(value) => return value, Err(err) => log::error!("Error loading head text: {:?}", err), } @@ -109,31 +107,35 @@ impl GitRepository for RealGitRepository { } fn remote_url(&self, name: &str) -> Option { - let remote = self.repository.find_remote(name).ok()?; + let repo = self.repository.lock(); + let remote = repo.find_remote(name).ok()?; remote.url().map(|url| url.to_string()) } fn branch_name(&self) -> Option { - let head = self.repository.head().log_err()?; + let repo = self.repository.lock(); + let head = repo.head().log_err()?; let branch = String::from_utf8_lossy(head.shorthand_bytes()); Some(branch.to_string()) } fn head_sha(&self) -> Option { - let head = self.repository.head().ok()?; - head.target().map(|oid| oid.to_string()) + Some(self.repository.lock().head().ok()?.target()?.to_string()) } fn statuses(&self, path_prefix: &Path) -> Result { let working_directory = self .repository + .lock() .workdir() - .context("failed to read git work directory")?; - GitStatus::new(&self.git_binary_path, working_directory, path_prefix) + .context("failed to read git work directory")? + .to_path_buf(); + GitStatus::new(&self.git_binary_path, &working_directory, path_prefix) } fn branches(&self) -> Result> { - let local_branches = self.repository.branches(Some(BranchType::Local))?; + let repo = self.repository.lock(); + let local_branches = repo.branches(Some(BranchType::Local))?; let valid_branches = local_branches .filter_map(|branch| { branch.ok().and_then(|(branch, _)| { @@ -158,36 +160,40 @@ impl GitRepository for RealGitRepository { } fn change_branch(&self, name: &str) -> Result<()> { - let revision = self.repository.find_branch(name, BranchType::Local)?; + let repo = self.repository.lock(); + let revision = repo.find_branch(name, BranchType::Local)?; let revision = revision.get(); let as_tree = revision.peel_to_tree()?; - self.repository.checkout_tree(as_tree.as_object(), None)?; - self.repository.set_head( + repo.checkout_tree(as_tree.as_object(), None)?; + repo.set_head( revision .name() .ok_or_else(|| anyhow::anyhow!("Branch name could not be retrieved"))?, )?; Ok(()) } - fn create_branch(&self, name: &str) -> Result<()> { - let current_commit = self.repository.head()?.peel_to_commit()?; - self.repository.branch(name, ¤t_commit, false)?; + fn create_branch(&self, name: &str) -> Result<()> { + let repo = self.repository.lock(); + let current_commit = repo.head()?.peel_to_commit()?; + repo.branch(name, ¤t_commit, false)?; Ok(()) } fn blame(&self, path: &Path, content: Rope) -> Result { let working_directory = self .repository + .lock() .workdir() - .with_context(|| format!("failed to get git working directory for file {:?}", path))?; + .with_context(|| format!("failed to get git working directory for file {:?}", path))? + .to_path_buf(); const REMOTE_NAME: &str = "origin"; let remote_url = self.remote_url(REMOTE_NAME); crate::blame::Blame::for_path( &self.git_binary_path, - working_directory, + &working_directory, path, &content, remote_url, @@ -210,8 +216,8 @@ pub struct FakeGitRepositoryState { } impl FakeGitRepository { - pub fn open(state: Arc>) -> Arc> { - Arc::new(Mutex::new(FakeGitRepository { state })) + pub fn open(state: Arc>) -> Arc { + Arc::new(FakeGitRepository { state }) } } diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 72abe6c635..3de4567265 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -7856,10 +7856,7 @@ impl Project { None } else { let relative_path = repo.relativize(&snapshot, &path).ok()?; - local_repo_entry - .repo() - .lock() - .load_index_text(&relative_path) + local_repo_entry.repo().load_index_text(&relative_path) }; Some((buffer, base_text)) } @@ -8194,7 +8191,7 @@ impl Project { &self, project_path: &ProjectPath, cx: &AppContext, - ) -> Option>> { + ) -> Option> { self.worktree_for_id(project_path.worktree_id, cx)? .read(cx) .as_local()? @@ -8202,10 +8199,7 @@ impl Project { .local_git_repo(&project_path.path) } - pub fn get_first_worktree_root_repo( - &self, - cx: &AppContext, - ) -> Option>> { + pub fn get_first_worktree_root_repo(&self, cx: &AppContext) -> Option> { let worktree = self.visible_worktrees(cx).next()?.read(cx).as_local()?; let root_entry = worktree.root_git_entry()?; @@ -8255,8 +8249,7 @@ impl Project { cx.background_executor().spawn(async move { let (repo, relative_path, content) = blame_params?; - let lock = repo.lock(); - lock.blame(&relative_path, content) + repo.blame(&relative_path, content) .with_context(|| format!("Failed to blame {:?}", relative_path.0)) }) } else { diff --git a/crates/vcs_menu/src/lib.rs b/crates/vcs_menu/src/lib.rs index a29123eb31..5f06305a74 100644 --- a/crates/vcs_menu/src/lib.rs +++ b/crates/vcs_menu/src/lib.rs @@ -109,7 +109,7 @@ impl BranchListDelegate { .get_first_worktree_root_repo(cx) .context("failed to get root repository for first worktree")?; - let all_branches = repo.lock().branches()?; + let all_branches = repo.branches()?; Ok(Self { matches: vec![], workspace: handle, @@ -237,7 +237,6 @@ impl PickerDelegate for BranchListDelegate { .get_first_worktree_root_repo(cx) .context("failed to get root repository for first worktree")?; let status = repo - .lock() .change_branch(¤t_pick); if status.is_err() { this.delegate.display_error_toast(format!("Failed to checkout branch '{current_pick}', check for conflicts or unstashed files"), cx); @@ -316,8 +315,6 @@ impl PickerDelegate for BranchListDelegate { let repo = project .get_first_worktree_root_repo(cx) .context("failed to get root repository for first worktree")?; - let repo = repo - .lock(); let status = repo .create_branch(¤t_pick); if status.is_err() { diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index bbc7430f9f..2a0336cbba 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -311,14 +311,14 @@ struct BackgroundScannerState { #[derive(Debug, Clone)] pub struct LocalRepositoryEntry { pub(crate) git_dir_scan_id: usize, - pub(crate) repo_ptr: Arc>, + pub(crate) repo_ptr: Arc, /// Path to the actual .git folder. /// Note: if .git is a file, this points to the folder indicated by the .git file pub(crate) git_dir_path: Arc, } impl LocalRepositoryEntry { - pub fn repo(&self) -> &Arc> { + pub fn repo(&self) -> &Arc { &self.repo_ptr } } @@ -1145,7 +1145,7 @@ impl LocalWorktree { if abs_path_metadata.is_dir || abs_path_metadata.is_symlink { None } else { - git_repo.lock().load_index_text(&repo_path) + git_repo.load_index_text(&repo_path) } } })); @@ -2236,7 +2236,7 @@ impl LocalSnapshot { Some((repo_entry, self.git_repositories.get(&work_directory_id)?)) } - pub fn local_git_repo(&self, path: &Path) -> Option>> { + pub fn local_git_repo(&self, path: &Path) -> Option> { self.repo_for_path(path) .map(|(_, entry)| entry.repo_ptr.clone()) } @@ -2556,7 +2556,6 @@ impl BackgroundScannerState { work_directory: workdir_path, statuses: repo .repo_ptr - .lock() .statuses(&repo_path) .log_err() .unwrap_or_default(), @@ -2704,7 +2703,7 @@ impl BackgroundScannerState { &mut self, dot_git_path: Arc, fs: &dyn Fs, - ) -> Option<(RepositoryWorkDirectory, Arc>)> { + ) -> Option<(RepositoryWorkDirectory, Arc)> { let work_dir_path: Arc = match dot_git_path.parent() { Some(parent_dir) => { // Guard against repositories inside the repository metadata @@ -2739,7 +2738,7 @@ impl BackgroundScannerState { dot_git_path: Arc, location_in_repo: Option>, fs: &dyn Fs, - ) -> Option<(RepositoryWorkDirectory, Arc>)> { + ) -> Option<(RepositoryWorkDirectory, Arc)> { let work_dir_id = self .snapshot .entry_for_path(work_dir_path.clone()) @@ -2750,14 +2749,16 @@ impl BackgroundScannerState { } let abs_path = self.snapshot.abs_path.join(&dot_git_path); + let t0 = Instant::now(); let repository = fs.open_repo(&abs_path)?; + log::trace!("constructed libgit2 repo in {:?}", t0.elapsed()); let work_directory = RepositoryWorkDirectory(work_dir_path.clone()); self.snapshot.repository_entries.insert( work_directory.clone(), RepositoryEntry { work_directory: work_dir_id.into(), - branch: repository.lock().branch_name().map(Into::into), + branch: repository.branch_name().map(Into::into), location_in_repo, }, ); @@ -3827,16 +3828,17 @@ impl BackgroundScanner { let child_path: Arc = job.path.join(child_name).into(); if child_name == *DOT_GIT { - if let Some((work_directory, repository)) = self + let repo = self .state .lock() - .build_git_repository(child_path.clone(), self.fs.as_ref()) - { + .build_git_repository(child_path.clone(), self.fs.as_ref()); + if let Some((work_directory, repository)) = repo { + let t0 = Instant::now(); let statuses = repository - .lock() .statuses(Path::new("")) .log_err() .unwrap_or_default(); + log::trace!("computed git status in {:?}", t0.elapsed()); containing_repository = Some(ScanJobContainingRepository { work_directory, statuses, @@ -4077,7 +4079,7 @@ impl BackgroundScanner { if !is_dir && !fs_entry.is_ignored && !fs_entry.is_external { if let Some((repo_entry, repo)) = state.snapshot.repo_for_path(path) { if let Ok(repo_path) = repo_entry.relativize(&state.snapshot, path) { - fs_entry.git_status = repo.repo_ptr.lock().status(&repo_path); + fs_entry.git_status = repo.repo_ptr.status(&repo_path); } } } @@ -4263,7 +4265,7 @@ impl BackgroundScanner { if !entry.is_dir() && !entry.is_ignored && !entry.is_external { if let Some((ref repo_entry, local_repo)) = repo { if let Ok(repo_path) = repo_entry.relativize(&snapshot, &entry.path) { - entry.git_status = local_repo.repo_ptr.lock().status(&repo_path); + entry.git_status = local_repo.repo_ptr.status(&repo_path); } } } @@ -4326,7 +4328,7 @@ impl BackgroundScanner { }; log::info!("reload git repository {dot_git_dir:?}"); - let repo = repository.repo_ptr.lock(); + let repo = &repository.repo_ptr; let branch = repo.branch_name(); repo.reload_index(); @@ -4344,7 +4346,6 @@ impl BackgroundScanner { }; let statuses = repository - .lock() .statuses(Path::new("")) .log_err() .unwrap_or_default();