From 6c37a63704253075feadd4ffd1e253a19b64b779 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Mon, 26 Oct 2026 22:22:22 +0100 Subject: [PATCH 1/4] perf(calc): index the pass's spill areas so reads don't scan every spill (#2683) A cell read and a range read looked at every spill computed so far in the pass, and cells_changed looked at every spill range of the sheet for each changed cell. A spatial index (16 x 16 cell blocks) now finds the areas covering a cell or meeting a range. Where areas overlap, the one that counted before (first/last in the pass's spill map order) still counts. Fixes #2683 Co-Authored-By: Claude Sonnet 5.5 --- crates/calc/src/lib.rs | 1 + crates/calc/src/recalc.rs | 152 +++++++++++++++++++--------- crates/calc/src/spill_index.rs | 175 +++++++++++++++++++++++++++++++++ crates/calc/src/tests.rs | 29 ++++++ 4 files changed, 310 insertions(+), 47 deletions(-) create mode 100644 crates/calc/src/spill_index.rs diff --git a/crates/calc/src/lib.rs b/crates/calc/src/lib.rs index 27c9e1961..c71d8b7b1 100644 --- a/crates/calc/src/lib.rs +++ b/crates/calc/src/lib.rs @@ -5,6 +5,7 @@ pub mod eval; pub mod recalc; +mod spill_index; mod trimrange; pub use eval::{Area, SPECIAL_FUNCTIONS, is_known_function}; diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index 17a0e6649..d86e73fa5 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -45,6 +45,7 @@ use gridcraft_formula::Expr; use gridcraft_model::{CalcMode, Workbook}; use crate::eval::{Area, Evaluator, Host, precedents, stack_position}; +use crate::spill_index::{Cover, SpillIndex}; pub type Key = (usize, CellRef); @@ -393,6 +394,8 @@ struct PassHost<'a> { in_progress: HashSet, /// New spills found during the pass: anchor → array. spills: HashMap>, + /// Where the areas in `spills` are, so a read finds the ones it meets without scanning them all. + spill_index: SpillIndex, /// Used range per sheet: the workbook doesn't change during a pass, and working it out walks /// every row, so once per sheet rather than once per range read. used: HashMap>, @@ -447,6 +450,7 @@ impl<'a> PassHost<'a> { pending: exprs.keys().copied().collect(), in_progress: HashSet::default(), spills: HashMap::default(), + spill_index: SpillIndex::default(), used: HashMap::default(), rng, now, @@ -472,6 +476,33 @@ impl<'a> PassHost<'a> { } } +/// Most spill areas a range read meets before they are checked for overlapping one another by +/// walking all of the pass's spills instead of comparing pairs. +/// Changed cells (in one `cells_changed`) from which the sheet's spill areas are indexed. +const INDEX_SPILLS_FROM: usize = 16; + +const MAX_UNORDERED_HITS: usize = 16; + +/// Whether an array at `anchor` covers `c`. +fn spill_covers(anchor: CellRef, arr: &Array, c: CellRef) -> bool { + c.row >= anchor.row && c.col >= anchor.col && ((c.row - anchor.row) as usize) < arr.rows && ((c.col - anchor.col) as usize) < arr.cols +} + +/// Whether two of the spill areas anchored at `hits` share a cell inside `range`. +fn spill_hits_overlap(index: &SpillIndex, sheet: usize, hits: &[CellRef], range: RangeRef) -> bool { + let rects: Vec = hits.iter().filter_map(|a| index.area(sheet, *a)).filter(|(_, empty)| !empty).map(|(r, _)| r).collect(); + rects.iter().enumerate().any(|(i, a)| { + rects.iter().skip(i + 1).any(|b| { + a.intersects(b) + && RangeRef::new( + CellRef::new(a.start.row.max(b.start.row), a.start.col.max(b.start.col)), + CellRef::new(a.end.row.min(b.end.row), a.end.col.min(b.end.col)), + ) + .intersects(&range) + }) + }) +} + /// Whether an array result at `k` can't spill: it is in a table, or its area runs off the sheet, /// overlaps merged cells, holds other cells, or another formula's spill (unless that formula is /// recalculated in this pass, and so may spill elsewhere now). @@ -659,6 +690,7 @@ impl PassHost<'_> { // value -- a FILTER matching one row, a SORT or UNIQUE over one item -- so `C1#` // is the 1 x 1 range and `ROWS(C1#)` is 1, as in Excel (#1491). A formula that // returns a scalar never reaches this arm, so it does not become an anchor. + self.spill_index.insert(k, a.rows, a.cols); self.spills.insert(k, a.clone()); a.data.first().cloned().unwrap_or_default() } @@ -669,6 +701,7 @@ impl PassHost<'_> { let v = match self.spills.get(&k) { Some(a) if spill_blocked(self, k, a) => { self.spills.remove(&k); + self.spill_index.remove(k); Value::Error(CellError::Spill) } _ => v, @@ -715,7 +748,11 @@ impl PassHost<'_> { while passes < max_iterations.min(max_passes) && !self.converged(max_change) { self.prev = again.iter().filter_map(|k| self.results.remove(k).map(|v| (*k, v))).collect(); self.pending = set.clone(); - self.spills.retain(|a, _| !set.contains(a)); + let gone: Vec = self.spills.keys().filter(|a| set.contains(a)).copied().collect(); + for a in gone { + self.spills.remove(&a); + self.spill_index.remove(a); + } for k in &again { self.calculate(*k); } @@ -799,17 +836,19 @@ impl Host for PassHost<'_> { if self.pending.contains(&k) || self.results.contains_key(&k) { return self.compute(k); } - // A cell covered by a spill computed in this pass. - for (anchor, arr) in &self.spills { - if anchor.0 == sheet && c.row >= anchor.1.row && c.col >= anchor.1.col { - let (dr, dc) = ((c.row - anchor.1.row) as usize, (c.col - anchor.1.col) as usize); - if dr < arr.rows && dc < arr.cols { - if self.iterate && self.tainted.contains(anchor) { - self.taint = true; - } - return arr.get(dr, dc).cloned().unwrap_or_default(); - } + // A cell covered by a spill computed in this pass. Where several overlap, the first in + // `spills`' iteration order counts, as it always has. + let covering = match self.spill_index.covering(sheet, c) { + Cover::None => None, + Cover::One(a) => self.spills.get_key_value(&(sheet, a)), + Cover::Many => self.spills.iter().find(|(a, arr)| a.0 == sheet && spill_covers(a.1, arr, c)), + }; + if let Some((anchor, arr)) = covering { + let (dr, dc) = ((c.row - anchor.1.row) as usize, (c.col - anchor.1.col) as usize); + if self.iterate && self.tainted.contains(anchor) { + self.taint = true; } + return arr.get(dr, dc).cloned().unwrap_or_default(); } self.wb.sheet(sheet).map(|s| s.value(c)).unwrap_or_default() } @@ -845,41 +884,44 @@ impl Host for PassHost<'_> { } } if !self.spills.is_empty() { - let new: Vec<(usize, Value)> = self - .spills - .iter() - .filter(|(a, _)| a.0 == sheet) - .flat_map(|(a, arr)| { - let mut v = Vec::new(); - for r in 0..arr.rows { - for cc in 0..arr.cols { - let c = CellRef::new(a.1.row.saturating_add(r as u32), a.1.col.saturating_add(cc as u32)); - if c != a.1 && range.contains(c) { - v.push(( - (c.row - range.start.row) as usize * w + (c.col - range.start.col) as usize, - arr.get(r, cc).cloned().unwrap_or_default(), - )); - } - } - } - v - }) - .collect(); + let mut hits = Vec::new(); + self.spill_index.meeting(sheet, range, &mut hits); // Iterating: an array spilled by a formula that read a previous value. - if self.iterate - && self.spills.iter().any(|(a, arr)| { - let end = CellRef::new( - a.1.row.saturating_add((arr.rows as u32).saturating_sub(1)), - a.1.col.saturating_add((arr.cols as u32).saturating_sub(1)), - ); - a.0 == sheet && self.tainted.contains(a) && RangeRef::new(a.1, end).intersects(&range) - }) - { + if self.iterate && hits.iter().any(|a| self.tainted.contains(&(sheet, *a))) { self.taint = true; } - for (i, v) in new { - if let Some(slot) = out.get_mut(i) { - *slot = v; + // Where areas overlap inside the range, the last in `spills`' iteration order wins, + // as it always has; otherwise the order doesn't matter. + let ordered = hits.len() > MAX_UNORDERED_HITS || spill_hits_overlap(&self.spill_index, sheet, &hits, range); + let mut put = |anchor: CellRef, arr: &Array| { + if arr.rows == 0 || arr.cols == 0 { + return; + } + let (r0, c0) = (range.start.row.max(anchor.row), range.start.col.max(anchor.col)); + let r1 = range.end.row.min(anchor.row.saturating_add((arr.rows as u32).saturating_sub(1))); + let c1 = range.end.col.min(anchor.col.saturating_add((arr.cols as u32).saturating_sub(1))); + for r in r0..=r1 { + for cc in c0..=c1 { + if (r, cc) != (anchor.row, anchor.col) + && let Some(slot) = out.get_mut((r - range.start.row) as usize * w + (cc - range.start.col) as usize) + { + *slot = arr.get((r - anchor.row) as usize, (cc - anchor.col) as usize).cloned().unwrap_or_default(); + } + } + } + }; + if ordered { + let wanted: HashSet = hits.iter().map(|a| (sheet, *a)).collect(); + for (a, arr) in &self.spills { + if wanted.contains(a) { + put(a.1, arr); + } + } + } else { + for a in &hits { + if let Some(arr) = self.spills.get(&(sheet, *a)) { + put(*a, arr); + } } } } @@ -898,7 +940,7 @@ impl Host for PassHost<'_> { } // Spilled values sit between the stored cells (and the formulas just calculated may have // produced some): leave such blocks to `range_values`. - if self.spills.keys().any(|a| a.0 == sheet) || sh.spill.range(range.start..=range.end).any(|(c, _)| range.contains(*c)) { + if self.spill_index.has_sheet(sheet) || sh.spill.range(range.start..=range.end).any(|(c, _)| range.contains(*c)) { return false; } for (c, cell) in sh.cells.iter_range(range) { @@ -1124,11 +1166,27 @@ impl Calc { let mut emptied: Vec = Vec::new(); // Typing into (or clearing) a cell a dynamic array spills over (or would spill over) // re-evaluates the anchor, which may now be blocked or unblocked. + // With many changed cells (a paste) the sheet's spill areas are indexed once instead of + // being walked for each one. + let mut areas = SpillIndex::default(); + let mut indexed: HashSet = HashSet::default(); + let mut at = Vec::new(); for &k in changed { if let Some(sh) = wb.sheet(k.0) { - for (anchor, r) in &sh.spill_ranges { - if *anchor != k.1 && r.contains(k.1) { - seeds.push((k.0, *anchor)); + if changed.len() > INDEX_SPILLS_FROM && !sh.spill_ranges.is_empty() { + if indexed.insert(k.0) { + for (anchor, r) in &sh.spill_ranges { + areas.insert_rect((k.0, *anchor), *r, false); + } + } + // Anchor order, as walking `spill_ranges` gives. + areas.meeting(k.0, RangeRef::new(k.1, k.1), &mut at); + seeds.extend(at.iter().filter(|a| **a != k.1).map(|a| (k.0, *a))); + } else { + for (anchor, r) in &sh.spill_ranges { + if *anchor != k.1 && r.contains(k.1) { + seeds.push((k.0, *anchor)); + } } } for (c, cell) in sh.cells.iter_range(RangeRef::new(CellRef::new(k.1.row.saturating_sub(64), k.1.col.saturating_sub(64)), k.1)) { diff --git a/crates/calc/src/spill_index.rs b/crates/calc/src/spill_index.rs new file mode 100644 index 000000000..9a50996a3 --- /dev/null +++ b/crates/calc/src/spill_index.rs @@ -0,0 +1,175 @@ +//! A spatial index over spill areas: which areas cover a cell, and which meet a range, without +//! looking at every area. Areas are registered in 16 x 16 cell blocks, so a lookup touches one +//! block (a cell) or the blocks a range covers (a range) rather than every spill on the sheet. + +use crate::recalc::{FxHasher, Key}; +use gridcraft_core::{CellRef, RangeRef}; +use std::hash::BuildHasherDefault; + +type HashMap = std::collections::HashMap>; + +/// Block edge in cells (rows and columns). +const BLOCK: u32 = 16; + +/// One indexed area: its rectangle, and whether it covers no cells (an array with no rows or no +/// columns still meets the ranges its anchor is in, as a 1 x 1 area does for taint tracking). +#[derive(Clone, Copy)] +struct Entry { + rect: RangeRef, + empty: bool, +} + +#[derive(Default)] +struct SheetAreas { + entries: HashMap, + blocks: HashMap<(u32, u32), Vec>, +} + +/// What covers a cell. +pub(crate) enum Cover { + None, + One(CellRef), + /// Several areas overlap here: the caller decides which one counts. + Many, +} + +#[derive(Default)] +pub(crate) struct SpillIndex { + sheets: HashMap, +} + +fn block_span(rect: &RangeRef) -> (u32, u32, u32, u32) { + (rect.start.row / BLOCK, rect.end.row / BLOCK, rect.start.col / BLOCK, rect.end.col / BLOCK) +} + +impl SpillIndex { + /// Whether any area is indexed on `sheet`. + pub(crate) fn has_sheet(&self, sheet: usize) -> bool { + self.sheets.get(&sheet).is_some_and(|s| !s.entries.is_empty()) + } + + /// Indexes the area an array of `rows` x `cols` anchored at `anchor` covers (replacing the + /// one already indexed for that anchor). + pub(crate) fn insert(&mut self, (sheet, anchor): Key, rows: usize, cols: usize) { + let end = + CellRef::new(anchor.row.saturating_add((rows as u32).saturating_sub(1)), anchor.col.saturating_add((cols as u32).saturating_sub(1))); + self.insert_rect((sheet, anchor), RangeRef::new(anchor, end), rows == 0 || cols == 0); + } + + /// Indexes an area given as a range. + pub(crate) fn insert_rect(&mut self, (sheet, anchor): Key, rect: RangeRef, empty: bool) { + self.remove((sheet, anchor)); + let areas = self.sheets.entry(sheet).or_default(); + let (r0, r1, c0, c1) = block_span(&rect); + for br in r0..=r1 { + for bc in c0..=c1 { + areas.blocks.entry((br, bc)).or_default().push(anchor); + } + } + areas.entries.insert(anchor, Entry { rect, empty }); + } + + pub(crate) fn remove(&mut self, (sheet, anchor): Key) { + let Some(areas) = self.sheets.get_mut(&sheet) else { return }; + let Some(old) = areas.entries.remove(&anchor) else { return }; + let (r0, r1, c0, c1) = block_span(&old.rect); + for br in r0..=r1 { + for bc in c0..=c1 { + if let Some(list) = areas.blocks.get_mut(&(br, bc)) { + list.retain(|a| *a != anchor); + if list.is_empty() { + areas.blocks.remove(&(br, bc)); + } + } + } + } + } + + /// The areas that cover `c`. + pub(crate) fn covering(&self, sheet: usize, c: CellRef) -> Cover { + let Some(areas) = self.sheets.get(&sheet) else { return Cover::None }; + let Some(list) = areas.blocks.get(&(c.row / BLOCK, c.col / BLOCK)) else { return Cover::None }; + let mut found = Cover::None; + for a in list { + if let Some(e) = areas.entries.get(a) + && !e.empty + && e.rect.contains(c) + { + found = match found { + Cover::None => Cover::One(*a), + _ => return Cover::Many, + }; + } + } + found + } + + /// The anchors of the areas whose rectangle meets `range` (an empty area counts through its + /// anchor cell), sorted by anchor. + pub(crate) fn meeting(&self, sheet: usize, range: RangeRef, out: &mut Vec) { + out.clear(); + let Some(areas) = self.sheets.get(&sheet) else { return }; + let (r0, r1, c0, c1) = block_span(&range); + let touched = u64::from(r1 - r0 + 1) * u64::from(c1 - c0 + 1); + if touched > 4 * areas.entries.len() as u64 + 16 { + // A range wider than the areas are many: look at each area once. + out.extend(areas.entries.iter().filter(|(_, e)| e.rect.intersects(&range)).map(|(a, _)| *a)); + } else { + for br in r0..=r1 { + for bc in c0..=c1 { + if let Some(list) = areas.blocks.get(&(br, bc)) { + out.extend(list.iter().copied().filter(|a| areas.entries.get(a).is_some_and(|e| e.rect.intersects(&range)))); + } + } + } + } + out.sort_unstable(); + out.dedup(); + } + + /// The rectangle indexed for `anchor` and whether it covers no cells. + pub(crate) fn area(&self, sheet: usize, anchor: CellRef) -> Option<(RangeRef, bool)> { + self.sheets.get(&sheet)?.entries.get(&anchor).map(|e| (e.rect, e.empty)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn at(r: u32, c: u32) -> CellRef { + CellRef::new(r, c) + } + + #[test] + fn finds_the_areas_covering_a_cell_and_meeting_a_range() { + let mut ix = SpillIndex::default(); + ix.insert((0, at(0, 0)), 40, 3); + ix.insert((0, at(30, 1)), 1, 40); + ix.insert((1, at(0, 0)), 5, 5); + assert!(matches!(ix.covering(0, at(39, 2)), Cover::One(a) if a == at(0, 0))); + assert!(matches!(ix.covering(0, at(30, 2)), Cover::Many)); + assert!(matches!(ix.covering(0, at(40, 0)), Cover::None)); + assert!(matches!(ix.covering(2, at(0, 0)), Cover::None)); + let mut hits = Vec::new(); + ix.meeting(0, RangeRef::new(at(30, 30), at(50, 60)), &mut hits); + assert_eq!(hits, vec![at(30, 1)]); + ix.meeting(0, RangeRef::new(at(0, 0), at(1_000_000, 16_000)), &mut hits); + assert_eq!(hits, vec![at(0, 0), at(30, 1)]); + ix.remove((0, at(30, 1))); + assert!(matches!(ix.covering(0, at(30, 2)), Cover::One(_))); + assert!(ix.has_sheet(1) && !ix.has_sheet(2)); + ix.insert((0, at(0, 0)), 1, 1); + assert!(matches!(ix.covering(0, at(39, 2)), Cover::None)); + } + + #[test] + fn an_area_without_cells_covers_none_but_is_met_at_its_anchor() { + let mut ix = SpillIndex::default(); + ix.insert((0, at(3, 3)), 0, 4); + assert!(matches!(ix.covering(0, at(3, 3)), Cover::None)); + let mut hits = Vec::new(); + ix.meeting(0, RangeRef::new(at(3, 3), at(3, 3)), &mut hits); + assert_eq!(hits, vec![at(3, 3)]); + } +} diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 5b9e90085..a90a997ec 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -2274,3 +2274,32 @@ fn loan_schedule_residue_keeps_excels_sign() { } assert_eq!(t.get("P7"), Value::Text("".into())); } + +#[test] +fn many_spills_are_read_and_blocked_by_a_paste() { + let mut t = T::new(); + // 40 arrays of 3 rows side by side in rows 1-3, and a reader of a range over two of them. + for i in 0..40u32 { + let anchor = CellRef::new(0, 2 * i); + t.wb.sheet_mut(0).unwrap().cells.set(anchor, Cell::formula(Formula::new(&format!("=SEQUENCE(3,1,{})", 10 * i)))); + } + t.set("A10", "=SUM(C1:F3)"); + t.set("A12", "=COUNT(C1:F3)"); + t.set("A11", "=C2*1"); + t.calc.recalc_all(&mut t.wb); + assert_eq!(t.num("A10"), (10 + 11 + 12 + 20 + 21 + 22) as f64); + assert_eq!(t.num("A11"), 11.0); + assert_eq!(t.num("A12"), 6.0); + // Pasting 20 cells (more than one spill's width worth, so the changed set is indexed) over + // the second row of every other array blocks those arrays; the rest are unchanged. + let changed: Vec<(usize, CellRef)> = (0..20u32).map(|i| (0, CellRef::new(1, 4 * i))).collect(); + for k in &changed { + t.wb.sheet_mut(0).unwrap().cells.set(k.1, Cell::value(Value::Number(1.0))); + } + t.calc.cells_changed(&mut t.wb, &changed); + assert_eq!(t.get("E1"), Value::Error(CellError::Spill)); + assert_eq!(t.get("C1"), Value::Number(10.0)); + assert_eq!(t.get("G2"), Value::Number(31.0)); + assert_eq!(t.get("A10"), Value::Error(CellError::Spill)); + assert_eq!(t.num("A12"), 4.0); +} From 6c912d2eb130130f93d9281baca435a37e5d30e7 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Thu, 29 Oct 2026 23:48:11 +0100 Subject: [PATCH 2/4] perf(calc): keep tall and wide spill areas out of the block index A full-height column spill took 65,536 block registrations, on every pass that produced it. Areas over 1024 blocks are now tested directly by each lookup instead. Adds a randomized comparison against a scan of all areas. Co-Authored-By: Claude Sonnet 5.5 --- crates/calc/src/spill_index.rs | 121 +++++++++++++++++++++++++++++---- crates/calc/src/tests.rs | 21 ++++++ 2 files changed, 127 insertions(+), 15 deletions(-) diff --git a/crates/calc/src/spill_index.rs b/crates/calc/src/spill_index.rs index 7cf2d44fe..1bbf0f9c0 100644 --- a/crates/calc/src/spill_index.rs +++ b/crates/calc/src/spill_index.rs @@ -1,6 +1,9 @@ //! A spatial index over spill areas: which areas cover a cell, and which meet a range, without //! looking at every area. Areas are registered in 16 x 16 cell blocks, so a lookup touches one //! block (a cell) or the blocks a range covers (a range) rather than every spill on the sheet. +//! An area that would take more than `BIG_AREA_BLOCKS` blocks (a full-height column spill takes +//! 65,536) is not registered in blocks but kept in a short list that every lookup tests directly, +//! so registering costs what the area's size warrants only for small areas. use crate::recalc::{FxHasher, Key}; use gridcraft_core::{CellRef, RangeRef}; @@ -11,6 +14,9 @@ type HashMap = std::collections::HashMap, blocks: HashMap<(u32, u32), Vec>, + /// Anchors of the areas too big to register in blocks. + big: Vec, } /// What covers a cell. @@ -42,16 +50,24 @@ fn block_span(rect: &RangeRef) -> (u32, u32, u32, u32) { (rect.start.row / BLOCK, rect.end.row / BLOCK, rect.start.col / BLOCK, rect.end.col / BLOCK) } +fn block_count(rect: &RangeRef) -> u64 { + let (r0, r1, c0, c1) = block_span(rect); + u64::from(r1 - r0 + 1) * u64::from(c1 - c0 + 1) +} + /// Whether registering `areas` in blocks costs less than checking each of them against each of /// `lookups` cells would: a few block entries cost about one containment check each. A tall or -/// wide area (a full-height column spill takes 65,536 blocks) is cheaper to test directly. +/// wide area is not registered at all (see `BIG_AREA_BLOCKS`) and so costs nothing here. pub(crate) fn indexing_pays<'a>(areas: impl ExactSizeIterator, lookups: usize) -> bool { const BLOCK_COST: u64 = 16; let budget = (lookups as u64).saturating_mul(areas.len() as u64) / BLOCK_COST; let mut blocks = 0u64; for r in areas { - let (r0, r1, c0, c1) = block_span(r); - blocks = blocks.saturating_add(u64::from(r1 - r0 + 1) * u64::from(c1 - c0 + 1)); + let n = block_count(r); + if n > BIG_AREA_BLOCKS { + continue; + } + blocks = blocks.saturating_add(n); if blocks > budget { return false; } @@ -77,10 +93,14 @@ impl SpillIndex { pub(crate) fn insert_rect(&mut self, (sheet, anchor): Key, rect: RangeRef, empty: bool) { self.remove((sheet, anchor)); let areas = self.sheets.entry(sheet).or_default(); - let (r0, r1, c0, c1) = block_span(&rect); - for br in r0..=r1 { - for bc in c0..=c1 { - areas.blocks.entry((br, bc)).or_default().push(anchor); + if block_count(&rect) > BIG_AREA_BLOCKS { + areas.big.push(anchor); + } else { + let (r0, r1, c0, c1) = block_span(&rect); + for br in r0..=r1 { + for bc in c0..=c1 { + areas.blocks.entry((br, bc)).or_default().push(anchor); + } } } areas.entries.insert(anchor, Entry { rect, empty }); @@ -89,6 +109,10 @@ impl SpillIndex { pub(crate) fn remove(&mut self, (sheet, anchor): Key) { let Some(areas) = self.sheets.get_mut(&sheet) else { return }; let Some(old) = areas.entries.remove(&anchor) else { return }; + if block_count(&old.rect) > BIG_AREA_BLOCKS { + areas.big.retain(|a| *a != anchor); + return; + } let (r0, r1, c0, c1) = block_span(&old.rect); for br in r0..=r1 { for bc in c0..=c1 { @@ -105,9 +129,9 @@ impl SpillIndex { /// The areas that cover `c`. pub(crate) fn covering(&self, sheet: usize, c: CellRef) -> Cover { let Some(areas) = self.sheets.get(&sheet) else { return Cover::None }; - let Some(list) = areas.blocks.get(&(c.row / BLOCK, c.col / BLOCK)) else { return Cover::None }; + let in_block = areas.blocks.get(&(c.row / BLOCK, c.col / BLOCK)).map_or(&[][..], |l| l.as_slice()); let mut found = Cover::None; - for a in list { + for a in in_block.iter().chain(&areas.big) { if let Some(e) = areas.entries.get(a) && !e.empty && e.rect.contains(c) @@ -139,6 +163,7 @@ impl SpillIndex { } } } + out.extend(areas.big.iter().copied().filter(|a| areas.entries.get(a).is_some_and(|e| e.rect.intersects(&range)))); } out.sort_unstable(); out.dedup(); @@ -181,15 +206,81 @@ mod tests { } #[test] - fn tall_or_wide_areas_are_not_worth_indexing_for_a_small_paste() { + fn tall_or_wide_areas_are_not_registered_in_blocks() { let column = RangeRef::new(at(0, 0), at(1_048_575, 0)); - assert!(!indexing_pays([column].iter(), 17)); - assert!(!indexing_pays([column].iter(), 1_000)); - // Many small areas pay back for a paste of a hundred cells. + assert!(block_count(&column) > BIG_AREA_BLOCKS); + let mut ix = SpillIndex::default(); + ix.insert_rect((0, at(0, 0)), column, false); + assert!(ix.sheets[&0].blocks.is_empty()); + assert!(matches!(ix.covering(0, at(900_000, 0)), Cover::One(a) if a == at(0, 0))); + assert!(matches!(ix.covering(0, at(900_000, 1)), Cover::None)); + let mut hits = Vec::new(); + ix.meeting(0, RangeRef::new(at(5, 0), at(6, 3)), &mut hits); + assert_eq!(hits, vec![at(0, 0)]); + ix.remove((0, at(0, 0))); + assert!(matches!(ix.covering(0, at(900_000, 0)), Cover::None) && ix.sheets[&0].big.is_empty()); + // Registering a big area costs nothing (lookups test it directly), so a small paste still indexes next to many small areas. + assert!(indexing_pays([column].iter(), 17)); + // Many small areas pay back for a paste of a hundred cells, but not for a handful. let small: Vec = (0..1_000).map(|i| RangeRef::new(at(i * 3, 0), at(i * 3 + 2, 0))).collect(); assert!(indexing_pays(small.iter(), 100)); - // A big area among many areas is paid for by a big paste. - assert!(indexing_pays([column].iter(), 2_000_000)); + assert!(!indexing_pays(small.iter(), 17)); + } + + /// The index against the old way: test every area for every lookup. Mixed shapes: small, + /// thin and tall, wide, big (kept out of the blocks), empty, on two sheets, replaced and removed. + #[test] + fn matches_a_scan_of_every_area_on_mixed_shapes() { + let mut seed = 0x2545_F491_4F6C_DD1Du64; + let mut next = |n: u32| { + seed ^= seed << 13; + seed ^= seed >> 7; + seed ^= seed << 17; + (seed >> 11) as u32 % n + }; + let mut ix = SpillIndex::default(); + let mut model: std::collections::BTreeMap<(usize, CellRef), (RangeRef, bool)> = Default::default(); + for step in 0..3_000 { + let sheet = next(2) as usize; + let anchor = at(next(400) * 5, next(40) * 3); + if next(4) == 0 { + ix.remove((sheet, anchor)); + model.remove(&(sheet, anchor)); + } else { + let (rows, cols) = match next(8) { + 0 => (0, 1 + next(5) as usize), + 1 => (1 + next(5) as usize, 0), + 2 => (5_000 + next(100_000) as usize, 1), + 3 => (1, 1 + next(40_000) as usize), + 4 => (200 + next(300) as usize, 40 + next(40) as usize), + _ => (1 + next(60) as usize, 1 + next(40) as usize), + }; + ix.insert((sheet, anchor), rows, cols); + let end = at(anchor.row + (rows as u32).saturating_sub(1), anchor.col + (cols as u32).saturating_sub(1)); + model.insert((sheet, anchor), (RangeRef::new(anchor, end), rows == 0 || cols == 0)); + } + if step % 7 != 0 { + continue; + } + let sheet = next(3) as usize; + for _ in 0..20 { + let cell = at(next(2_100), next(130)); + let want: Vec = + model.iter().filter(|((s, _), (r, e))| *s == sheet && !e && r.contains(cell)).map(|((_, a), _)| *a).collect(); + match ix.covering(sheet, cell) { + Cover::None => assert!(want.is_empty(), "{cell:?}"), + Cover::One(a) => assert_eq!(want, vec![a], "{cell:?}"), + Cover::Many => assert!(want.len() > 1, "{cell:?}"), + } + let (r0, c0) = (next(2_100), next(130)); + let (tall, wide) = (if next(5) == 0 { 3_000 } else { 40 }, if next(5) == 0 { 130 } else { 20 }); + let range = RangeRef::new(at(r0, c0), at(r0 + next(tall), c0 + next(wide))); + let want: Vec = model.iter().filter(|((s, _), (r, _))| *s == sheet && r.intersects(&range)).map(|((_, a), _)| *a).collect(); + let mut got = Vec::new(); + ix.meeting(sheet, range, &mut got); + assert_eq!(got, want, "{range:?}"); + } + } } #[test] diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index 9dd1fd925..ac254239a 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -2350,3 +2350,24 @@ fn many_spills_are_read_and_blocked_by_a_paste() { assert_eq!(t.get("A10"), Value::Error(CellError::Spill)); assert_eq!(t.num("A12"), 4.0); } + +#[test] +fn a_paste_over_small_and_tall_spills_blocks_those_it_touches() { + let mut t = T::new(); + for i in 0..40u32 { + t.wb.sheet_mut(0).unwrap().cells.set(CellRef::new(0, 2 * i), Cell::formula(Formula::new("=SEQUENCE(3)"))); + } + t.wb.sheet_mut(0).unwrap().cells.set(CellRef::new(0, 200), Cell::formula(Formula::new("=SEQUENCE(1048576)"))); + t.calc.recalc_all(&mut t.wb); + // 20 cells: the middle row of the small arrays in columns 0, 8, 16 ... 72 (columns 80 and up hit + // nothing), and one deep inside the tall one. + let mut cells: Vec<(u32, u32)> = (0..19u32).map(|i| (1, 8 * i)).collect(); + cells.push((700_000, 200)); + paste_into(&mut t, &cells); + assert_eq!(t.get("A1"), Value::Error(CellError::Spill)); + assert_eq!(t.get("GS1"), Value::Error(CellError::Spill)); + assert_eq!(t.num("C1"), 1.0); + assert_eq!(t.num("C3"), 3.0); + assert_eq!(t.get("I1"), Value::Error(CellError::Spill)); + assert_eq!(t.num("E3"), 3.0); +} From bdbd29e623c7ddf4df27331a7f7834295b5f770c Mon Sep 17 00:00:00 2001 From: vangel76 Date: Fri, 30 Oct 2026 00:04:25 +0100 Subject: [PATCH 3/4] perf(calc): size the spill-index decision by the sheet's own changed cells A batch spread over several sheets used its total length for every sheet, so a one-cell sheet with many spills built the block index as if it had the whole batch's lookups. Co-Authored-By: Claude Sonnet 5.5 --- crates/calc/src/recalc.rs | 44 +++++++++++++++++++++++++++++---------- crates/calc/src/tests.rs | 22 ++++++++++++++++++++ 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index aae212dd3..7c3a6f361 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -22,6 +22,13 @@ use crate::spill_index::{SpillIndex, indexing_pays}; /// Changed cells (in one `cells_changed`) from which a sheet's spill areas may be indexed. const INDEX_SPILLS_FROM: usize = 16; +/// Whether a sheet's spill areas are indexed for a batch of `batch` changed cells of which +/// `on_sheet` are on that sheet: the batch must be a paste, and registering the areas must cost +/// less than walking them once for each of the sheet's own changed cells. +fn index_spills<'a>(batch: usize, on_sheet: usize, areas: impl ExactSizeIterator) -> bool { + batch > INDEX_SPILLS_FROM && areas.len() > 0 && indexing_pays(areas, on_sheet) +} + mod graph; mod pass_host; @@ -250,20 +257,22 @@ impl Calc { // walked for each one, when registering them costs less than walking them would. let mut areas = SpillIndex::default(); let mut strategy: HashMap = HashMap::default(); + let mut per_sheet: HashMap = HashMap::default(); + for k in changed { + *per_sheet.entry(k.0).or_insert(0) += 1; + } let mut at = Vec::new(); for &k in changed { if let Some(sh) = wb.sheet(k.0) { - let indexed = changed.len() > INDEX_SPILLS_FROM - && !sh.spill_ranges.is_empty() - && *strategy.entry(k.0).or_insert_with(|| { - let indexed = indexing_pays(sh.spill_ranges.values(), changed.len()); - if indexed { - for (anchor, r) in &sh.spill_ranges { - areas.insert_rect((k.0, *anchor), *r, false); - } + let indexed = *strategy.entry(k.0).or_insert_with(|| { + let indexed = index_spills(changed.len(), per_sheet[&k.0], sh.spill_ranges.values()); + if indexed { + for (anchor, r) in &sh.spill_ranges { + areas.insert_rect((k.0, *anchor), *r, false); } - indexed - }); + } + indexed + }); if indexed { // Anchor order, as walking `spill_ranges` gives. areas.meeting(k.0, RangeRef::new(k.1, k.1), &mut at); @@ -556,7 +565,7 @@ pub(crate) mod tests { use gridcraft_core::{CellRef, RangeRef, Value}; use gridcraft_model::{Cell, Formula, Workbook}; - use super::{Calc, HashMap, Host, Intervals, Key, PassHost}; + use super::{Calc, HashMap, Host, Intervals, Key, PassHost, index_spills}; /// xorshift64 for repeatable pseudo-random cases. fn rng(state: &mut u64) -> u64 { @@ -573,6 +582,19 @@ pub(crate) mod tests { v } + /// One changed cell on a sheet with many small spills does not index them just because the + /// batch (mostly on another sheet) is large; the sheet's own share of the batch decides. + #[test] + fn the_spill_index_is_chosen_from_the_sheets_own_changed_cells() { + let small: Vec = (0..1_000).map(|i| RangeRef::new(CellRef::new(i * 3, 0), CellRef::new(i * 3 + 2, 0))).collect(); + // Same areas, same batch of 5000: indexed when 5000 of them are on the sheet, walked when 1 is. + assert!(index_spills(5_000, 5_000, small.iter())); + assert!(!index_spills(5_000, 1, small.iter())); + // Not a paste, or nothing to index: never. + assert!(!index_spills(16, 16, small.iter())); + assert!(!index_spills(5_000, 5_000, [].iter())); + } + #[test] fn intervals_match_a_linear_scan() { let mut iv: Intervals = Intervals::default(); diff --git a/crates/calc/src/tests.rs b/crates/calc/src/tests.rs index ac254239a..a3e7f810d 100644 --- a/crates/calc/src/tests.rs +++ b/crates/calc/src/tests.rs @@ -2371,3 +2371,25 @@ fn a_paste_over_small_and_tall_spills_blocks_those_it_touches() { assert_eq!(t.get("I1"), Value::Error(CellError::Spill)); assert_eq!(t.num("E3"), 3.0); } + +#[test] +fn a_big_paste_on_one_sheet_and_one_cell_over_many_spills_on_another() { + let mut t = T::new(); + t.wb.sheets.push(std::sync::Arc::new(gridcraft_model::Sheet::new("Data"))); + for i in 0..1_500u32 { + t.wb.sheet_mut(1).unwrap().cells.set(CellRef::new(4 * i, 0), Cell::formula(Formula::new(&format!("=SEQUENCE(3,1,{})", 10 * i)))); + } + t.calc.recalc_all(&mut t.wb); + assert_eq!(t.wb.sheet(1).unwrap().value(CellRef::new(4 * 700 + 2, 0)), Value::Number(7002.0)); + // 5000 cells pasted on the first sheet, one over the middle of the 701st array on the second. + let mut changed: Vec<(usize, CellRef)> = (0..5_000u32).map(|i| (0, CellRef::new(i / 50, i % 50))).collect(); + changed.push((1, CellRef::new(4 * 700 + 1, 0))); + for k in &changed { + t.wb.sheet_mut(k.0).unwrap().cells.set(k.1, Cell::value(Value::Number(1.0))); + } + t.calc.cells_changed(&mut t.wb, &changed); + let data = t.wb.sheet(1).unwrap(); + assert_eq!(data.value(CellRef::new(4 * 700, 0)), Value::Error(CellError::Spill)); + assert_eq!(data.value(CellRef::new(4 * 699 + 2, 0)), Value::Number(6992.0)); + assert_eq!(data.value(CellRef::new(4 * 701 + 2, 0)), Value::Number(7012.0)); +} From e85b24df5724d224ae69b2d9b7bc5cf7554f4d42 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Mon, 2 Nov 2026 20:31:52 +0100 Subject: [PATCH 4/4] calc: register a Python result's spill in the spill index python_result wrote into spills but not into spill_index, so indexed readers (cell_value, range_values, visit_range) missed a Python array laid out in the same pass. Insert on spill, remove when blocked, as the formula path does. Co-Authored-By: Claude Sonnet 5.5 --- crates/calc/src/recalc.rs | 33 +++++++++++++++++++++++++++++ crates/calc/src/recalc/pass_host.rs | 2 ++ 2 files changed, 35 insertions(+) diff --git a/crates/calc/src/recalc.rs b/crates/calc/src/recalc.rs index 77d5b68dd..286569822 100644 --- a/crates/calc/src/recalc.rs +++ b/crates/calc/src/recalc.rs @@ -663,6 +663,39 @@ pub(crate) mod tests { assert!(!host.visit_range(0, inside, &mut |_| true), "an overlapping read leaves the block to range_values"); } + /// A Python array laid out in this pass is seen by the indexed readers, whether the sheet + /// has no spill from before or an older, smaller one. + #[test] + fn a_python_spill_found_in_the_pass_is_read_through_the_index() { + for stale in [false, true] { + let mut wb = Workbook::new(); + let sh = wb.sheet_mut(0).unwrap(); + let anchor = CellRef::new(0, 0); + let py = gridcraft_model::PyCode::new("grid"); + sh.set_cell(anchor, Cell { value: Value::Number(0.0), py: Some(std::sync::Arc::new(py)), ..Cell::default() }); + if stale { + // The last calculation spilled a 2 x 2 block of 100s. + sh.spill_ranges.insert(anchor, RangeRef::new(anchor, CellRef::new(1, 1))); + for c in RangeRef::new(anchor, CellRef::new(1, 1)).iter().skip(1) { + sh.spill.insert(c, Value::Number(100.0)); + } + } + let data: Vec = (1..=9).map(|i| Value::Number(f64::from(i))).collect(); + let array = gridcraft_core::Array::new(3, 3, data).unwrap(); + let mut python = HashMap::default(); + python.insert((0, anchor), Value::Array(std::sync::Arc::new(array))); + let exprs = HashMap::default(); + let mut rng = 1u64; + let mut host = PassHost::new(&wb, &exprs, &mut rng, 0.0).with_python(python); + assert_eq!(host.cell_value(0, anchor), Value::Number(1.0)); + let all = RangeRef::parse("A1:C3").unwrap(); + let sum: f64 = host.range_values(0, all).iter().map(|v| if let Value::Number(n) = v { *n } else { 0.0 }).sum(); + assert_eq!(sum, 45.0, "stale={stale}: a SUM over the new 3 x 3 spill reads all of it"); + assert_eq!(host.cell_value(0, CellRef::new(2, 2)), Value::Number(9.0), "stale={stale}"); + assert!(!host.visit_range(0, all, &mut |_| true), "stale={stale}: the new spill leaves the block to range_values"); + } + } + #[test] fn intervals_match_a_linear_scan() { let mut iv: Intervals = Intervals::default(); diff --git a/crates/calc/src/recalc/pass_host.rs b/crates/calc/src/recalc/pass_host.rs index 9a733501b..1ef24bc42 100644 --- a/crates/calc/src/recalc/pass_host.rs +++ b/crates/calc/src/recalc/pass_host.rs @@ -400,6 +400,7 @@ impl PassHost<'_> { && a.cols >= 1 && a.rows.checked_mul(a.cols).is_some_and(|n| n == a.data.len() && n <= gridcraft_model::python::MAX_RESULT_CELLS); if sane { + self.spill_index.insert(k, a.rows, a.cols); self.spills.insert(k, a.clone()); a.data.first().cloned().unwrap_or_default() } else { @@ -411,6 +412,7 @@ impl PassHost<'_> { let v = match self.spills.get(&k) { Some(a) if spill_blocked(self, k, a) => { self.spills.remove(&k); + self.spill_index.remove(k); Value::Error(CellError::Spill) } _ => v,