From b21a8541e73168ec02f00b49da25ad7a00654d65 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 07:26:18 -0500 Subject: [PATCH] Row auto-fit: sweep merges once per sheet instead of per row (#3195) * Row auto-fit: sweep merges once per sheet instead of per row auto_row_height() picked the merges touching a row by scanning every merge on the sheet, and opening a workbook (fit_imported_rows) or a commit fitting many rows called it per row, so rows x merges comparisons ran synchronously on open. Rows are fitted in ascending order, so a MergeSweep sorts the sheet's merges by first row once and keeps only those active on the current row. All comparisons count against MERGE_FIT_WORK (50,000,000) per pass; past it the remaining rows keep the heights they have. auto_row_height keeps its signature for single rows. * address review: share the merge-comparison budget across the whole auto-fit pass * address review: build the merge sweep lazily, after the row fast paths * address review: charge the merge sort to the fit budget, keep row heights once it is spent * address review: a pass that fits one row scans the merges instead of sorting them --- crates/engine/src/cmd.rs | 146 +++++++++++++++++++++++++- crates/engine/src/io.rs | 7 +- crates/engine/tests/it/row_autofit.rs | 28 +++++ 3 files changed, 177 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index a59bf9b2a..0f79495fc 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -451,8 +451,18 @@ pub(crate) fn commit(d: &mut DocState, f: impl FnOnce(&mut Ctx) -> Result) } fit_rows.sort_unstable(); fit_rows.dedup(); + let mut sweep: Option<(usize, MergeSweep)> = None; + let mut left = MERGE_FIT_WORK; for (si, row) in fit_rows { - auto_row_height(&mut wb, si, row); + if sweep.as_ref().is_none_or(|(at, _)| *at != si) { + if let Some((_, old)) = &sweep { + left = old.left(); + } + sweep = Some((si, MergeSweep::new(left))); + } + if let Some((_, sw)) = sweep.as_mut() { + fit_row_height(&mut wb, si, row, sw); + } } if let Some(t0) = t0 { eprintln!("commit: + rows {:?}", since(t0)); @@ -726,6 +736,94 @@ fn wrapped_line_count(line: &str, width: f32, size: f32, bold: bool) -> f32 { /// Line breaks in a cell's text count only when the cell wraps, as in Excel: with Wrap Text off /// a multi-line value shows on one line and AutoFit gives a one-line row. pub fn auto_row_height(wb: &mut Workbook, si: usize, row: u32) { + let mut sweep = MergeSweep::new(MERGE_FIT_WORK); + fit_row_height(wb, si, row, &mut sweep); +} + +/// Most merge comparisons one auto-fit pass (a commit, or opening a workbook) makes. Past it the +/// remaining rows keep the heights they have rather than stall on a sheet whose merges overlap +/// the fitted rows many times over. +pub(crate) const MERGE_FIT_WORK: usize = 50_000_000; + +/// The merged areas of one sheet that touch a row, for rows visited in ascending order: the +/// merges are sorted by first row once and swept, so fitting many rows never rescans every merge +/// per row. All comparisons count against [`MERGE_FIT_WORK`]. +pub(crate) struct MergeSweep { + /// Built on the first row that needs merge checks, so rows that return early never pay for it. + by_start: Option>, + next: usize, + active: Vec, + budget: usize, + /// The first row was answered by a plain scan: a pass that fits one row never pays for a sort. + scanned: bool, +} + +impl MergeSweep { + /// A sweep for one sheet's merges with `budget` comparisons left for the pass (carry + /// [`MergeSweep::left`] of the previous sheet's sweep into the next). Every call to + /// [`MergeSweep::at`] must pass that same sheet. + pub(crate) fn new(budget: usize) -> Self { + Self { by_start: None, next: 0, active: Vec::new(), budget, scanned: false } + } + + /// Comparisons left for the rest of the pass. + pub(crate) fn left(&self) -> usize { + self.budget + } + + /// Takes `n` comparisons from the budget; `false` once it is spent. + fn charge(&mut self, n: usize) -> bool { + match self.budget.checked_sub(n) { + Some(rest) => { + self.budget = rest; + true + } + None => { + self.budget = 0; + false + } + } + } + + /// The merges covering `row`, or `None` when the budget is spent. Rows must not decrease + /// between calls (skipping rows is fine). + fn at(&mut self, sh: &gridcraft_model::Sheet, row: u32) -> Option> { + if self.by_start.is_none() && !self.scanned { + self.scanned = true; + if !self.charge(sh.merges.len()) { + return None; + } + return Some(sh.merges.iter().copied().filter(|m| !m.is_single() && m.start.row <= row && row <= m.end.row).collect()); + } + if self.by_start.is_none() { + // Sorting is about n log2 n comparisons, charged before it starts. + let n = sh.merges.len(); + if !self.charge(n.saturating_mul((usize::BITS - n.leading_zeros()) as usize)) { + return None; + } + let mut v: Vec = sh.merges.iter().copied().filter(|m| !m.is_single()).collect(); + v.sort_unstable_by_key(|m| m.start.row); + self.by_start = Some(v); + } + let by_start = self.by_start.as_ref()?; + while let Some(m) = by_start.get(self.next).filter(|m| m.start.row <= row) { + self.active.push(*m); + self.next += 1; + } + if !self.charge(self.active.len()) { + return None; + } + self.active.retain(|m| m.end.row >= row); + Some(self.active.clone()) + } +} + +/// [`auto_row_height`] with the sheet's merges taken from `sweep`. +pub(crate) fn fit_row_height(wb: &mut Workbook, si: usize, row: u32, sweep: &mut MergeSweep) { + // Out of budget: the row keeps the height it has. + if sweep.left() == 0 { + return; + } let Some(sh) = wb.sheet(si) else { return }; if sh.rows.get(&row).is_some_and(|i| i.custom) { return; @@ -755,9 +853,14 @@ pub fn auto_row_height(wb: &mut Workbook, si: usize, row: u32) { // picked out once, and per cell the candidates are re-checked (normally none or a couple; // a sheet with many merges and many saved columns would otherwise scan all of them per // cell, rows × merges × columns). - let row_merges: Vec = - sh.merges.iter().copied().filter(|m| !m.is_single() && m.start.row <= row && row <= m.end.row).collect(); + // Rows are fitted in ascending order, so the merges touching this one come from a sweep over + // the sheet's merges sorted once, not from a scan of all of them. Out of budget: the row keeps + // the height it has. + let Some(row_merges) = sweep.at(sh, row) else { return }; for (c, cell) in sh.cells.row(row, 0, gridcraft_core::MAX_COLS - 1) { + if !sweep.charge(row_merges.len()) { + return; + } // Merged cells never auto-fit their rows, like Excel (column autofit skips them too). if row_merges.iter().any(|m| m.contains(CellRef::new(row, c))) { continue; @@ -801,3 +904,40 @@ impl Ctx<'_> { self.changed.push((sheet, c)); } } + +#[cfg(test)] +mod merge_sweep_tests { + use super::*; + + #[test] + fn the_budget_carries_from_one_sheets_sweep_to_the_next() { + let mut sheet = gridcraft_model::Sheet::new("S"); + for r in ["A1:B2", "C1:D2", "E1:F2"] { + sheet.merges.push(gridcraft_core::RangeRef::parse(r).unwrap()); + } + let mut first = MergeSweep::new(5); + assert!(first.at(&sheet, 0).is_some(), "the first row is a plain scan: three comparisons"); + assert!(first.at(&sheet, 1).is_none(), "the second row needs the sort: six comparisons, two are left"); + assert_eq!(first.left(), 0); + // The next sheet starts with what is left: nothing, so it fits nothing. + let mut second = MergeSweep::new(first.left()); + assert!(second.at(&sheet, 0).is_none()); + assert!(MergeSweep::new(MERGE_FIT_WORK).at(&sheet, 0).is_some()); + } + + #[test] + fn sorting_the_merges_is_charged() { + let mut sheet = gridcraft_model::Sheet::new("S"); + for r in ["A1:B2", "C1:D2", "E1:F2", "G1:H2"] { + sheet.merges.push(gridcraft_core::RangeRef::parse(r).unwrap()); + } + // The first row costs a plain scan (4); from the second on the merges are sorted, about + // 4 * 3 comparisons: 7 left is not enough, 12 plus the row's 4 is. + let mut sweep = MergeSweep::new(11); + assert!(sweep.at(&sheet, 0).is_some()); + assert!(sweep.at(&sheet, 1).is_none()); + assert_eq!(sweep.left(), 0); + let mut sweep = MergeSweep::new(20); + assert!(sweep.at(&sheet, 0).is_some() && sweep.at(&sheet, 1).is_some()); + } +} diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index 8b0522fb0..35420186a 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -100,14 +100,18 @@ pub fn open_bytes_with(name: &str, bytes: &[u8], password: Option<&str>, csv_as_ /// line-height estimate can differ from Excel's, and shrinking files that already look /// right would be a visible regression. Custom-height rows are left alone entirely. fn fit_imported_rows(wb: &mut Workbook) { + // The whole pass shares one budget of merge comparisons: later sheets keep their heights once it is spent. + let mut left = crate::cmd::MERGE_FIT_WORK; for si in 0..wb.sheets.len() { let Some(sh) = wb.sheet(si) else { continue }; let mut rows: Vec = sh.cells.iter().map(|(c, _)| c.row).collect(); rows.sort_unstable(); rows.dedup(); + // One sweep over the sheet's merges for all of its rows, in ascending order. + let mut sweep = crate::cmd::MergeSweep::new(left); for row in rows { let before = wb.sheet(si).and_then(|sh| sh.rows.get(&row)).and_then(|i| i.size); - crate::cmd::auto_row_height(wb, si, row); + crate::cmd::fit_row_height(wb, si, row, &mut sweep); let Some(before) = before else { continue }; let after = wb.sheet(si).and_then(|sh| sh.rows.get(&row)).and_then(|i| i.size); if after.is_none_or(|a| before > a + 0.5) @@ -116,6 +120,7 @@ fn fit_imported_rows(wb: &mut Workbook) { sh.rows.entry(row).or_default().size = Some(before); } } + left = sweep.left(); } } diff --git a/crates/engine/tests/it/row_autofit.rs b/crates/engine/tests/it/row_autofit.rs index 5bea5abca..438a39870 100644 --- a/crates/engine/tests/it/row_autofit.rs +++ b/crates/engine/tests/it/row_autofit.rs @@ -59,3 +59,31 @@ fn whole_row_format_keeps_hidden_rows_hidden() { assert!(info.size.unwrap_or(before) > before + 10.0, "hidden row still grows to its font: before={before}, size={:?}", info.size); assert!(info.hidden, "formatting a hidden row must not unhide it"); } + +#[test] +fn many_merges_fit_rows_the_same_and_skip_merged_cells() { + // Rows are fitted with one sweep over the sheet's merges; a merged cell still never grows + // its row, and rows next to thousands of merges still fit. + let mut s = gridcraft_engine::Session::new(); + s.new_workbook(); + let values: Vec<_> = (0..2_000).map(|i| json!([format!("v{i}")])).collect(); + s.execute("range.setValues", json!({"range": "A1", "values": values})).unwrap(); + { + let sh = sheet_mut(&mut s); + sh.merges.push(gridcraft_engine::core::RangeRef::parse("A3:B4").unwrap()); + for i in 0..20_000u32 { + let row = 4 + i % 1_990; + sh.merges.push(gridcraft_engine::core::RangeRef::new( + gridcraft_engine::core::CellRef::new(row, 3 + (i % 7) * 2), + gridcraft_engine::core::CellRef::new(row + 3, 4 + (i % 7) * 2), + )); + } + } + s.execute("home.fontSize", json!({"range": "A:A", "size": 20})).unwrap(); + let sh = s.doc().unwrap().wb.active().unwrap(); + let default = sh.default_row_height; + assert!(sh.row_height(0) > 30.0 && sh.row_height(1_999) > 30.0, "rows with big text grow"); + assert_eq!(sh.row_height(2), default, "a merged cell does not grow its row"); + assert_eq!(sh.row_height(3), default, "nor any row of the merge"); + assert!(sh.row_height(4) > 30.0 && sh.row_height(1_000) > 30.0, "rows beside other merges grow"); +}