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)); +}