Skip to content

Commit aa81505

Browse files
multibuffer: Speed up anchor resolution by avoiding an excerpts seek (#53340)
Before, every creation of `AnchorSeekTarget` called `MultiBufferSnapshot::buffer_for_path`, which seeks the excerpts sum tree. Now we avoid that by - looking up the buffer snapshot for the anchor's buffer in the `buffers` map (keyed by `BufferId`) - handling the case where the anchor's buffer doesn't exist at the original path key with a separate `AnchorSeekTarget::Missing` variant We also added a separate optimization to `AnchorSeekTarget::cmp`, to skip the full `PathKey` comparison when the cursor's `PathKeyIndex` is the same as the anchor's. This should help with singleton multibuffers. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - N/A --------- Co-authored-by: Anthony Eid <hello@anthonyeid.me>
1 parent 36a23c2 commit aa81505

File tree

2 files changed

+75
-46
lines changed

2 files changed

+75
-46
lines changed

crates/multi_buffer/src/anchor.rs

Lines changed: 52 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -34,29 +34,40 @@ pub enum Anchor {
3434
Max,
3535
}
3636

37-
pub(crate) enum AnchorSeekTarget {
37+
pub(crate) enum AnchorSeekTarget<'a> {
38+
// buffer no longer exists at its original path key in the multibuffer
39+
Missing {
40+
path_key: &'a PathKey,
41+
},
42+
// we have excerpts for the buffer at the expected path key
3843
Excerpt {
39-
path_key: PathKey,
40-
anchor: ExcerptAnchor,
41-
// None when the buffer no longer exists in the multibuffer
42-
snapshot: Option<BufferSnapshot>,
44+
path_key: &'a PathKey,
45+
path_key_index: PathKeyIndex,
46+
anchor: text::Anchor,
47+
snapshot: &'a BufferSnapshot,
4348
},
49+
// no excerpts and it's a min or max anchor
4450
Empty,
4551
}
4652

47-
impl std::fmt::Debug for AnchorSeekTarget {
53+
impl std::fmt::Debug for AnchorSeekTarget<'_> {
4854
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
4955
match self {
5056
Self::Excerpt {
5157
path_key,
58+
path_key_index: _,
5259
anchor,
5360
snapshot: _,
5461
} => f
5562
.debug_struct("Excerpt")
5663
.field("path_key", path_key)
5764
.field("anchor", anchor)
5865
.finish(),
59-
Self::Empty => write!(f, "Empty"),
66+
Self::Missing { path_key } => f
67+
.debug_struct("Missing")
68+
.field("path_key", path_key)
69+
.finish(),
70+
Self::Empty => f.debug_struct("Empty").finish(),
6071
}
6172
}
6273
}
@@ -110,15 +121,16 @@ impl ExcerptAnchor {
110121
return self.text_anchor.buffer_id.cmp(&other.text_anchor.buffer_id);
111122
}
112123

