From 1446d84941cf0f04912559f31a399bb79053238b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yara=20=F0=9F=8F=B3=EF=B8=8F=E2=80=8D=E2=9A=A7=EF=B8=8F?= Date: Wed, 17 Dec 2025 17:14:57 +0100 Subject: [PATCH] Blockmap sync fix (#44743) Release Notes: - Improved display map rendering performance with many lines in the the multi-buffer. --------- Co-authored-by: cameron Co-authored-by: Cole Miller --- .mailmap | 3 + crates/editor/src/display_map.rs | 49 +++++++++ crates/editor/src/display_map/block_map.rs | 3 +- crates/editor/src/display_map/inlay_map.rs | 9 ++ crates/editor/src/display_map/wrap_map.rs | 118 ++++++++++++++++++--- crates/gpui/src/test.rs | 5 +- crates/ztracing/src/lib.rs | 22 ++-- 7 files changed, 185 insertions(+), 24 deletions(-) diff --git a/.mailmap b/.mailmap index db4632d6ca34346d3e8fa289222d7f310b7bdfe5..1e956c52cf76589fc016e1410122ccd94e4818ae 100644 --- a/.mailmap +++ b/.mailmap @@ -141,6 +141,9 @@ Uladzislau Kaminski Uladzislau Kaminski Vitaly Slobodin Vitaly Slobodin +Yara +Yara +Yara Will Bradley Will Bradley WindSoilder diff --git a/crates/editor/src/display_map.rs b/crates/editor/src/display_map.rs index cab5b3686ee2f77dade059b434b1090cf9b2f7e5..413766cb283dfa2c5de0351b3ff10ff9b90a9c56 100644 --- a/crates/editor/src/display_map.rs +++ b/crates/editor/src/display_map.rs @@ -14,8 +14,57 @@ //! - [`DisplayMap`] that adds background highlights to the regions of text. //! Each one of those builds on top of preceding map. //! +//! ## Structure of the display map layers +//! +//! Each layer in the map (and the multibuffer itself to some extent) has a few +//! structures that are used to implement the public API available to the layer +//! above: +//! - a `Transform` type - this represents a region of text that the layer in +//! question is "managing", that it transforms into a more "processed" text +//! for the layer above. For example, the inlay map has an `enum Transform` +//! that has two variants: +//! - `Isomorphic`, representing a region of text that has no inlay hints (i.e. +//! is passed through the map transparently) +//! - `Inlay`, representing a location where an inlay hint is to be inserted. +//! - a `TransformSummary` type, which is usually a struct with two fields: +//! [`input: TextSummary`][`TextSummary`] and [`output: TextSummary`][`TextSummary`]. Here, +//! `input` corresponds to "text in the layer below", and `output` corresponds to the text +//! exposed to the layer above. So in the inlay map case, a `Transform::Isomorphic`'s summary is +//! just `input = output = summary`, where `summary` is the [`TextSummary`] stored in that +//! variant. Conversely, a `Transform::Inlay` always has an empty `input` summary, because it's +//! not "replacing" any text that exists on disk. The `output` is the summary of the inlay text +//! to be injected. - Various newtype wrappers for co-ordinate spaces (e.g. [`WrapRow`] +//! represents a row index, after soft-wrapping (and all lower layers)). +//! - A `Snapshot` type (e.g. [`InlaySnapshot`]) that captures the state of a layer at a specific +//! point in time. +//! - various APIs which drill through the layers below to work with the underlying text. Notably: +//! - `fn text_summary_for_offset()` returns a [`TextSummary`] for the range in the co-ordinate +//! space that the map in question is responsible for. +//! - `fn _point_to__point()` converts a point in co-ordinate space `A` into co-ordinate +//! space `B`. +//! - A [`RowInfo`] iterator (e.g. [`InlayBufferRows`]) and a [`Chunk`] iterator +//! (e.g. [`InlayChunks`]) +//! - A `sync` function (e.g. [`InlayMap::sync`]) that takes a snapshot and list of [`Edit`]s, +//! and returns a new snapshot and a list of transformed [`Edit`]s. Note that the generic +//! parameter on `Edit` changes, since these methods take in edits in the co-ordinate space of +//! the lower layer, and return edits in their own co-ordinate space. The term "edit" is +//! slightly misleading, since an [`Edit`] doesn't tell you what changed - rather it can be +//! thought of as a "region to invalidate". In theory, it would be correct to always use a +//! single edit that covers the entire range. However, this would lead to lots of unnecessary +//! recalculation. +//! +//! See the docs for the [`inlay_map`] module for a more in-depth explanation of how a single layer +//! works. +//! //! [Editor]: crate::Editor //! [EditorElement]: crate::element::EditorElement +//! [`TextSummary`]: multi_buffer::MBTextSummary +//! [`WrapRow`]: wrap_map::WrapRow +//! [`InlayBufferRows`]: inlay_map::InlayBufferRows +//! [`InlayChunks`]: inlay_map::InlayChunks +//! [`Edit`]: text::Edit +//! [`Edit`]: text::Edit +//! [`Chunk`]: language::Chunk #[macro_use] mod dimensions; diff --git a/crates/editor/src/display_map/block_map.rs b/crates/editor/src/display_map/block_map.rs index 9c7f9d8632224208248a6585fc6f94939ee076fe..15bf012cd907da2455c1a2205bcccd363162fd46 100644 --- a/crates/editor/src/display_map/block_map.rs +++ b/crates/editor/src/display_map/block_map.rs @@ -545,7 +545,7 @@ impl BlockMap { { let max_point = wrap_snapshot.max_point(); let edit_start = wrap_snapshot.prev_row_boundary(max_point); - let edit_end = max_point.row() + WrapRow(1); + let edit_end = max_point.row() + WrapRow(1); // this is end of file edits = edits.compose([WrapEdit { old: edit_start..edit_end, new: edit_start..edit_end, @@ -715,6 +715,7 @@ impl BlockMap { let placement = block.placement.to_wrap_row(wrap_snapshot)?; if let BlockPlacement::Above(row) = placement && row < new_start + // this will be true more often now { return None; } diff --git a/crates/editor/src/display_map/inlay_map.rs b/crates/editor/src/display_map/inlay_map.rs index d85f761a82e2f466b6868c4ce28bcb3a4e6b061d..cbdc4b18fee452163c5a11932c968cb7cc500f96 100644 --- a/crates/editor/src/display_map/inlay_map.rs +++ b/crates/editor/src/display_map/inlay_map.rs @@ -1,3 +1,10 @@ +//! The inlay map. See the [`display_map`][super] docs for an overview of how the inlay map fits +//! into the rest of the [`DisplayMap`][super::DisplayMap]. Much of the documentation for this +//! module generalizes to other layers. +//! +//! The core of this module is the [`InlayMap`] struct, which maintains a vec of [`Inlay`]s, and +//! [`InlaySnapshot`], which holds a sum tree of [`Transform`]s. + use crate::{ ChunkRenderer, HighlightStyles, inlays::{Inlay, InlayContent}, @@ -69,7 +76,9 @@ impl sum_tree::Item for Transform { #[derive(Clone, Debug, Default)] struct TransformSummary { + /// Summary of the text before inlays have been applied. input: MBTextSummary, + /// Summary of the text after inlays have been applied. output: MBTextSummary, } diff --git a/crates/editor/src/display_map/wrap_map.rs b/crates/editor/src/display_map/wrap_map.rs index 4d6b79d06170a22aaffafa05e0f144219e4d20a7..879ca11be1a84ffd44daa6e53677b06887172026 100644 --- a/crates/editor/src/display_map/wrap_map.rs +++ b/crates/editor/src/display_map/wrap_map.rs @@ -840,35 +840,62 @@ impl WrapSnapshot { self.tab_point_to_wrap_point(self.tab_snapshot.clip_point(self.to_tab_point(point), bias)) } - #[ztracing::instrument(skip_all, fields(point=?point, ret))] - pub fn prev_row_boundary(&self, mut point: WrapPoint) -> WrapRow { + /// Try to find a TabRow start that is also a WrapRow start + /// Every TabRow start is a WrapRow start + #[ztracing::instrument(skip_all, fields(point=?point))] + pub fn prev_row_boundary(&self, point: WrapPoint) -> WrapRow { if self.transforms.is_empty() { return WrapRow(0); } - *point.column_mut() = 0; + let point = WrapPoint::new(point.row(), 0); let mut cursor = self .transforms .cursor::>(()); - // start + cursor.seek(&point, Bias::Right); - // end if cursor.item().is_none() { cursor.prev(); } - // start + // real newline fake fake + // text: helloworldasldlfjasd\njdlasfalsk\naskdjfasdkfj\n + // dimensions v v v v v + // transforms |-------|-----NW----|-----W------|-----W------| + // cursor ^ ^^^^^^^^^^^^^ ^ + // (^) ^^^^^^^^^^^^^^ + // point: ^ + // point(col_zero): (^) + while let Some(transform) = cursor.item() { - if transform.is_isomorphic() && cursor.start().1.column() == 0 { - return cmp::min(cursor.end().0.row(), point.row()); - } else { - cursor.prev(); + if transform.is_isomorphic() { + // this transform only has real linefeeds + let tab_summary = &transform.summary.input; + // is the wrap just before the end of the transform a tab row? + // thats only if this transform has at least one newline + // + // "this wrap row is a tab row" <=> self.to_tab_point(WrapPoint::new(wrap_row, 0)).column() == 0 + + // Note on comparison: + // We have code that relies on this to be row > 1 + // It should work with row >= 1 but it does not :( + // + // That means that if every line is wrapped we walk back all the + // way to the start. Which invalidates the entire state triggering + // a full re-render. + if tab_summary.lines.row > 1 { + let wrap_point_at_end = cursor.end().0.row(); + return cmp::min(wrap_point_at_end - RowDelta(1), point.row()); + } else if cursor.start().1.column() == 0 { + return cmp::min(cursor.end().0.row(), point.row()); + } } + + cursor.prev(); } - // end - unreachable!() + WrapRow(0) } #[ztracing::instrument(skip_all)] @@ -891,13 +918,11 @@ impl WrapSnapshot { } #[cfg(test)] - #[ztracing::instrument(skip_all)] pub fn text(&self) -> String { self.text_chunks(WrapRow(0)).collect() } #[cfg(test)] - #[ztracing::instrument(skip_all)] pub fn text_chunks(&self, wrap_row: WrapRow) -> impl Iterator { self.chunks( wrap_row..self.max_point().row() + WrapRow(1), @@ -1298,6 +1323,71 @@ mod tests { use text::Rope; use theme::LoadThemes; + #[gpui::test] + async fn test_prev_row_boundary(cx: &mut gpui::TestAppContext) { + init_test(cx); + + fn test_wrap_snapshot( + text: &str, + soft_wrap_every: usize, // font size multiple + cx: &mut gpui::TestAppContext, + ) -> WrapSnapshot { + let text_system = cx.read(|cx| cx.text_system().clone()); + let tab_size = 4.try_into().unwrap(); + let font = test_font(); + let _font_id = text_system.resolve_font(&font); + let font_size = px(14.0); + // this is very much an estimate to try and get the wrapping to + // occur at `soft_wrap_every` we check that it pans out for every test case + let soft_wrapping = Some(font_size * soft_wrap_every * 0.6); + + let buffer = cx.new(|cx| language::Buffer::local(text, cx)); + let buffer = cx.new(|cx| MultiBuffer::singleton(buffer, cx)); + let buffer_snapshot = buffer.read_with(cx, |buffer, cx| buffer.snapshot(cx)); + let (_inlay_map, inlay_snapshot) = InlayMap::new(buffer_snapshot); + let (_fold_map, fold_snapshot) = FoldMap::new(inlay_snapshot); + let (mut tab_map, _) = TabMap::new(fold_snapshot, tab_size); + let tabs_snapshot = tab_map.set_max_expansion_column(32); + let (_wrap_map, wrap_snapshot) = + cx.update(|cx| WrapMap::new(tabs_snapshot, font, font_size, soft_wrapping, cx)); + + wrap_snapshot + } + + // These two should pass but dont, see the comparison note in + // prev_row_boundary about why. + // + // // 0123 4567 wrap_rows + // let wrap_snapshot = test_wrap_snapshot("1234\n5678", 1, cx); + // assert_eq!(wrap_snapshot.text(), "1\n2\n3\n4\n5\n6\n7\n8"); + // let row = wrap_snapshot.prev_row_boundary(wrap_snapshot.max_point()); + // assert_eq!(row.0, 3); + + // // 012 345 678 wrap_rows + // let wrap_snapshot = test_wrap_snapshot("123\n456\n789", 1, cx); + // assert_eq!(wrap_snapshot.text(), "1\n2\n3\n4\n5\n6\n7\n8\n9"); + // let row = wrap_snapshot.prev_row_boundary(wrap_snapshot.max_point()); + // assert_eq!(row.0, 5); + + // 012345678 wrap_rows + let wrap_snapshot = test_wrap_snapshot("123456789", 1, cx); + assert_eq!(wrap_snapshot.text(), "1\n2\n3\n4\n5\n6\n7\n8\n9"); + let row = wrap_snapshot.prev_row_boundary(wrap_snapshot.max_point()); + assert_eq!(row.0, 0); + + // 111 2222 44 wrap_rows + let wrap_snapshot = test_wrap_snapshot("123\n4567\n\n89", 4, cx); + assert_eq!(wrap_snapshot.text(), "123\n4567\n\n89"); + let row = wrap_snapshot.prev_row_boundary(wrap_snapshot.max_point()); + assert_eq!(row.0, 2); + + // 11 2223 wrap_rows + let wrap_snapshot = test_wrap_snapshot("12\n3456\n\n", 3, cx); + assert_eq!(wrap_snapshot.text(), "12\n345\n6\n\n"); + let row = wrap_snapshot.prev_row_boundary(wrap_snapshot.max_point()); + assert_eq!(row.0, 3); + } + #[gpui::test(iterations = 100)] async fn test_random_wraps(cx: &mut gpui::TestAppContext, mut rng: StdRng) { // todo this test is flaky diff --git a/crates/gpui/src/test.rs b/crates/gpui/src/test.rs index 5ae72d2be1688893374e16a55445558b5bc33040..2a5711a01a9c8f2874cea4803fc517089cafd0fe 100644 --- a/crates/gpui/src/test.rs +++ b/crates/gpui/src/test.rs @@ -69,7 +69,10 @@ pub fn run_test( std::mem::forget(error); } else { if is_multiple_runs { - eprintln!("failing seed: {}", seed); + eprintln!("failing seed: {seed}"); + eprintln!( + "You can rerun from this seed by setting the environmental variable SEED to {seed}" + ); } if let Some(on_fail_fn) = on_fail_fn { on_fail_fn() diff --git a/crates/ztracing/src/lib.rs b/crates/ztracing/src/lib.rs index b9b318cc3565d8ced2a5496f1240409542d23c5a..c9007be1ed43150ef877d51c882aee77845e5bd6 100644 --- a/crates/ztracing/src/lib.rs +++ b/crates/ztracing/src/lib.rs @@ -1,8 +1,8 @@ -pub use tracing::Level; +pub use tracing::{Level, field}; #[cfg(ztracing)] pub use tracing::{ - debug_span, error_span, event, info_span, instrument, span, trace_span, warn_span, + Span, debug_span, error_span, event, info_span, instrument, span, trace_span, warn_span, }; #[cfg(not(ztracing))] pub use ztracing_macro::instrument; @@ -26,17 +26,23 @@ pub use __consume_all_tokens as span; #[macro_export] macro_rules! __consume_all_tokens { ($($t:tt)*) => { - $crate::FakeSpan + $crate::Span }; } -pub struct FakeSpan; -impl FakeSpan { +#[cfg(not(ztracing))] +pub struct Span; + +#[cfg(not(ztracing))] +impl Span { + pub fn current() -> Self { + Self + } + pub fn enter(&self) {} -} -// #[cfg(not(ztracing))] -// pub use span; + pub fn record(&self, _t: T, _s: S) {} +} #[cfg(ztracing)] pub fn init() {