From e861c8ec79e6af6e2e5623ea5b16f39397aaeeba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hubert=20G=C5=82uchowski?= Date: Fri, 31 Oct 2025 09:45:32 +0000 Subject: [PATCH] Avoid cloning `Row`s during `Galley::concat` (#7649) Moves `ends_with_newline` into `PlacedRow` to avoid clones during layout. I don't think there was a rationale stronger than "don't change too much" for not doing this in https://github.com/emilk/egui/pull/5411, so I should've just done this from the start. This was a significant part of the profile for text layout (as it cloned almost every `Row`, even though it only needed to change a single boolean). Before: image After: image (note that these profiles focus solely on the top-level `Galley::layout_inline` subtree, also don't compare sample count as the duration of these tests was completely arbitrary) egui_demo_lib `*text_layout*` benches: image * [X] I have followed the instructions in the PR template (As usual, the tests fail for me even on master but the failures on master and with these changes seem the same :)) --- crates/egui/src/text_selection/visuals.rs | 5 +-- crates/epaint/src/shapes/text_shape.rs | 8 +++-- crates/epaint/src/text/text_layout.rs | 13 ++++--- crates/epaint/src/text/text_layout_types.rs | 38 +++++++++++---------- 4 files changed, 35 insertions(+), 29 deletions(-) diff --git a/crates/egui/src/text_selection/visuals.rs b/crates/egui/src/text_selection/visuals.rs index e3054b19d..0f6d54abd 100644 --- a/crates/egui/src/text_selection/visuals.rs +++ b/crates/egui/src/text_selection/visuals.rs @@ -31,7 +31,8 @@ pub fn paint_text_selection( let max = galley.layout_from_cursor(max); for ri in min.row..=max.row { - let row = Arc::make_mut(&mut galley.rows[ri].row); + let placed_row = &mut galley.rows[ri]; + let row = Arc::make_mut(&mut placed_row.row); let left = if ri == min.row { row.x_offset(min.column) @@ -41,7 +42,7 @@ pub fn paint_text_selection( let right = if ri == max.row { row.x_offset(max.column) } else { - let newline_size = if row.ends_with_newline { + let newline_size = if placed_row.ends_with_newline { row.height() / 2.0 // visualize that we select the newline } else { 0.0 diff --git a/crates/epaint/src/shapes/text_shape.rs b/crates/epaint/src/shapes/text_shape.rs index 349707eac..92a0a0514 100644 --- a/crates/epaint/src/shapes/text_shape.rs +++ b/crates/epaint/src/shapes/text_shape.rs @@ -137,7 +137,12 @@ impl TextShape { *mesh_bounds = transform.scaling * *mesh_bounds; *intrinsic_size = transform.scaling * *intrinsic_size; - for text::PlacedRow { pos, row } in rows { + for text::PlacedRow { + pos, + row, + ends_with_newline: _, + } in rows + { *pos *= transform.scaling; let text::Row { @@ -145,7 +150,6 @@ impl TextShape { glyphs: _, // TODO(emilk): would it make sense to transform these? size, visuals, - ends_with_newline: _, } = Arc::make_mut(row); *size *= transform.scaling; diff --git a/crates/epaint/src/text/text_layout.rs b/crates/epaint/src/text/text_layout.rs index cf791351a..b1fe895da 100644 --- a/crates/epaint/src/text/text_layout.rs +++ b/crates/epaint/src/text/text_layout.rs @@ -296,8 +296,8 @@ fn rows_from_paragraphs( glyphs: vec![], visuals: Default::default(), size: vec2(0.0, paragraph.empty_paragraph_height), - ends_with_newline: !is_last_paragraph, }), + ends_with_newline: !is_last_paragraph, }); } else { let paragraph_max_x = paragraph.glyphs.last().unwrap().max_x(); @@ -310,14 +310,13 @@ fn rows_from_paragraphs( glyphs: paragraph.glyphs, visuals: Default::default(), size: vec2(paragraph_max_x, 0.0), - ends_with_newline: !is_last_paragraph, }), + ends_with_newline: !is_last_paragraph, }); } else { line_break(¶graph, job, &mut rows, elided); let placed_row = rows.last_mut().unwrap(); - let row = Arc::make_mut(&mut placed_row.row); - row.ends_with_newline = !is_last_paragraph; + placed_row.ends_with_newline = !is_last_paragraph; } } } @@ -363,8 +362,8 @@ fn line_break( glyphs: vec![], visuals: Default::default(), size: Vec2::ZERO, - ends_with_newline: false, }), + ends_with_newline: false, }); row_start_x += first_row_indentation; first_row_indentation = 0.0; @@ -389,8 +388,8 @@ fn line_break( glyphs, visuals: Default::default(), size: vec2(paragraph_max_x, 0.0), - ends_with_newline: false, }), + ends_with_newline: false, }); // Start a new row: @@ -431,8 +430,8 @@ fn line_break( glyphs, visuals: Default::default(), size: vec2(paragraph_max_x - paragraph_min_x, 0.0), - ends_with_newline: false, }), + ends_with_newline: false, }); } } diff --git a/crates/epaint/src/text/text_layout_types.rs b/crates/epaint/src/text/text_layout_types.rs index 1adcc515e..d87f9a579 100644 --- a/crates/epaint/src/text/text_layout_types.rs +++ b/crates/epaint/src/text/text_layout_types.rs @@ -572,6 +572,13 @@ pub struct PlacedRow { /// The underlying unpositioned [`Row`]. pub row: Arc, + + /// If true, this [`PlacedRow`] came from a paragraph ending with a `\n`. + /// The `\n` itself is omitted from row's [`Row::glyphs`]. + /// A `\n` in the input text always creates a new [`PlacedRow`] below it, + /// so that text that ends with `\n` has an empty [`PlacedRow`] last. + /// This also implies that the last [`PlacedRow`] in a [`Galley`] always has `ends_with_newline == false`. + pub ends_with_newline: bool, } impl PlacedRow { @@ -617,13 +624,6 @@ pub struct Row { /// The mesh, ready to be rendered. pub visuals: RowVisuals, - - /// If true, this [`Row`] came from a paragraph ending with a `\n`. - /// The `\n` itself is omitted from [`Self::glyphs`]. - /// A `\n` in the input text always creates a new [`Row`] below it, - /// so that text that ends with `\n` has an empty [`Row`] last. - /// This also implies that the last [`Row`] in a [`Galley`] always has `ends_with_newline == false`. - pub ends_with_newline: bool, } /// The tessellated output of a row. @@ -735,12 +735,6 @@ impl Row { self.glyphs.len() } - /// Includes the implicit `\n` after the [`Row`], if any. - #[inline] - pub fn char_count_including_newline(&self) -> usize { - self.glyphs.len() + (self.ends_with_newline as usize) - } - /// Closest char at the desired x coordinate in row-relative coordinates. /// Returns something in the range `[0, char_count_excluding_newline()]`. pub fn char_at(&self, desired_x: f32) -> usize { @@ -776,6 +770,12 @@ impl PlacedRow { pub fn max_y(&self) -> f32 { self.rect().bottom() } + + /// Includes the implicit `\n` after the [`PlacedRow`], if any. + #[inline] + pub fn char_count_including_newline(&self) -> usize { + self.row.glyphs.len() + (self.ends_with_newline as usize) + } } impl Galley { @@ -867,13 +867,15 @@ impl Galley { placed_row.visuals.mesh_bounds.translate(new_pos.to_vec2()); merged_galley.rect |= Rect::from_min_size(new_pos, placed_row.size); - let mut row = placed_row.row.clone(); + let mut ends_with_newline = placed_row.ends_with_newline; let is_last_row_in_galley = row_idx + 1 == galley.rows.len(); - if !is_last_galley && is_last_row_in_galley { - // Since we remove the `\n` when splitting rows, we need to add it back here - Arc::make_mut(&mut row).ends_with_newline = true; + // Since we remove the `\n` when splitting rows, we need to add it back here + ends_with_newline |= !is_last_galley && is_last_row_in_galley; + super::PlacedRow { + pos: new_pos, + row: placed_row.row.clone(), + ends_with_newline, } - super::PlacedRow { pos: new_pos, row } })); merged_galley.num_vertices += galley.num_vertices;