From bb368ce1fe974cfef409d08a4415f4c0fd048ab0 Mon Sep 17 00:00:00 2001 From: Remco Smits Date: Sun, 22 Feb 2026 00:05:23 +0100 Subject: [PATCH] git_graph: Improve loading time (#49736) This PR Fixes a loading performance regresssion inside the git graph. The main issue is that we always acted on the received repository events, this is good in 9 out of 10 cases except for the initial loading phase of the git graph. This is because we invalidate the graph data every time we receive a `GitStoreEvent::ActiveRepositoryChanged`, `RepositoryEvent::BranchChanged` or `RepositoryEvent::MergeHeadsChanged` event this still sounds good, but the caveat is that we receive these 3 events on initial repository loading. This happens when you start up Zed and is getting the active repository, branch ect. from your project. When it detects a repository/branch etc. it checks if it has been changed and emits an event for it. This is always the case for initial repository loading, because the active repository/branch always start as **None**. So receive an event for these non actual changes makes the git graph cancel its initial loading and start fetching again on every invalidated graph data call. We fixed this by checking the **scan_id** of the repo to check if the repo has been initialized, if its bigger then 1 we know we need to invalidate the data because it was a actual user change instead of a initial loading event. **Before** (note you see the loading state twice): https://github.com/user-attachments/assets/c25bfae1-0e2f-4c8b-a0d0-926acb33adff **After** (almost instant): https://github.com/user-attachments/assets/7e4ac116-65a2-4eb6-aa4c-37291d6acd0f ----- **Before** (switching repositories shows empty commits pane) https://github.com/user-attachments/assets/71b04285-49e7-47bb-9660-ad53bbf15c46 **After** (switching repositories shows correct graph from the cache) https://github.com/user-attachments/assets/38c33d93-f592-4440-b63b-567fda0fbeb8 Before you mark this PR as ready for review, make sure that you have: - [x] Added a solid test coverage and/or screenshots from doing manual testing - [x] Done a self-review taking into account security and performance aspects - [x] Aligned any UI changes with the [UI checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) Release Notes: - N/A --------- Co-authored-by: Anthony Co-authored-by: Anthony Eid --- crates/git_graph/src/git_graph.rs | 357 ++++++++++++++++++++++++------ crates/project/src/git_store.rs | 6 +- 2 files changed, 291 insertions(+), 72 deletions(-) diff --git a/crates/git_graph/src/git_graph.rs b/crates/git_graph/src/git_graph.rs index 791e0f61117d0e84c86f1849af079a664206fb4b..ad6aa7ec3c6f9a129495e437bd379b023ec7cf61 100644 --- a/crates/git_graph/src/git_graph.rs +++ b/crates/git_graph/src/git_graph.rs @@ -8,15 +8,15 @@ use git::{ use git_ui::{commit_tooltip::CommitAvatar, commit_view::CommitView}; use gpui::{ AnyElement, App, Bounds, ClickEvent, ClipboardItem, Context, Corner, DefiniteLength, - DragMoveEvent, ElementId, Entity, EventEmitter, FocusHandle, Focusable, FontWeight, Hsla, - InteractiveElement, ParentElement, PathBuilder, Pixels, Point, Render, ScrollStrategy, + DragMoveEvent, ElementId, Empty, Entity, EventEmitter, FocusHandle, Focusable, FontWeight, + Hsla, InteractiveElement, ParentElement, PathBuilder, Pixels, Point, Render, ScrollStrategy, ScrollWheelEvent, SharedString, Styled, Subscription, Task, WeakEntity, Window, actions, anchored, deferred, point, px, }; use menu::{SelectNext, SelectPrevious}; use project::{ Project, - git_store::{CommitDataState, GitStoreEvent, Repository, RepositoryEvent}, + git_store::{CommitDataState, GitStoreEvent, Repository, RepositoryEvent, RepositoryId}, }; use settings::Settings; use smallvec::{SmallVec, smallvec}; @@ -541,6 +541,8 @@ impl GraphData { color_idx: commit_color.0 as usize, })); } + + self.max_commit_count = AllCommitCount::Loaded(self.commits.len()); } } @@ -645,8 +647,8 @@ pub struct GitGraph { log_order: LogOrder, selected_commit_diff: Option, _commit_diff_task: Option>, - _load_task: Option>, commit_details_split_state: Entity, + selected_repo_id: Option, } impl GitGraph { @@ -672,36 +674,38 @@ impl GitGraph { let git_store = project.read(cx).git_store().clone(); let accent_colors = cx.theme().accents(); - let mut graph = GraphData::new(accent_colors_count(accent_colors)); + let graph = GraphData::new(accent_colors_count(accent_colors)); let log_source = LogSource::default(); let log_order = LogOrder::default(); cx.subscribe(&git_store, |this, _, event, cx| match event { - GitStoreEvent::RepositoryUpdated(_, repo_event, is_active) => { - if *is_active { - if let Some(repository) = this.project.read(cx).active_repository(cx) { + GitStoreEvent::RepositoryUpdated(updated_repo_id, repo_event, _) => { + if this + .selected_repo_id + .as_ref() + .is_some_and(|repo_id| repo_id == updated_repo_id) + { + if let Some(repository) = this.get_selected_repository(cx) { this.on_repository_event(repository, repo_event, cx); } } } - GitStoreEvent::ActiveRepositoryChanged(_) => { - this.graph_data.clear(); - cx.notify(); + GitStoreEvent::ActiveRepositoryChanged(changed_repo_id) => { + // todo(git_graph): Make this selectable from UI so we don't have to always use active repository + if this.selected_repo_id != *changed_repo_id { + this.selected_repo_id = *changed_repo_id; + this.graph_data.clear(); + cx.notify(); + } } _ => {} }) .detach(); - if let Some(repository) = project.read(cx).active_repository(cx) { - repository.update(cx, |repository, cx| { - // This won't overlap with loading commits from the repository because - // we either have all commits or commits loaded in chunks and loading commits - // from the repository event is always adding the last chunk of commits. - let (commits, _) = - repository.graph_data(log_source.clone(), log_order, 0..usize::MAX, cx); - graph.add_commits(commits); - }); - } + let active_repository = project + .read(cx) + .active_repository(cx) + .map(|repo| repo.read(cx).id); let table_interaction_state = cx.new(|cx| TableInteractionState::new(cx)); let table_column_widths = cx.new(|cx| TableColumnWidths::new(4, cx)); @@ -715,17 +719,16 @@ impl GitGraph { state.scroll_handle.0.borrow_mut().last_item_size = None; }); row_height = new_row_height; + cx.notify(); } - cx.notify(); }) .detach(); - GitGraph { + let mut this = GitGraph { focus_handle, project, workspace, graph_data: graph, - _load_task: None, _commit_diff_task: None, context_menu: None, row_height, @@ -738,7 +741,11 @@ impl GitGraph { log_source, log_order, commit_details_split_state: cx.new(|_cx| SplitState::new()), - } + selected_repo_id: active_repository, + }; + + this.fetch_initial_graph_data(cx); + this } fn on_repository_event( @@ -748,29 +755,52 @@ impl GitGraph { cx: &mut Context, ) { match event { - RepositoryEvent::GitGraphCountUpdated(_, commit_count) => { + RepositoryEvent::GitGraphCountUpdated((order, source), commit_count) => { + if order != &self.log_order || source != &self.log_source { + return; + } + let old_count = self.graph_data.commits.len(); repository.update(cx, |repository, cx| { - let (commits, _) = repository.graph_data( - self.log_source.clone(), - self.log_order, - old_count..*commit_count, - cx, - ); + let (commits, _) = + repository.graph_data(source.clone(), *order, old_count..*commit_count, cx); self.graph_data.add_commits(commits); }); - - self.graph_data.max_commit_count = AllCommitCount::Loaded(*commit_count); - } - RepositoryEvent::BranchChanged => { - self.graph_data.clear(); cx.notify(); } + RepositoryEvent::BranchChanged | RepositoryEvent::MergeHeadsChanged => { + // Only invalidate if we scanned atleast once, + // meaning we are not inside the initial repo loading state + // NOTE: this fixes an loading performance regression + if repository.read(cx).scan_id > 1 { + self.graph_data.clear(); + cx.notify(); + } + } _ => {} } + } - cx.notify(); + fn fetch_initial_graph_data(&mut self, cx: &mut App) { + if let Some(repository) = self.get_selected_repository(cx) { + repository.update(cx, |repository, cx| { + let (commits, _) = repository.graph_data( + self.log_source.clone(), + self.log_order, + 0..usize::MAX, + cx, + ); + self.graph_data.add_commits(commits); + }); + } + } + + fn get_selected_repository(&self, cx: &App) -> Option> { + let project = self.project.read(cx); + self.selected_repo_id + .as_ref() + .and_then(|repo_id| project.repositories(cx).get(&repo_id).cloned()) } fn render_badge(&self, name: &SharedString, accent_color: gpui::Hsla) -> impl IntoElement { @@ -799,9 +829,7 @@ impl GitGraph { _window: &mut Window, cx: &mut Context, ) -> Vec> { - let repository = self - .project - .read_with(cx, |project, cx| project.active_repository(cx)); + let repository = self.get_selected_repository(cx); let row_height = self.row_height; @@ -943,11 +971,8 @@ impl GitGraph { }; let sha = commit.data.sha.to_string(); - let repository = self - .project - .read_with(cx, |project, cx| project.active_repository(cx)); - let Some(repository) = repository else { + let Some(repository) = self.get_selected_repository(cx) else { return; }; @@ -984,11 +1009,7 @@ impl GitGraph { return; }; - let repository = self - .project - .read_with(cx, |project, cx| project.active_repository(cx)); - - let Some(repository) = repository else { + let Some(repository) = self.get_selected_repository(cx) else { return; }; @@ -1034,19 +1055,15 @@ impl GitGraph { cx: &mut Context, ) -> impl IntoElement { let Some(selected_idx) = self.selected_entry_idx else { - return div().into_any_element(); + return Empty.into_any_element(); }; let Some(commit_entry) = self.graph_data.commits.get(selected_idx) else { - return div().into_any_element(); + return Empty.into_any_element(); }; - let repository = self - .project - .read_with(cx, |project, cx| project.active_repository(cx)); - - let Some(repository) = repository else { - return div().into_any_element(); + let Some(repository) = self.get_selected_repository(cx) else { + return Empty.into_any_element(); }; let data = repository.update(cx, |repository, cx| { @@ -1634,24 +1651,28 @@ impl Render for GitGraph { let (commit_count, is_loading) = match self.graph_data.max_commit_count { AllCommitCount::Loaded(count) => (count, true), AllCommitCount::NotLoaded => { - let is_loading = self.project.update(cx, |project, cx| { - if let Some(repository) = project.active_repository(cx) { + let (commit_count, is_loading) = + if let Some(repository) = self.get_selected_repository(cx) { repository.update(cx, |repository, cx| { // Start loading the graph data if we haven't started already - repository - .graph_data(self.log_source.clone(), self.log_order, 0..0, cx) - .1 + let (commits, is_loading) = repository.graph_data( + self.log_source.clone(), + self.log_order, + 0..usize::MAX, + cx, + ); + self.graph_data.add_commits(&commits); + (commits.len(), is_loading) }) } else { - false - } - }) && self.graph_data.commits.is_empty(); + (0, false) + }; - (self.graph_data.commits.len(), is_loading) + (commit_count, is_loading) } }; - let content = if self.graph_data.commits.is_empty() { + let content = if commit_count == 0 { let message = if is_loading { "Loading" } else { @@ -1685,6 +1706,7 @@ impl Render for GitGraph { div() .p_2() .border_b_1() + .whitespace_nowrap() .border_color(cx.theme().colors().border) .child(Label::new("Graph").color(Color::Muted)), ) @@ -1945,12 +1967,14 @@ mod tests { use git::repository::InitialGraphCommitData; use gpui::TestAppContext; use project::Project; + use project::git_store::{GitStoreEvent, RepositoryEvent}; use rand::prelude::*; use serde_json::json; use settings::SettingsStore; use smallvec::{SmallVec, smallvec}; use std::path::Path; - use std::sync::Arc; + use std::sync::{Arc, Mutex}; + use workspace::MultiWorkspace; fn init_test(cx: &mut TestAppContext) { cx.update(|cx| { @@ -1959,6 +1983,14 @@ mod tests { }); } + fn init_test_with_theme(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + theme::init(theme::LoadThemes::JustBase, cx); + }); + } + /// Generates a random commit DAG suitable for testing git graph rendering. /// /// The commits are ordered newest-first (like git log output), so: @@ -2526,7 +2558,7 @@ mod tests { // The full integration test has less iterations because it's significantly slower // than the random commit test - #[gpui::test(iterations = 5)] + #[gpui::test(iterations = 10)] async fn test_git_graph_random_integration(mut rng: StdRng, cx: &mut TestAppContext) { init_test(cx); @@ -2591,4 +2623,189 @@ mod tests { ); } } + + #[gpui::test] + async fn test_initial_graph_data_not_cleared_on_initial_loading(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + Path::new("/project"), + json!({ + ".git": {}, + "file.txt": "content", + }), + ) + .await; + + let mut rng = StdRng::seed_from_u64(42); + let commits = generate_random_commit_dag(&mut rng, 10, false); + fs.set_graph_commits(Path::new("/project/.git"), commits.clone()); + + let project = Project::test(fs.clone(), [Path::new("/project")], cx).await; + let observed_repository_events = Arc::new(Mutex::new(Vec::new())); + project.update(cx, |project, cx| { + let observed_repository_events = observed_repository_events.clone(); + cx.subscribe(project.git_store(), move |_, _, event, _| { + if let GitStoreEvent::RepositoryUpdated(_, repository_event, true) = event { + observed_repository_events + .lock() + .expect("repository event mutex should be available") + .push(repository_event.clone()); + } + }) + .detach(); + }); + + let repository = project.read_with(cx, |project, cx| { + project + .active_repository(cx) + .expect("should have a repository") + }); + + repository.update(cx, |repo, cx| { + repo.graph_data( + crate::LogSource::default(), + crate::LogOrder::default(), + 0..usize::MAX, + cx, + ); + }); + + project + .update(cx, |project, cx| project.git_scans_complete(cx)) + .await; + cx.run_until_parked(); + + let observed_repository_events = observed_repository_events + .lock() + .expect("repository event mutex should be available"); + assert!( + observed_repository_events + .iter() + .any(|event| matches!(event, RepositoryEvent::BranchChanged)), + "initial repository scan should emit BranchChanged" + ); + assert!( + observed_repository_events + .iter() + .any(|event| matches!(event, RepositoryEvent::MergeHeadsChanged)), + "initial repository scan should emit MergeHeadsChanged" + ); + + let graph_data_key = (crate::LogOrder::default(), crate::LogSource::default()); + let commit_count_after = repository.read_with(cx, |repo, _| { + repo.initial_graph_data + .get(&graph_data_key) + .map(|(_, data)| data.len()) + .unwrap_or(0) + }); + assert_eq!( + commits.len(), + commit_count_after, + "initial_graph_data should remain populated after events emitted by initial repository scan" + ); + } + + #[gpui::test] + async fn test_graph_data_repopulated_from_cache_after_repo_switch(cx: &mut TestAppContext) { + init_test_with_theme(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + Path::new("/project_a"), + json!({ + ".git": {}, + "file.txt": "content", + }), + ) + .await; + fs.insert_tree( + Path::new("/project_b"), + json!({ + ".git": {}, + "other.txt": "content", + }), + ) + .await; + + let mut rng = StdRng::seed_from_u64(42); + let commits = generate_random_commit_dag(&mut rng, 10, false); + fs.set_graph_commits(Path::new("/project_a/.git"), commits.clone()); + + let project = Project::test( + fs.clone(), + [Path::new("/project_a"), Path::new("/project_b")], + cx, + ) + .await; + cx.run_until_parked(); + + let (first_repository, second_repository) = project.read_with(cx, |project, cx| { + let mut first_repository = None; + let mut second_repository = None; + + for repository in project.repositories(cx).values() { + let work_directory_abs_path = &repository.read(cx).work_directory_abs_path; + if work_directory_abs_path.as_ref() == Path::new("/project_a") { + first_repository = Some(repository.clone()); + } else if work_directory_abs_path.as_ref() == Path::new("/project_b") { + second_repository = Some(repository.clone()); + } + } + + ( + first_repository.expect("should have repository for /project_a"), + second_repository.expect("should have repository for /project_b"), + ) + }); + first_repository.update(cx, |repository, cx| repository.set_as_active_repository(cx)); + cx.run_until_parked(); + + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + + let workspace_weak = + multi_workspace.read_with(&*cx, |multi, _| multi.workspace().downgrade()); + let git_graph = cx.new_window_entity(|window, cx| { + GitGraph::new(project.clone(), workspace_weak, window, cx) + }); + cx.run_until_parked(); + + // Verify initial graph data is loaded + let initial_commit_count = + git_graph.read_with(&*cx, |graph, _| graph.graph_data.commits.len()); + assert!( + initial_commit_count > 0, + "graph data should have been loaded, got 0 commits" + ); + + second_repository.update(&mut *cx, |repository, cx| { + repository.set_as_active_repository(cx) + }); + cx.run_until_parked(); + + let commit_count_after_clear = + git_graph.read_with(&*cx, |graph, _| graph.graph_data.commits.len()); + assert_eq!( + commit_count_after_clear, 0, + "graph_data should be cleared after switching away" + ); + + first_repository.update(&mut *cx, |repository, cx| { + repository.set_as_active_repository(cx) + }); + + git_graph.update_in(&mut *cx, |this, window, cx| { + this.render(window, cx); + }); + cx.run_until_parked(); + + let commit_count_after_switch_back = + git_graph.read_with(&*cx, |graph, _| graph.graph_data.commits.len()); + assert_eq!( + initial_commit_count, commit_count_after_switch_back, + "graph_data should be repopulated from cache after switching back to the same repo" + ); + } } diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index b9d6f72d532609775ccc8b8d426cf278a390cdc2..1272a689b908413fff5eef71cf5e0e98fd72429b 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -3743,9 +3743,11 @@ impl Repository { }) .shared(); - cx.subscribe_self(|this, event: &RepositoryEvent, _| match event { + cx.subscribe_self(move |this, event: &RepositoryEvent, _| match event { RepositoryEvent::BranchChanged | RepositoryEvent::MergeHeadsChanged => { - this.initial_graph_data.clear(); + if this.scan_id > 1 { + this.initial_graph_data.clear(); + } } _ => {} })