113-
let Some(buffer) = snapshot.buffer_for_path(&self_path_key) else {
114-
return Ordering::Equal;
115-
};
116-
// Comparing two anchors into buffer A that formerly existed at path P,
117-
// when path P has since been reused for a different buffer B
118-
if buffer.remote_id() != self.text_anchor.buffer_id {
124+
// two anchors into the same buffer at the same path
125+
// TODO(cole) buffer_for_path is slow
126+
let Some(buffer) = snapshot
127+
.buffer_for_path(&self_path_key)
128+
.filter(|buffer| buffer.remote_id() == self.text_anchor.buffer_id)
129+
else {
130+
// buffer no longer exists at the original path (which may have been reused for a different buffer),
131+
// so no way to compare the anchors
119132
return Ordering::Equal;
120133
};
121-
assert_eq!(self.text_anchor.buffer_id, buffer.remote_id());
122134
let text_cmp = self.text_anchor().cmp(&other.text_anchor(), buffer);
123135
if text_cmp != Ordering::Equal {
124136
return text_cmp;
@@ -234,21 +246,33 @@ impl ExcerptAnchor {
234246
.is_ge()
235247
}
236248

237-
pub(crate) fn seek_target(&self, snapshot: &MultiBufferSnapshot) -> AnchorSeekTarget {
249+
pub(crate) fn seek_target<'a>(
250+
&self,
251+
snapshot: &'a MultiBufferSnapshot,
252+
) -> AnchorSeekTarget<'a> {
238253
self.try_seek_target(snapshot)
239254
.expect("anchor is from different multi-buffer")
240255
}
241256

242-
pub(crate) fn try_seek_target(
257+
pub(crate) fn try_seek_target<'a>(
243258
&self,
244-
snapshot: &MultiBufferSnapshot,
245-
) -> Option<AnchorSeekTarget> {
259+
snapshot: &'a MultiBufferSnapshot,
260+
) -> Option<AnchorSeekTarget<'a>> {
246261
let path_key = snapshot.try_path_for_anchor(*self)?;
247-
let buffer = snapshot.buffer_for_path(&path_key).cloned();
262+
263+
let Some(state) = snapshot
264+
.buffers
265+
.get(&self.buffer_id())
266+
.filter(|state| &state.path_key == path_key)
267+
else {
268+
return Some(AnchorSeekTarget::Missing { path_key });
269+
};
270+
248271
Some(AnchorSeekTarget::Excerpt {
249272
path_key,
250-
anchor: *self,
251-
snapshot: buffer,
273+
path_key_index: self.path,
274+
anchor: self.text_anchor(),
275+
snapshot: &state.buffer_snapshot,
252276
})
253277
}
254278
}
@@ -372,7 +396,10 @@ impl Anchor {
372396
}
373397
}
374398

375-
pub(crate) fn seek_target(&self, snapshot: &MultiBufferSnapshot) -> AnchorSeekTarget {
399+
pub(crate) fn seek_target<'a>(
400+
&self,
401+
snapshot: &'a MultiBufferSnapshot,
402+
) -> AnchorSeekTarget<'a> {
376403
let Some(excerpt_anchor) = self.to_excerpt_anchor(snapshot) else {
377404
return AnchorSeekTarget::Empty;
378405
};
@@ -406,10 +433,10 @@ impl Anchor {
406433
}
407434
}
408435

409-
pub(crate) fn try_seek_target(
436+
pub(crate) fn try_seek_target<'a>(
410437
&self,
411-
snapshot: &MultiBufferSnapshot,
412-
) -> Option<AnchorSeekTarget> {
438+
snapshot: &'a MultiBufferSnapshot,
439+
) -> Option<AnchorSeekTarget<'a>> {
413440
let Some(excerpt_anchor) = self.to_excerpt_anchor(snapshot) else {
414441
return Some(AnchorSeekTarget::Empty);
415442
};

