From 5447821715aaf628dfb0b31dd9bcad508c978b48 Mon Sep 17 00:00:00 2001 From: Piotr Osiewicz <24362066+osiewicz@users.noreply.github.com> Date: Thu, 2 Jan 2025 12:06:01 +0100 Subject: [PATCH] gpui/perf: Use SharedString on API boundary of line layout (#22566) This commit is all about strings, not about line layout at all. When laying out text, we use a line layout cache to avoid roundtrips to system layout engine where possible. This makes it so that we might end up not needing an owned version of text to insert into the cache, as we might get a cached version. The API boundary of line layout accepted text to be laid out as &str. It then performed cache lookup (which didn't require having an owned version) and only resorted to making an owned version when needed. As it turned out though, exact cache hits are quite rare and we end up needing owned version more often than not. The callers of line layout either dealt with SharedStrings or owned Strings. Due to coercing them into &str, we were ~always copying text into a new string (unless there was a same-frame-hit). This is a bit wasteful, thus this PR generifies the API a bit to make it easier to reuse existing string allocations if there are any. Benchmark scenario: scrolling down page-by-page through editor_tests (I ran the same scenario twice): ![1](https://github.com/user-attachments/assets/8cd09692-2699-41d9-b211-83554d93902f) ![2](https://github.com/user-attachments/assets/d11f7c22-2315-4261-8189-2356baf5d2f7) Release Notes: - N/A --- crates/gpui/src/shared_string.rs | 6 +++ crates/gpui/src/text_system.rs | 12 ++++-- crates/gpui/src/text_system/line_layout.rs | 43 +++++++++++++++------- 3 files changed, 44 insertions(+), 17 deletions(-) diff --git a/crates/gpui/src/shared_string.rs b/crates/gpui/src/shared_string.rs index 1fbd337bd0..b5210efa50 100644 --- a/crates/gpui/src/shared_string.rs +++ b/crates/gpui/src/shared_string.rs @@ -81,6 +81,12 @@ impl<'a> PartialEq<&'a str> for SharedString { } } +impl From<&SharedString> for SharedString { + fn from(value: &SharedString) -> Self { + value.clone() + } +} + impl From for Arc { fn from(val: SharedString) -> Self { match val.0 { diff --git a/crates/gpui/src/text_system.rs b/crates/gpui/src/text_system.rs index 27e7e55982..b695aac21c 100644 --- a/crates/gpui/src/text_system.rs +++ b/crates/gpui/src/text_system.rs @@ -356,7 +356,7 @@ impl WindowTextSystem { }); } - let layout = self.layout_line(text.as_ref(), font_size, runs)?; + let layout = self.layout_line(&text, font_size, runs)?; Ok(ShapedLine { layout, @@ -483,12 +483,16 @@ impl WindowTextSystem { /// Subsets of the line can be styled independently with the `runs` parameter. /// Generally, you should prefer to use `TextLayout::shape_line` instead, which /// can be painted directly. - pub fn layout_line( + pub fn layout_line( &self, - text: &str, + text: Text, font_size: Pixels, runs: &[TextRun], - ) -> Result> { + ) -> Result> + where + Text: AsRef, + SharedString: From, + { let mut font_runs = self.font_runs_pool.lock().pop().unwrap_or_default(); for run in runs.iter() { let font_id = self.resolve_font(&run.font); diff --git a/crates/gpui/src/text_system/line_layout.rs b/crates/gpui/src/text_system/line_layout.rs index 66eb914a30..a78c07fa26 100644 --- a/crates/gpui/src/text_system/line_layout.rs +++ b/crates/gpui/src/text_system/line_layout.rs @@ -1,4 +1,4 @@ -use crate::{point, px, FontId, GlyphId, Pixels, PlatformTextSystem, Point, Size}; +use crate::{point, px, FontId, GlyphId, Pixels, PlatformTextSystem, Point, SharedString, Size}; use collections::FxHashMap; use parking_lot::{Mutex, RwLock, RwLockUpgradableReadGuard}; use smallvec::SmallVec; @@ -420,15 +420,19 @@ impl LineLayoutCache { curr_frame.used_wrapped_lines.clear(); } - pub fn layout_wrapped_line( + pub fn layout_wrapped_line( &self, - text: &str, + text: Text, font_size: Pixels, runs: &[FontRun], wrap_width: Option, - ) -> Arc { + ) -> Arc + where + Text: AsRef, + SharedString: From, + { let key = &CacheKeyRef { - text, + text: text.as_ref(), font_size, runs, wrap_width, @@ -449,8 +453,8 @@ impl LineLayoutCache { layout } else { drop(current_frame); - - let unwrapped_layout = self.layout_line(text, font_size, runs); + let text = SharedString::from(text); + let unwrapped_layout = self.layout_line::<&SharedString>(&text, font_size, runs); let wrap_boundaries = if let Some(wrap_width) = wrap_width { unwrapped_layout.compute_wrap_boundaries(text.as_ref(), wrap_width) } else { @@ -462,7 +466,7 @@ impl LineLayoutCache { wrap_width, }); let key = Arc::new(CacheKey { - text: text.into(), + text, font_size, runs: SmallVec::from(runs), wrap_width, @@ -478,9 +482,18 @@ impl LineLayoutCache { } } - pub fn layout_line(&self, text: &str, font_size: Pixels, runs: &[FontRun]) -> Arc { + pub fn layout_line( + &self, + text: Text, + font_size: Pixels, + runs: &[FontRun], + ) -> Arc + where + Text: AsRef, + SharedString: From, + { let key = &CacheKeyRef { - text, + text: text.as_ref(), font_size, runs, wrap_width: None, @@ -497,9 +510,13 @@ impl LineLayoutCache { current_frame.used_lines.push(key); layout } else { - let layout = Arc::new(self.platform_text_system.layout_line(text, font_size, runs)); + let text = SharedString::from(text); + let layout = Arc::new( + self.platform_text_system + .layout_line(&text, font_size, runs), + ); let key = Arc::new(CacheKey { - text: text.into(), + text, font_size, runs: SmallVec::from(runs), wrap_width: None, @@ -524,7 +541,7 @@ trait AsCacheKeyRef { #[derive(Clone, Debug, Eq)] struct CacheKey { - text: String, + text: SharedString, font_size: Pixels, runs: SmallVec<[FontRun; 1]>, wrap_width: Option,