From 6c912d2eb130130f93d9281baca435a37e5d30e7 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Thu, 29 Oct 2026 23:48:11 +0100 Subject: [PATCH] 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); +}