crates/multi_buffer/src/multi_buffer.rs

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -870,6 +870,7 @@ impl ExcerptRange<text::Anchor> {
870870
#[derive(Clone, Debug)]
871871
pub struct ExcerptSummary {
872872
path_key: PathKey,
873+
path_key_index: Option<PathKeyIndex>,
873874
max_anchor: Option<text::Anchor>,
874875
widest_line_number: u32,
875876
text: MBTextSummary,
@@ -880,6 +881,7 @@ impl ExcerptSummary {
880881
pub fn min() -> Self {
881882
ExcerptSummary {
882883
path_key: PathKey::min(),
884+
path_key_index: None,
883885
max_anchor: None,
884886
widest_line_number: 0,
885887
text: MBTextSummary::default(),
@@ -6386,11 +6388,11 @@ impl MultiBufferSnapshot {
63866388
self.buffers.get(&id).map(|state| &state.buffer_snapshot)
63876389
}
63886390

6389-
fn try_path_for_anchor(&self, anchor: ExcerptAnchor) -> Option<PathKey> {
6390-
self.path_keys_by_index.get(&anchor.path).cloned()
6391+
fn try_path_for_anchor(&self, anchor: ExcerptAnchor) -> Option<&PathKey> {
6392+
self.path_keys_by_index.get(&anchor.path)
63916393
}
63926394

6393-
pub fn path_for_anchor(&self, anchor: ExcerptAnchor) -> PathKey {
6395+
pub fn path_for_anchor(&self, anchor: ExcerptAnchor) -> &PathKey {
63946396
self.try_path_for_anchor(anchor)
63956397
.expect("invalid anchor: path was never added to multibuffer")
63966398
}
@@ -7327,6 +7329,7 @@ impl sum_tree::Item for Excerpt {
73277329
}
73287330
ExcerptSummary {
73297331
path_key: self.path_key.clone(),
7332+
path_key_index: Some(self.path_key_index),
73307333
max_anchor: Some(self.range.context.end),
73317334
widest_line_number: self.max_buffer_row,
73327335
text: text.into(),
@@ -7425,45 +7428,44 @@ impl sum_tree::ContextLessSummary for ExcerptSummary {
74257428
);
74267429

74277430
self.path_key = summary.path_key.clone();
7431+
self.path_key_index = summary.path_key_index;
74287432
self.max_anchor = summary.max_anchor;
74297433
self.text += summary.text;
74307434
self.widest_line_number = cmp::max(self.widest_line_number, summary.widest_line_number);
74317435
self.count += summary.count;
74327436
}
74337437
}
74347438

7435-
impl sum_tree::SeekTarget<'_, ExcerptSummary, ExcerptSummary> for AnchorSeekTarget {
7439+
impl sum_tree::SeekTarget<'_, ExcerptSummary, ExcerptSummary> for AnchorSeekTarget<'_> {
74367440
fn cmp(
74377441
&self,
74387442
cursor_location: &ExcerptSummary,
74397443
_cx: <ExcerptSummary as sum_tree::Summary>::Context<'_>,
74407444
) -> cmp::Ordering {
74417445
match self {
7446+
AnchorSeekTarget::Missing { path_key } => {
7447+
// Want to end up after any excerpts for (a different buffer at) the original path
7448+
match Ord::cmp(*path_key, &cursor_location.path_key) {
7449+
Ordering::Less => Ordering::Less,
7450+
Ordering::Equal | Ordering::Greater => Ordering::Greater,
7451+
}
7452+
}
74427453
AnchorSeekTarget::Excerpt {
74437454
path_key,
7455+
path_key_index,
74447456
anchor,
74457457
snapshot,
74467458
} => {
7447-
let path_comparison = Ord::cmp(path_key, &cursor_location.path_key);
7448-
if path_comparison.is_ne() {
7449-
path_comparison
7450-
} else if let Some(snapshot) = snapshot {
7451-
if anchor.text_anchor.buffer_id != snapshot.remote_id() {
7452-
Ordering::Greater
7453-
} else if let Some(max_anchor) = cursor_location.max_anchor {
7454-
debug_assert_eq!(max_anchor.buffer_id, snapshot.remote_id());
7455-
anchor.text_anchor().cmp(&max_anchor, snapshot)
7456-
} else {
7457-
Ordering::Greater
7458-
}
7459+
if Some(*path_key_index) != cursor_location.path_key_index {
7460+
Ord::cmp(*path_key, &cursor_location.path_key)
7461+
} else if let Some(max_anchor) = cursor_location.max_anchor {
7462+
debug_assert_eq!(max_anchor.buffer_id, snapshot.remote_id());
7463+
anchor.cmp(&max_anchor, snapshot)
74597464
} else {
7460-
// shouldn't happen because we expect this buffer not to have any excerpts
7461-
// (otherwise snapshot would have been Some)
7462-
Ordering::Equal
7465+
Ordering::Greater
74637466
}
74647467
}
7465-
// This should be dead code because Empty is only constructed for an empty snapshot
7466-
AnchorSeekTarget::Empty => Ordering::Equal,
7468+
AnchorSeekTarget::Empty => Ordering::Greater,
74677469
}
74687470
}
74697471
}

0 commit comments

Comments
 (0)