git: Improve diff performance (#54435)

Previously, passing an empty range to `range_to_buffer_range` would
return no ranges, which would trigger a fallback case in the git diff
that caused the entire multibuffer to be invalidated every frame.

This PR fixes `range_to_buffer_range`, so that it returns a sensible
range if the input range is empty, preventing the fallback behaviour and
edit expansion.

Self-Review Checklist:

- [ ] I've reviewed my own diff for quality, security, and reliability
- [ ] Unsafe blocks (if any) have justifying comments
- [ ] The content is consistent with the [UI/UX
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
- [ ] Tests cover the new/changed behavior
- [ ] Performance impact has been considered and is acceptable

Closes #ISSUE

Release Notes:

- N/A or Added/Fixed/Improved ...

---------

Co-authored-by: Cole Miller <cole@zed.dev>
Co-authored-by: Anthony Eid <hello@anthonyeid.me>

crates/editor/src/display_map.rs

@@ -287,6 +287,12 @@ impl Companion {
         };
 
         let Some(excerpt) = patches.into_iter().next() else {
+            if cfg!(any(test, debug_assertions)) {
+                assert!(
+                    our_snapshot.max_point() == Point::zero(),
+                    "`patches_for_*_in_range` is only allowed to return an empty vec if the multibuffer is empty"
+                );
+            }
             return Point::zero()..our_snapshot.max_point();
         };
         excerpt.patch.edit_for_old_position(point).new

crates/editor/src/split.rs

@@ -207,14 +207,12 @@ where
             return;
         };
 
-        let Some(diff) =
-            source_snapshot.diff_for_buffer_id(first.source_buffer_snapshot.remote_id())
-        else {
+        let source_buffer_id = first.source_buffer_snapshot.remote_id();
+        let Some(diff) = source_snapshot.diff_for_buffer_id(source_buffer_id) else {
             pending.clear();
             return;
         };
-        let source_is_lhs =
-            first.source_buffer_snapshot.remote_id() == diff.base_text().remote_id();
+        let source_is_lhs = source_buffer_id == diff.base_text().remote_id();
         let target_buffer_id = if source_is_lhs {
             diff.buffer_id()
         } else {
@@ -232,28 +230,34 @@ where
 
         let patch = translate_fn(diff, union_start..=union_end, rhs_buffer);
 
-        for excerpt in pending.drain(..) {
-            let target_position = patch.old_to_new(excerpt.buffer_point_range.start);
-            let target_position = target_buffer.anchor_before(target_position);
-            let Some(target_position) = target_snapshot.anchor_in_excerpt(target_position) else {
-                continue;
-            };
-            let Some((target_buffer_snapshot, target_excerpt_range)) =
-                target_snapshot.excerpt_containing(target_position..target_position)
-            else {
-                continue;
-            };
+        let mut source_excerpts = source_snapshot
+            .excerpts_for_buffer(source_buffer_id)
+            .peekable();
+        let mut target_excerpts = target_snapshot
+            .excerpts_for_buffer(target_buffer_id)
+            .peekable();
 
-            result.push(patch_for_excerpt(
-                source_snapshot,
-                target_snapshot,
-                &excerpt.source_buffer_snapshot,
-                target_buffer_snapshot,
-                excerpt.source_excerpt_range,
-                target_excerpt_range,
-                &patch,
-                excerpt.buffer_point_range,
-            ));
+        for excerpt in pending.drain(..) {
+            while let Some(source_excerpt_range) = source_excerpts.peek()
+                && source_excerpt_range != &excerpt.source_excerpt_range
+            {
+                source_excerpts.next();
+                target_excerpts.next();
+            }
+            if let Some(source_excerpt_range) = source_excerpts.peek()
+                && let Some(target_excerpt_range) = target_excerpts.peek()
+            {
+                result.push(patch_for_excerpt(
+                    source_snapshot,
+                    target_snapshot,
+                    &excerpt.source_buffer_snapshot,
+                    target_buffer,
+                    source_excerpt_range.clone(),
+                    target_excerpt_range.clone(),
+                    &patch,
+                    excerpt.buffer_point_range,
+                ));
+            }
         }
     };
 

crates/git_ui/src/project_diff.rs

@@ -6,7 +6,7 @@ use crate::{
 use agent_settings::AgentSettings;
 use anyhow::{Context as _, Result, anyhow};
 use buffer_diff::{BufferDiff, DiffHunkSecondaryStatus};
-use collections::{HashMap, HashSet};
+use collections::HashMap;
 use editor::{
     Addon, Editor, EditorEvent, EditorSettings, SelectionEffects, SplittableEditor,
     actions::{GoToHunk, GoToPreviousHunk, SendReviewToAgent},
@@ -768,7 +768,7 @@ impl ProjectDiff {
         needs_fold
     }
 
-    #[instrument(skip_all)]
+    #[instrument(skip(this, cx))]
     pub async fn refresh(
         this: WeakEntity<Self>,
         reason: RefreshReason,
@@ -780,13 +780,13 @@ impl ProjectDiff {
                 let load_buffers = branch_diff.load_buffers(cx);
                 (branch_diff.repo().cloned(), load_buffers)
             });
-            let mut previous_paths = this
+            let mut previous_buffers = this
                 .multibuffer
                 .read(cx)
                 .snapshot(cx)
                 .buffers_with_paths()
-                .map(|(_, path_key)| path_key.clone())
-                .collect::<HashSet<_>>();
+                .map(|(buffer_snapshot, path_key)| (path_key.clone(), buffer_snapshot.remote_id()))
+                .collect::<HashMap<_, _>>();
 
             if let Some(repo) = repo {
                 let repo = repo.read(cx);
@@ -796,14 +796,14 @@ impl ProjectDiff {
                     let sort_prefix = sort_prefix(&repo, &entry.repo_path, entry.file_status, cx);
                     let path_key =
                         PathKey::with_sort_prefix(sort_prefix, entry.repo_path.as_ref().clone());
-                    previous_paths.remove(&path_key);
+                    previous_buffers.remove(&path_key);
                     path_keys.push(path_key)
                 }
             }
 
             this.editor.update(cx, |editor, cx| {
-                for path in previous_paths {
-                    if let Some(buffer) = this.multibuffer.read(cx).buffer_for_path(&path, cx) {
+                for (path, buffer_id) in previous_buffers {
+                    if let Some(buffer) = this.multibuffer.read(cx).buffer(buffer_id) {
                         let skip = match reason {
                             RefreshReason::DiffChanged | RefreshReason::EditorSaved => {
                                 buffer.read(cx).is_dirty()
@@ -816,6 +816,8 @@ impl ProjectDiff {
                     }
 
                     this.buffer_diff_subscriptions.remove(&path.path);
+                    let _span = ztracing::info_span!("remove_excerpts_for_path");
+                    _span.enter();
                     editor.remove_excerpts_for_path(path, cx);
                 }
             });

crates/multi_buffer/src/multi_buffer.rs

@@ -3636,6 +3636,7 @@ impl MultiBufferSnapshot {
         result
     }
 
+    /// Callers should not provide a range where `end < start`
     pub fn range_to_buffer_ranges<T: ToOffset>(
         &self,
         range: Range<T>,
@@ -3647,6 +3648,7 @@ impl MultiBufferSnapshot {
         let mut cursor = self.cursor::<MultiBufferOffset, BufferOffset>();
         let start = range.start.to_offset(self);
         let end = range.end.to_offset(self);
+        let range_non_empty = end > start;
         cursor.seek(&start);
 
         let mut result: Vec<(
@@ -3655,7 +3657,7 @@ impl MultiBufferSnapshot {
             ExcerptRange<text::Anchor>,
         )> = Vec::new();
         while let Some(region) = cursor.region() {
-            if region.range.start >= end {
+            if region.range.start > end || (region.range.start == end && range_non_empty) {
                 break;
             }
             if region.is_main_buffer {

crates/multi_buffer/src/multi_buffer_tests.rs

@@ -5864,6 +5864,48 @@ fn test_range_to_buffer_ranges(cx: &mut App) {
     assert_eq!(ranges_half_open_max[1].1, BufferOffset(0)..BufferOffset(0));
 }
 
+#[gpui::test]
+fn test_range_to_buffer_ranges_zero_length_at_excerpt_boundary(cx: &mut App) {
+    let buffer_1 = cx.new(|cx| Buffer::local("aaa\nbbb", cx));
+    let buffer_2 = cx.new(|cx| Buffer::local("ccc\nddd", cx));
+
+    let multibuffer = cx.new(|_| MultiBuffer::new(Capability::ReadWrite));
+    multibuffer.update(cx, |multibuffer, cx| {
+        multibuffer.set_excerpts_for_path(
+            PathKey::sorted(0),
+            buffer_1.clone(),
+            [Point::new(0, 0)..Point::new(1, 3)],
+            0,
+            cx,
+        );
+        multibuffer.set_excerpts_for_path(
+            PathKey::sorted(1),
+            buffer_2.clone(),
+            [Point::new(0, 0)..Point::new(1, 3)],
+            0,
+            cx,
+        );
+    });
+
+    let snapshot = multibuffer.read(cx).snapshot(cx);
+    assert_eq!(snapshot.text(), "aaa\nbbb\nccc\nddd");
+
+    // This point is right at the start of the very first excerpt, so if we get
+    // a buffer range, we should get `0..0`
+    let excerpt_2_start = Point::new(2, 0);
+    let expected_ranges = vec![BufferOffset(0)..BufferOffset(0)];
+    let ranges = snapshot
+        .range_to_buffer_ranges(excerpt_2_start..excerpt_2_start)
+        .into_iter()
+        .map(|tup| tup.1)
+        .collect_vec();
+
+    assert_eq!(
+        ranges, expected_ranges,
+        "Zero-length range at excerpt boundary should return the excerpt at that point"
+    );
+}
+
 #[gpui::test]
 async fn test_buffer_range_to_excerpt_ranges(cx: &mut TestAppContext) {
     let base_text = indoc!(

crates/multi_buffer/src/path_key.rs

@@ -58,12 +58,6 @@ impl PathKey {
 }
 
 impl MultiBuffer {
-    pub fn buffer_for_path(&self, path: &PathKey, cx: &App) -> Option<Entity<Buffer>> {
-        let snapshot = self.snapshot(cx);
-        let excerpt = snapshot.excerpts_for_path(path).next()?;
-        self.buffer(excerpt.context.start.buffer_id)
-    }
-
     pub fn location_for_path(&self, path: &PathKey, cx: &App) -> Option<Anchor> {
         let snapshot = self.snapshot(cx);
         let excerpt = snapshot.excerpts_for_path(path).next()?;