diff --git a/crates/benchmarks/Cargo.toml b/crates/benchmarks/Cargo.toml index 80e76ecf3d5..16c7dcd564e 100644 --- a/crates/benchmarks/Cargo.toml +++ b/crates/benchmarks/Cargo.toml @@ -24,7 +24,7 @@ gpui_platform = { workspace = true, features = ["bench", "font-kit"] } itertools.workspace = true language = { workspace = true, features = ["test-support"] } markdown.workspace = true -multi_buffer = { workspace = true, features = ["test-support"] } +multi_buffer = { workspace = true, features = ["benchmarks"] } project.workspace = true settings = { workspace = true, features = ["benchmarks"] } text.workspace = true diff --git a/crates/benchmarks/benches/display_map.rs b/crates/benchmarks/benches/display_map.rs index 04cbf1172cd..32c278c1820 100644 --- a/crates/benchmarks/benches/display_map.rs +++ b/crates/benchmarks/benches/display_map.rs @@ -19,7 +19,7 @@ fn to_tab_point_benchmark(c: &mut Criterion) { let text = RandomCharIter::new(&mut rng) .take(length) .collect::(); - let buffer = cx.update(|cx| MultiBuffer::build_simple(&text, cx)); + let buffer = cx.update(|cx| MultiBuffer::build_simple_for_benchmarks(&text, cx)); let buffer_snapshot = cx.read(|cx| buffer.read(cx).snapshot(cx)); use editor::display_map::*; @@ -64,7 +64,7 @@ fn to_fold_point_benchmark(c: &mut Criterion) { let text = RandomCharIter::new(&mut rng) .take(length) .collect::(); - let buffer = cx.update(|cx| MultiBuffer::build_simple(&text, cx)); + let buffer = cx.update(|cx| MultiBuffer::build_simple_for_benchmarks(&text, cx)); let buffer_snapshot = cx.read(|cx| buffer.read(cx).snapshot(cx)); use editor::display_map::*; @@ -145,7 +145,7 @@ fn create_highlight_endpoints_benchmark(c: &mut Criterion) { text.push_str("; }\n"); } - let buffer = cx.update(|cx| MultiBuffer::build_simple(&text, cx)); + let buffer = cx.update(|cx| MultiBuffer::build_simple_for_benchmarks(&text, cx)); let buffer_snapshot = cx.read(|cx| buffer.read(cx).snapshot(cx)); let highlight_ranges = highlight_ranges .into_iter() @@ -240,7 +240,7 @@ fn highlighted_chunks_benchmark(c: &mut Criterion) { let text = std::iter::repeat_n(line, LINE_COUNT) .collect::>() .join("\n"); - let buffer = cx.update(|cx| MultiBuffer::build_simple(&text, cx)); + let buffer = cx.update(|cx| MultiBuffer::build_simple_for_benchmarks(&text, cx)); let map = cx.new(|cx| { DisplayMap::new( buffer, diff --git a/crates/benchmarks/benches/editor_render.rs b/crates/benchmarks/benches/editor_render.rs index 9623ff8239c..9864e29ddea 100644 --- a/crates/benchmarks/benches/editor_render.rs +++ b/crates/benchmarks/benches/editor_render.rs @@ -18,7 +18,7 @@ fn editor_multi_cursor_input(line_count: &usize, cx: &mut BenchAppContext) { init_context(cx); let text = "line:\n".repeat(*line_count); - let buffer = cx.update(|cx| MultiBuffer::build_simple(&text, cx)); + let buffer = cx.update(|cx| MultiBuffer::build_simple_for_benchmarks(&text, cx)); let mut window = cx.add_empty_window(); let editor = window.update(|window, cx| { @@ -70,7 +70,7 @@ fn open_editor_with_one_long_line(cx: &mut BenchAppContext) { let text = String::from_iter(["char"; 1000]); cx.bench_iter(move |cx| { - let buffer = cx.update(|cx| MultiBuffer::build_simple(&text, cx)); + let buffer = cx.update(|cx| MultiBuffer::build_simple_for_benchmarks(&text, cx)); let mut window = cx.add_empty_window(); window.update(|window, cx| { @@ -96,9 +96,9 @@ fn editor_render(cx: &mut BenchAppContext) { let text = RandomCharIter::new(&mut rng) .take(text_len) .collect::(); - MultiBuffer::build_simple(&text, cx) + MultiBuffer::build_simple_for_benchmarks(&text, cx) } else { - MultiBuffer::build_random(&mut rng, cx) + MultiBuffer::build_random_for_benchmarks(&mut rng, cx) } }); diff --git a/crates/multi_buffer/Cargo.toml b/crates/multi_buffer/Cargo.toml index dcdca9b7cf1..a729b4b46f2 100644 --- a/crates/multi_buffer/Cargo.toml +++ b/crates/multi_buffer/Cargo.toml @@ -13,6 +13,12 @@ path = "src/multi_buffer.rs" doctest = false [features] +# Gates `MultiBuffer::build_simple_for_benchmarks`/`build_random_for_benchmarks`, +# production-faithful equivalents of `build_simple`/`build_random` for +# production-rendering benchmarks (`crates/benchmarks`). Deliberately empty: +# unlike `test-support`, this must not enable any dependency's test-only +# surface, so `benchmarks` consumers keep compiling production code only. +benchmarks = [] test-support = [ "buffer_diff/test-support", "gpui/test-support", diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 10b0e25dd70..f583ecb3523 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -29,7 +29,7 @@ use language::{ language_settings::{AllLanguageSettings, LanguageSettings}, }; -#[cfg(any(test, feature = "test-support"))] +#[cfg(any(test, feature = "test-support", feature = "benchmarks"))] use gpui::AppContext as _; use rope::DimensionPair; @@ -3359,6 +3359,137 @@ impl MultiBuffer { } } +/// Character generator for `MultiBuffer::build_random_for_benchmarks`, vendored +/// from `util::RandomCharIter`'s non-`SIMPLE_TEXT` branch rather than reused +/// directly: `util::RandomCharIter` lives behind `util`'s `test-support` +/// feature, which the `benchmarks` feature must not pull in (see this crate's +/// `benchmarks` feature). Kept as an exact algorithmic copy, verified by +/// `random_for_benchmarks_matches_random` in `multi_buffer_tests`, so a +/// benchmark seed produces the same text `randomly_edit_excerpts` would have +/// produced. +#[cfg(any(test, feature = "benchmarks"))] +struct BenchmarkRandomCharIter(T); + +#[cfg(any(test, feature = "benchmarks"))] +impl Iterator for BenchmarkRandomCharIter { + type Item = char; + + fn next(&mut self) -> Option { + use rand::seq::IndexedRandom as _; + + match self.0.random_range(0..100) { + 0..=19 => [' ', '\n', '\r', '\t'].choose(&mut self.0).copied(), + 20..=32 => char::from_u32(self.0.random_range(('α' as u32)..('ω' as u32 + 1))), + 33..=45 => ['✋', '✅', '❌', '❎', '⭐'].choose(&mut self.0).copied(), + 46..=58 => ['🍐', '🏀', '🍗', '🎉'].choose(&mut self.0).copied(), + _ => Some(self.0.random_range(b'a'..b'z' + 1).into()), + } + } +} + +/// Production-faithful equivalents of `build_simple`/`build_random`, usable by +/// production-rendering benchmarks (`crates/benchmarks`) through the narrow +/// `benchmarks` feature instead of `test-support`. `build_simple` and +/// `build_random` above stay `test`/`test-support`-only rather than widening +/// their own `cfg` because `randomly_edit_excerpts` depends on +/// `util::RandomCharIter`, which brings in `util`'s `test-support` feature. +#[cfg(any(test, feature = "benchmarks"))] +impl MultiBuffer { + pub fn build_simple_for_benchmarks(text: &str, cx: &mut gpui::App) -> Entity { + let buffer = cx.new(|cx| Buffer::local(text, cx)); + cx.new(|cx| Self::singleton(buffer, cx)) + } + + pub fn build_random_for_benchmarks( + rng: &mut impl rand::Rng, + cx: &mut gpui::App, + ) -> Entity { + cx.new(|cx| { + let mut multibuffer = MultiBuffer::new(Capability::ReadWrite); + let mutation_count = rng.random_range(1..=5); + multibuffer.randomly_edit_excerpts_for_benchmarks(rng, mutation_count, cx); + multibuffer + }) + } + + /// Mirrors `randomly_edit_excerpts`'s algorithm exactly, substituting + /// `BenchmarkRandomCharIter` for `util::RandomCharIter` as the only + /// test-support-only dependency it has. Kept as a separate function + /// rather than sharing an implementation so that a future change to the + /// `test-support` version (e.g. restoring `SIMPLE_TEXT` support) cannot + /// accidentally widen the `benchmarks` feature's dependencies; a drift + /// between the two is instead caught by + /// `random_for_benchmarks_matches_random`. + fn randomly_edit_excerpts_for_benchmarks( + &mut self, + rng: &mut impl rand::Rng, + mutation_count: usize, + cx: &mut Context, + ) { + use rand::prelude::*; + use std::env; + + let max_buffers = env::var("MAX_BUFFERS") + .map(|i| i.parse().expect("invalid `MAX_EXCERPTS` variable")) + .unwrap_or(5); + + let mut buffers = Vec::new(); + for _ in 0..mutation_count { + let snapshot = self.snapshot(cx); + let buffer_ids = snapshot.all_buffer_ids().collect::>(); + if buffer_ids.is_empty() || (rng.random() && buffer_ids.len() < max_buffers) { + let buffer_handle = if rng.random() || self.buffers.is_empty() { + let text = BenchmarkRandomCharIter(&mut *rng) + .take(10) + .collect::(); + buffers.push(cx.new(|cx| Buffer::local(text, cx))); + buffers.last().unwrap().clone() + } else { + self.buffers.values().choose(rng).unwrap().buffer.clone() + }; + + let buffer = buffer_handle.read(cx); + let buffer_snapshot = buffer.snapshot(); + let mut next_min_start_ix = 0; + let ranges = (0..rng.random_range(0..5)) + .filter_map(|_| { + if next_min_start_ix >= buffer.len() { + return None; + } + let end_ix = buffer.clip_offset( + rng.random_range(next_min_start_ix..=buffer.len()), + Bias::Right, + ); + let start_ix = buffer + .clip_offset(rng.random_range(next_min_start_ix..=end_ix), Bias::Left); + next_min_start_ix = buffer.text().ceil_char_boundary(end_ix + 1); + Some(ExcerptRange::new(start_ix..end_ix)) + }) + .collect::>(); + + let path_key = PathKey::for_buffer(&buffer_handle, cx); + self.set_merged_excerpt_ranges_for_path( + path_key, + buffer_handle, + &buffer_snapshot, + ranges, + cx, + ); + } else { + let path_key = self + .snapshot + .borrow() + .buffers + .get(&buffer_ids.choose(rng).unwrap()) + .unwrap() + .path_key + .clone(); + self.remove_excerpts(path_key, cx); + } + } + } +} + impl EventEmitter for MultiBuffer {} impl MultiBufferSnapshot { diff --git a/crates/multi_buffer/src/multi_buffer_tests.rs b/crates/multi_buffer/src/multi_buffer_tests.rs index 8e52eac1cb1..c22bc2b22b6 100644 --- a/crates/multi_buffer/src/multi_buffer_tests.rs +++ b/crates/multi_buffer/src/multi_buffer_tests.rs @@ -5310,6 +5310,27 @@ fn test_new_empty_buffers_title_can_be_set(cx: &mut App) { assert_eq!(multibuffer.read(cx).title(cx), "Hey"); } +/// Guards `MultiBuffer::build_random_for_benchmarks` and +/// `BenchmarkRandomCharIter` (see `multi_buffer.rs`) against drifting from +/// `MultiBuffer::build_random`: given the same rng sequence, the two must +/// produce byte-identical multi-buffer content, since production-rendering +/// benchmarks rely on `build_random_for_benchmarks` to reproduce the same +/// input shape `build_random` would have produced. +#[gpui::test(iterations = 50)] +fn random_for_benchmarks_matches_random(cx: &mut App, mut rng: StdRng) { + let production_multibuffer = MultiBuffer::build_random(&mut rng.clone(), cx); + let benchmark_multibuffer = MultiBuffer::build_random_for_benchmarks(&mut rng, cx); + + let production_snapshot = production_multibuffer.read(cx).snapshot(cx); + let benchmark_snapshot = benchmark_multibuffer.read(cx).snapshot(cx); + + assert_eq!( + production_snapshot.excerpts().count(), + benchmark_snapshot.excerpts().count(), + ); + assert_eq!(production_snapshot.text(), benchmark_snapshot.text()); +} + #[gpui::test(iterations = 100)] fn test_random_chunk_bitmaps(cx: &mut App, mut rng: StdRng) { let multibuffer = if rng.random() { diff --git a/script/check-gpui-bench-feature-isolation b/script/check-gpui-bench-feature-isolation index d3aa729c23a..d4843d6ccbd 100755 --- a/script/check-gpui-bench-feature-isolation +++ b/script/check-gpui-bench-feature-isolation @@ -85,29 +85,33 @@ for isolated_crate in agent editor language_model project; do fi done -# `lsp`, `language`, and `multi_buffer` are not part of the loop below: -# `display_map`/`editor_render`/`markdown_renderer` (the benchmarks that -# remain in this package) directly call test-only APIs with no production -# equivalent — `LanguageRegistry::test`, `language::rust_lang`, -# `MultiBuffer::build_simple`/`build_random` — independent of -# `edit_file_tool`. `language/test-support` enables both `lsp/test-support` -# and `settings/test-support` directly, so both keep showing up transitively -# despite `benchmarks` no longer requesting either itself. This is real, -# tracked debt, not a gap in this script: `theme`, `util`, and `settings` are -# the crates whose benchmark usage turned out to need no test-only API at all +# `lsp` and `language` are not part of the loop below: `markdown_renderer` +# (the only benchmark left that needs either) directly calls test-only APIs +# with no production equivalent — `LanguageRegistry::test`, +# `language::rust_lang` — independent of `edit_file_tool`. `language/ +# test-support` enables both `lsp/test-support` and `settings/test-support` +# directly, so both keep showing up transitively despite `benchmarks` no +# longer requesting either itself. This is real, tracked debt, not a gap in +# this script: `theme`, `util`, `settings`, and `multi_buffer` are the crates +# whose benchmark usage turned out to need no test-only API at all # (`theme::LoadThemes`, `theme_settings::init`), one narrow enough to vendor # into `benchmarks::bench_utils` instead (`util::RandomCharIter`, a # synthetic-text generator with no production analogue), or one narrow # enough to move behind its own dedicated feature instead of `test-support` -# (`settings`'s `benchmarks` feature, gating `SettingsStore::benchmarks` and -# `benchmark_settings` — see `crates/settings/src/settings_file.rs`, which -# keeps their settings content byte-identical to `SettingsStore::test`'s so -# switching does not change what a benchmark measures). So `benchmarks` no -# longer needs a direct `test-support` edge to any of the three. This checks -# only for a *direct* edge from `benchmarks` to each crate's `test-support` -# feature, not for the feature's absence from the whole resolved graph, since -# `language`/`multi_buffer` keep populating `settings/test-support` -# (transitively, through `language`) until they get the same treatment. +# (`settings`'s and `multi_buffer`'s `benchmarks` features, gating +# `SettingsStore::benchmarks`/`benchmark_settings` and `MultiBuffer:: +# build_simple_for_benchmarks`/`build_random_for_benchmarks` respectively — +# see `crates/settings/src/settings_file.rs` and +# `crates/multi_buffer/src/multi_buffer.rs`, which keep their fixtures +# byte-identical to the `test-support` versions' so switching does not change +# what a benchmark measures). So `benchmarks` no longer needs a direct +# `test-support` edge to any of the four. This checks only for a *direct* +# edge from `benchmarks` to each crate's `test-support` feature, not for the +# feature's absence from the whole resolved graph, since `language` keeps +# populating `settings/test-support` (transitively) until it gets the same +# treatment; `multi_buffer` is checked separately below since nothing else +# `benchmarks` depends on populates its `test-support` feature at all +# anymore. for foundational_crate in theme util settings; do output=$(cargo tree \ --offline \ @@ -122,7 +126,7 @@ for foundational_crate in theme util settings; do # un-suffixed "test-support" feature line is where its direct dependents # are actually listed; checking the couple of lines after it for a # "benchmarks" edge is enough to catch a direct request without also - # matching the expected indirect one from `language`/`multi_buffer`. + # matching the expected indirect one from `language`. if echo "${output}" \ | grep -A2 "[|\`]-- ${foundational_crate} feature \"test-support\"\$" \ | grep --quiet 'benchmarks v'; then @@ -149,6 +153,46 @@ if ! echo "${output}" | grep --quiet 'settings feature "benchmarks"'; then exit 1 fi +# `crates/benchmarks/Cargo.toml` used to directly request `multi_buffer`'s +# `test-support` feature, entirely to reach `MultiBuffer::build_simple`/ +# `build_random` for `display_map`/`editor_render`'s fixtures. `multi_buffer` +# now exposes `build_simple_for_benchmarks`/`build_random_for_benchmarks` +# behind its own narrow `benchmarks` feature instead (see +# `crates/multi_buffer/Cargo.toml` and `crates/multi_buffer/src/ +# multi_buffer.rs`), so unlike `theme`/`util`/`settings` above, this checks +# `multi_buffer`'s `test-support` feature is absent from `benchmarks`' whole +# resolved graph, not just as a direct edge: nothing else `benchmarks` +# depends on has a legitimate reason to reach it either. +output=$(cargo tree \ + --offline \ + --package benchmarks \ + --edges features \ + --invert multi_buffer) +if echo "${output}" | grep --quiet 'multi_buffer feature "test-support"'; then + echo "benchmarks pulls multi_buffer's \"test-support\" feature:" + echo "${output}" + exit 1 +fi + +# The check above proves `benchmarks` no longer reaches `multi_buffer`'s +# `test-support` feature; this proves `multi_buffer`'s own `benchmarks` +# feature (which `build_simple_for_benchmarks`/`build_random_for_benchmarks` +# live behind) doesn't itself resolve to any dependency's `test-support`, +# the same guarantee the top of this script proves for `gpui`'s `bench` +# feature. +output=$(cargo tree \ + --offline \ + --package multi_buffer \ + --no-default-features \ + --features benchmarks \ + --edges features \ + --invert multi_buffer) +if echo "${output}" | grep --quiet 'test-support'; then + echo "multi_buffer's \"benchmarks\" feature pulls in \"test-support\":" + echo "${output}" + exit 1 +fi + # `edit_file_tool_benchmarks` is the isolated package `edit_file_tool` moved # into. It is intentionally *not* held to the isolation bar above, including # the `settings` checks just above: it still needs `test-support` from