zed-industries/zed
1
1
Fork 0
mirror of https://github.com/zed-industries/zed.git synced 2024-09-16 17:07:14 +03:00
Code Issues Packages Projects Releases Wiki Activity

Use repository mutex more sparingly. Don't hold it while running git status. (#12489)

Browse Source 
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.
This commit is contained in:
Max Brunsfeld 2024-05-30 09:37:11 -07:00 committed by GitHub
parent 1ecd13ba50
commit 8f942bf647
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 69 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
crates/editor/src/editor.rs
View File

@ -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"))?;

35
crates/fs/src/fs.rs
View File

@ -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<Box<dyn Send + Stream<Item = Vec<PathBuf>>>>;
fn open_repo(&self, abs_dot_git: &Path) -> Option<Arc<Mutex<dyn GitRepository>>>;
fn open_repo(&self, abs_dot_git: &Path) -> Option<Arc<dyn GitRepository>>;
fn is_fake(&self) -> bool;
async fn is_case_sensitive(&self) -> Result<bool>;
#[cfg(any(test, feature = "test-support"))]
@ -506,16 +504,13 @@ impl Fs for RealFs {
})))
}
fn open_repo(&self, dotgit_path: &Path) -> Option<Arc<Mutex<dyn GitRepository>>> {
LibGitRepository::open(dotgit_path)
.log_err()
.map::<Arc<Mutex<dyn GitRepository>>, _>(|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<Arc<dyn GitRepository>> {
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<Arc<Mutex<dyn GitRepository>>> {
fn open_repo(&self, abs_dot_git: &Path) -> Option<Arc<dyn GitRepository>> {
let state = self.state.lock();
let entry = state.read_path(abs_dot_git).unwrap();
let mut entry = entry.lock();

58
crates/git/src/repository.rs
View File

@ -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<i64>,
}
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<git2::Repository>,
pub git_binary_path: PathBuf,
hosting_provider_registry: Arc<GitHostingProviderRegistry>,
}
impl RealGitRepository {
pub fn new(
repository: LibGitRepository,
repository: git2::Repository,
git_binary_path: Option<PathBuf>,
hosting_provider_registry: Arc<GitHostingProviderRegistry>,
) -> 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<String> {
fn logic(repo: &LibGitRepository, relative_file_path: &Path) -> Result<Option<String>> {
fn logic(repo: &git2::Repository, relative_file_path: &Path) -> Result<Option<String>> {
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<String> {
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<String> {
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<String> {
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<GitStatus> {
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<Vec<Branch>> {
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, &current_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, &current_commit, false)?;
Ok(())
}
fn blame(&self, path: &Path, content: Rope) -> Result<crate::blame::Blame> {
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<Mutex<FakeGitRepositoryState>>) -> Arc<Mutex<dyn GitRepository>> {
Arc::new(Mutex::new(FakeGitRepository { state }))
pub fn open(state: Arc<Mutex<FakeGitRepositoryState>>) -> Arc<dyn GitRepository> {
Arc::new(FakeGitRepository { state })
}
}

15
crates/project/src/project.rs
View File

@ -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<Arc<Mutex<dyn GitRepository>>> {
) -> Option<Arc<dyn GitRepository>> {
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<Arc<Mutex<dyn GitRepository>>> {
pub fn get_first_worktree_root_repo(&self, cx: &AppContext) -> Option<Arc<dyn GitRepository>> {
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 {

5
crates/vcs_menu/src/lib.rs
View File

@ -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(&current_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(&current_pick);
if status.is_err() {

33
crates/worktree/src/worktree.rs
View File

@ -311,14 +311,14 @@ struct BackgroundScannerState {
#[derive(Debug, Clone)]
pub struct LocalRepositoryEntry {
pub(crate) git_dir_scan_id: usize,
pub(crate) repo_ptr: Arc<Mutex<dyn GitRepository>>,
pub(crate) repo_ptr: Arc<dyn GitRepository>,
/// 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<Path>,
}
impl LocalRepositoryEntry {
pub fn repo(&self) -> &Arc<Mutex<dyn GitRepository>> {
pub fn repo(&self) -> &Arc<dyn GitRepository> {
&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<Arc<Mutex<dyn GitRepository>>> {
pub fn local_git_repo(&self, path: &Path) -> Option<Arc<dyn GitRepository>> {
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<Path>,
fs: &dyn Fs,
) -> Option<(RepositoryWorkDirectory, Arc<Mutex<dyn GitRepository>>)> {
) -> Option<(RepositoryWorkDirectory, Arc<dyn GitRepository>)> {
let work_dir_path: Arc<Path> = 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<Path>,
location_in_repo: Option<Arc<Path>>,
fs: &dyn Fs,
) -> Option<(RepositoryWorkDirectory, Arc<Mutex<dyn GitRepository>>)> {
) -> Option<(RepositoryWorkDirectory, Arc<dyn GitRepository>)> {
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<Path> = 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();