diff --git a/crates/workbook/benches/baselines.json b/crates/workbook/benches/baselines.json index 6d743eddb..7d39a1233 100644 --- a/crates/workbook/benches/baselines.json +++ b/crates/workbook/benches/baselines.json @@ -1,127 +1,139 @@ { - "recorded_at": "2026-08-28", + "recorded_at": "2026-09-03", "recorded_on": "Apple M1 Max (10 core), macOS 14.4, rustc 1.94.1, release profile; best of 5 full bench runs", - "note": "ref_units = min(benchmark time) / min(reference time), each the best of 5 full bench runs, not necessarily the same run - best-of-5 minima can (and do) come from different runs. This is a deliberate choice, not an oversight: best-of-5 biases the floor in the safe direction, and the resulting bias in ref_units is small and one-directional (recorded baselines read slightly inflated, so future runs read slightly more negative). The gate compares ratios, not nanoseconds, so a baseline recorded on one machine still means something on another. best_ns_recorded and reference_best_ns_recorded are informational only - nothing is checked against them. The bands are two-sided on purpose: see .github/scripts/check_perf_regression.py. The five multi_sheet entries were recorded 2026-08-29 on the same machine, best of 3 runs. The four incremental_recalc(_cold)/independent_edit_root entries were re-recorded the same day, also best of 3, because this change moves them: Sheet1 was being folded ~16 times per formula cell even in a single-sheet workbook before this change (16.50 folds per extra formula cell, vs 0 after), so the single-sheet incremental family got legitimately faster (paired runs on one machine: base 216,815/212,730/215,348 ns vs head 196,590/196,909/195,420 ns on independent_edit_root/100, about -8%; independent_edit_root/1000 and the _cold variants moved similarly). The previously recorded independent_edit_root/100 baseline (best_ns_recorded 321,871) was already stale relative to both the base and head commits on this machine (both read ~200,000-300,000 ns) - this change added the last several points on top of that pre-existing drift, it did not cause the whole gap. Every other entry is unchanged from the 2026-08-28 recording and remains within run-to-run noise of the base commit. The six chain entries (full_recalc/chain and incremental_recalc/chain_edit_root|chain_edit_leaf, each at 1000 and 5000) were added 2026-08-29 and recorded on the machine named above, best of 5 full bench runs - the count this file's recorded_on documents, not the best-of-3 the two exceptions above used. All five runs measured one unmodified binary, and their reference minimum was 3,196,028 ns - not the reference_best_ns_recorded field above, which belongs to the 2026-08-28 set - so verify these six as best_ns_recorded / 3196028, the same way the 2026-08-29 entries above divide by their own run set's minimum. They are new benchmarks, so no existing entry moved when they were added; every other number here is unchanged from its earlier recording. The incremental_recalc/all_literal_edit/120000cells entry (issue #984) was added 2026-09-03 on the machine named above, best of 5 full bench runs of the two benchmarks alone (not the whole suite) - its reference minimum was 3,335,120 ns, again not the reference_best_ns_recorded field above; verify it as best_ns_recorded / 3335120. It is a new benchmark, so no existing entry moved when it was added.", + "note": "Single-run re-record after rebasing #983 (volatile-cell cache) onto main post-#984 (spill-anchor cache) merge \u2014 every entry comes from ONE cargo bench run of the fully-combined code (rustc 1.98.0 (88d9e12ae 2026-08-18), release profile), not a hand-merge of two separate recordings. Includes both PRs new groups: incremental_recalc/row_totals_volatile_seed/{100,10000,100000} (#983) and incremental_recalc/all_literal_edit/120000cells (#984). best_ns_recorded and reference_best_ns_recorded are informational only - nothing is checked against them. The bands are two-sided on purpose: see .github/scripts/check_perf_regression.py.", "reference": "calibration/hash_alloc", - "reference_best_ns_recorded": 3270207, + "reference_best_ns_recorded": 3164388, "regression_pct": 150, "improvement_pct": 40, "benchmarks": { "full_recalc/independent/100": { - "ref_units": 0.1436, - "best_ns_recorded": 469632 + "ref_units": 0.1391, + "best_ns_recorded": 440310 }, "full_recalc/independent/1000": { - "ref_units": 1.3395, - "best_ns_recorded": 4380494 + "ref_units": 1.2879, + "best_ns_recorded": 4075270 }, "full_recalc/independent/5000": { - "ref_units": 6.9069, - "best_ns_recorded": 22587145 + "ref_units": 7.0781, + "best_ns_recorded": 22397889 }, "full_recalc/chain/1000": { - "ref_units": 1.196, - "best_ns_recorded": 3822395 + "ref_units": 1.2234, + "best_ns_recorded": 3871375 }, "full_recalc/chain/5000": { - "ref_units": 6.5853, - "best_ns_recorded": 21046666 + "ref_units": 6.7395, + "best_ns_recorded": 21326255 }, "full_recalc/row_totals/500": { - "ref_units": 1.3837, - "best_ns_recorded": 4524877 + "ref_units": 1.4669, + "best_ns_recorded": 4641820 }, "full_recalc/row_totals/2000": { - "ref_units": 6.0191, - "best_ns_recorded": 19683565 + "ref_units": 6.3515, + "best_ns_recorded": 20098562 }, "full_recalc/block_subtotals/2000": { - "ref_units": 1.2934, - "best_ns_recorded": 4229585 + "ref_units": 1.3706, + "best_ns_recorded": 4337191 }, "full_recalc/block_subtotals/10000": { - "ref_units": 8.0703, - "best_ns_recorded": 26391430 + "ref_units": 8.2032, + "best_ns_recorded": 25957993 }, "full_recalc/multi_sheet/200x50": { - "ref_units": 13.8314, - "best_ns_recorded": 45612155 + "ref_units": 13.9405, + "best_ns_recorded": 44113229 }, "full_recalc/multi_sheet_long_names/200x50": { - "ref_units": 13.2369, - "best_ns_recorded": 43651501 + "ref_units": 13.3224, + "best_ns_recorded": 42157171 }, "full_recalc/multi_sheet_cross_refs/200x50": { - "ref_units": 13.4762, - "best_ns_recorded": 44440765 + "ref_units": 15.199, + "best_ns_recorded": 48095548 }, "full_recalc/tall_sparse/20000": { - "ref_units": 39.9981, - "best_ns_recorded": 130802021 + "ref_units": 40.8564, + "best_ns_recorded": 129285553 }, "depgraph_build/tall_sparse/20000": { - "ref_units": 9.9993, - "best_ns_recorded": 32699704 + "ref_units": 9.9017, + "best_ns_recorded": 31332860 }, "depgraph_build/row_totals/2000": { - "ref_units": 0.9512, - "best_ns_recorded": 3110508 + "ref_units": 0.9418, + "best_ns_recorded": 2980216 }, "depgraph_build/multi_sheet_cross_refs/200x50": { - "ref_units": 3.2537, - "best_ns_recorded": 10729783 + "ref_units": 3.1996, + "best_ns_recorded": 10124723 }, "incremental_recalc/independent_edit_root/100": { - "ref_units": 0.0605, - "best_ns_recorded": 192758 + "ref_units": 0.0566, + "best_ns_recorded": 179180 }, "incremental_recalc/independent_edit_root/1000": { - "ref_units": 0.3549, - "best_ns_recorded": 1130495 + "ref_units": 0.2876, + "best_ns_recorded": 910143 + }, + "incremental_recalc_cold/independent_edit_root/100": { + "ref_units": 0.0919, + "best_ns_recorded": 290789 + }, + "incremental_recalc_cold/independent_edit_root/1000": { + "ref_units": 0.736, + "best_ns_recorded": 2329101 + }, + "incremental_recalc/multi_sheet_edit_root/200x50": { + "ref_units": 2.5644, + "best_ns_recorded": 8114804 }, "incremental_recalc/block_subtotals_edit_root/10000": { - "ref_units": 1.2095, - "best_ns_recorded": 3823493 + "ref_units": 0.7581, + "best_ns_recorded": 2398977 }, "incremental_recalc/chain_edit_root/1000": { - "ref_units": 1.1638, - "best_ns_recorded": 3719666 + "ref_units": 1.0771, + "best_ns_recorded": 3408477 }, "incremental_recalc/chain_edit_root/5000": { - "ref_units": 6.3891, - "best_ns_recorded": 20419766 + "ref_units": 5.9662, + "best_ns_recorded": 18879251 }, "incremental_recalc/chain_edit_leaf/1000": { - "ref_units": 0.3183, - "best_ns_recorded": 1017376 + "ref_units": 0.2646, + "best_ns_recorded": 837166 }, "incremental_recalc/chain_edit_leaf/5000": { - "ref_units": 1.6274, - "best_ns_recorded": 5201330 + "ref_units": 1.3121, + "best_ns_recorded": 4152095 }, - "incremental_recalc/multi_sheet_edit_root/200x50": { - "ref_units": 3.2604, - "best_ns_recorded": 10751809 + "incremental_recalc/all_literal_edit/120000cells": { + "ref_units": 3.8168, + "best_ns_recorded": 12077798 }, - "from_json/500rows": { - "ref_units": 0.2833, - "best_ns_recorded": 926457 + "incremental_recalc/row_totals_volatile_seed/100": { + "ref_units": 0.0793, + "best_ns_recorded": 250884 }, - "to_json/500rows": { - "ref_units": 0.2216, - "best_ns_recorded": 724738 + "incremental_recalc/row_totals_volatile_seed/10000": { + "ref_units": 7.5273, + "best_ns_recorded": 23819279 }, - "incremental_recalc_cold/independent_edit_root/100": { - "ref_units": 0.0933, - "best_ns_recorded": 297138 + "incremental_recalc/row_totals_volatile_seed/100000": { + "ref_units": 96.2568, + "best_ns_recorded": 304593843 }, - "incremental_recalc_cold/independent_edit_root/1000": { - "ref_units": 0.7459, - "best_ns_recorded": 2375688 + "from_json/500rows": { + "ref_units": 0.2837, + "best_ns_recorded": 897782 }, - "incremental_recalc/all_literal_edit/120000cells": { - "ref_units": 3.7609, - "best_ns_recorded": 12543118 + "to_json/500rows": { + "ref_units": 0.2194, + "best_ns_recorded": 694132 } } } diff --git a/crates/workbook/benches/workbook_perf.rs b/crates/workbook/benches/workbook_perf.rs index 010ccd538..519d3c3b1 100644 --- a/crates/workbook/benches/workbook_perf.rs +++ b/crates/workbook/benches/workbook_perf.rs @@ -499,12 +499,14 @@ fn bench_incremental_recalc(c: &mut Criterion) { // Read the two together, not the ratio between them. That ratio is roughly // flat (3.7x at n=1000, 3.9x at n=5000 when recorded), because both cases // pay the same per-formula-cell fixed cost every incremental recalc owes — - // `snapshot_formula_values`, the volatile sweep over `formula_cells()`, and - // `seed_spill_sensitive`, each `O(formula cells)` by construction. At - // n=5000 the leaf case's time is almost entirely that floor rather than its - // one-cell recompute, which is precisely what makes it worth recording: no - // other incremental benchmark holds the dirty set fixed while the workbook - // grows, so nothing else here can see that floor at all. + // `snapshot_formula_values` and `seed_spill_sensitive`, each + // `O(formula cells)` by construction (the volatile sweep over + // `formula_cells()` was a third such term until issue #983 made it a + // cached lookup instead). At n=5000 the leaf case's time is almost + // entirely that remaining floor rather than its one-cell recompute, which + // is precisely what makes it worth recording: no other incremental + // benchmark holds the dirty set fixed while the workbook grows, so + // nothing else here can see that floor at all. // // The regression signal is the *gap*: a leaf closure that silently widened // to the whole chain would jump the leaf case onto the root case's number @@ -579,6 +581,39 @@ fn bench_incremental_recalc(c: &mut Criterion) { }); }); group.finish(); + + // Reproduces the exact shape issue #983 measured: a formula-heavy + // document with zero actually-volatile functions, timing a single + // unrelated literal write. Before the fix this scaled linearly with + // formula count (367ms at n=100,000, dirty closure verified at 1) + // because the volatile-seeding loop re-derived every formula cell's + // volatility from scratch on every call. + // + // The fix makes *that term* O(1) in formula count, but the numbers this + // group records are not themselves flat: `snapshot_formula_values` and + // `seed_spill_sensitive` are two other O(formula cells) passes this same + // call still pays on every incremental recalc (see `chain_edit_leaf`'s + // comment above, which documents the same floor), and issue #983 does not + // touch either. Don't read a still-scaling number here as the fix having + // failed — the isolated volatile-rescan term it actually targets is gone; + // what remains is the other two terms, tracked separately. + let mut group = c.benchmark_group("incremental_recalc/row_totals_volatile_seed"); + group.sample_size(10); + group.measurement_time(Duration::from_secs(5)); + for n in [100u32, 10_000, 100_000] { + let mut template = build_row_totals(n); + template.recalc(&ctx); + group.bench_with_input(BenchmarkId::from_parameter(n), &n, |b, _| { + b.iter(|| { + let mut wb = template.clone(); + let a1 = Address::new(1, 1).unwrap(); + wb.set("Sheet1", a1, CellInput::Literal(Value::Number(99.0))) + .unwrap(); + wb.recalc_incremental(&ctx, &[("Sheet1".to_string(), a1)]) + }); + }); + } + group.finish(); } fn bench_from_json(c: &mut Criterion) { diff --git a/crates/workbook/src/graph_cache.rs b/crates/workbook/src/graph_cache.rs index a1fd965c8..74bffab31 100644 --- a/crates/workbook/src/graph_cache.rs +++ b/crates/workbook/src/graph_cache.rs @@ -85,6 +85,11 @@ pub struct CachedGraph { pub(crate) order: Vec, /// The cycle set from that same pass. pub(crate) cycle: BTreeSet, + /// Every formula cell whose formula text names a volatile function + /// (`Workbook::is_volatile`), for this exact graph — computed once at + /// build time so `recalc_incremental`'s seeding step never re-derives it + /// per call (issue #983). + pub(crate) volatile: BTreeSet, } impl CachedGraph { diff --git a/crates/workbook/src/recalc.rs b/crates/workbook/src/recalc.rs index 869824b25..65fc7952b 100644 --- a/crates/workbook/src/recalc.rs +++ b/crates/workbook/src/recalc.rs @@ -305,10 +305,23 @@ impl Workbook { // times per incremental recalc (the spill widen loop) while ordering // the same graph every time. let (order, cycle) = graph.evaluation_order(); + // Likewise derived from the graph and cached alongside it: a formula + // cell's volatility cannot change without the formula text changing, + // which already invalidates this cache — so this is the one place + // `is_volatile` needs to run, once per formula cell per graph build, + // instead of once per formula cell per incremental recalc (issue + // #983). + let sheets = SheetIndex::build(self); + let volatile: BTreeSet = graph + .formula_cells() + .filter(|cell| self.is_volatile(&sheets, cell)) + .cloned() + .collect(); let entry = Arc::new(CachedGraph { graph, order, cycle, + volatile, }); self.store_cached_graph(entry.clone()); entry @@ -412,11 +425,11 @@ impl Workbook { } } - // (b) Volatile cells are always dirty (scope ADR Decision 3). - for cell in graph.formula_cells() { - if self.is_volatile(&sheets, cell) { - frontier.insert(cell.clone()); - } + // (b) Volatile cells are always dirty (scope ADR Decision 3). The set + // is cached on the graph at build time (issue #983) — no + // re-derivation here. + for cell in &cached.volatile { + frontier.insert(cell.clone()); } // Spill-occupancy seeding (issue #591). A cell's spill footprint or diff --git a/crates/workbook/tests/recalc_volatile_cache_tests.rs b/crates/workbook/tests/recalc_volatile_cache_tests.rs new file mode 100644 index 000000000..c1d6e4bd2 --- /dev/null +++ b/crates/workbook/tests/recalc_volatile_cache_tests.rs @@ -0,0 +1,202 @@ +//! Cache correctness for the volatile-cell set `CachedGraph` now carries +//! (issue #983): the set must be exactly right after every graph rebuild, not +//! merely "usually right until an edit exercises the bug." +//! +//! ## Why behavioral, not structural +//! +//! `CachedGraph::volatile` is `pub(crate)`, matching `order`/`cycle` (neither +//! is exposed either), so this asserts through the same instrumentation the +//! rest of the incremental-recalc suite already uses: +//! +//! * [`Workbook::graph_builds`] — an exact count of how many graphs have +//! actually been built, proving a rebuild happened (and, just as +//! important, that a later call reused the warm entry rather than +//! rebuilding it every time); +//! * `recalc_incremental_measured`'s returned closure size — the dirty set +//! an edit produced, which is exactly what a stale volatile entry would +//! get wrong. +//! +//! This is also the more convincing check: it catches the exact failure the +//! issue calls out — "a stale 'not volatile' entry producing a wrong recalc +//! result that looks fine but is stale" — rather than an internal flag that +//! could be right for the wrong reason. +//! +//! ## Fixture +//! +//! One sheet. `A1` a literal, `B1 = "=A1+1"` (a plain, never-volatile +//! formula), `C1` a lone literal used as the "unrelated edit" target in every +//! case, so the edit's own direct dependents are empty and the *only* thing +//! that can land in the dirty closure is whatever the volatile-seeding step +//! (or lack of it) contributes. + +use truecalc_workbook::{ + Address, CellInput, Change, EngineFlavor, RecalcContext, Value, Workbook, Worksheet, +}; + +fn ctx() -> RecalcContext { + RecalcContext::new(1_780_878_600_000, "Etc/GMT", 0).expect("Etc/GMT is a valid tz") +} + +fn addr(a1: &str) -> Address { + Address::from_a1(a1).expect("valid A1") +} + +/// `A1` = 1 (literal), `B1` = `=A1+1` (formula, never volatile), `C1` = 0 +/// (literal, the unrelated edit target). +fn base_workbook() -> Workbook { + let mut wb = Workbook::new(EngineFlavor::Sheets); + wb.add_sheet(Worksheet::new("Sheet1")).unwrap(); + wb.set("Sheet1", addr("A1"), CellInput::Literal(Value::Number(1.0))) + .unwrap(); + wb.set("Sheet1", addr("B1"), CellInput::Formula("=A1+1".into())) + .unwrap(); + wb.set("Sheet1", addr("C1"), CellInput::Literal(Value::Number(0.0))) + .unwrap(); + wb +} + +/// Writes a new literal into `C1` (an edit unrelated to anything under test — +/// it has no dependents) and runs an incremental recalc seeded from it, +/// returning the changes and the dirty-closure size. +fn edit_c1(wb: &mut Workbook, n: f64) -> (Vec, usize) { + wb.set("Sheet1", addr("C1"), CellInput::Literal(Value::Number(n))) + .unwrap(); + wb.recalc_incremental_measured(&ctx(), &[("Sheet1".to_string(), addr("C1"))]) +} + +fn touched(changes: &[Change]) -> Vec { + changes.iter().map(|c| c.addr.to_a1()).collect() +} + +// --------------------------------------------------------------------------- +// (a) Adding a new volatile formula. +// --------------------------------------------------------------------------- + +#[test] +fn adding_a_volatile_formula_puts_it_in_the_cached_set() { + let mut wb = base_workbook(); + wb.recalc(&ctx()); + assert_eq!(wb.graph_builds(), 1); + + // D1 was previously empty; adding "=NOW()" both rebuilds the graph (a new + // formula cell) and must recompute the volatile set for it. + wb.set("Sheet1", addr("D1"), CellInput::Formula("=NOW()".into())) + .unwrap(); + assert!( + !wb.graph_cache_is_warm(), + "a new formula cell invalidates the graph cache" + ); + + let (changes, closure) = edit_c1(&mut wb, 1.0); + assert_eq!(wb.graph_builds(), 2, "the graph rebuilt exactly once"); + assert_eq!( + closure, 1, + "the dirty closure must be exactly the volatile cell D1 — the C1 \ + edit itself contributes nothing else" + ); + assert!( + touched(&changes).contains(&"D1".to_string()), + "{:?}", + touched(&changes) + ); + + // Repeating the same kind of edit with no further mutation must reuse + // the warm cache (no rebuild) while still finding D1 volatile every time + // — proving this is a cache, not a rebuild-every-call in disguise. + let (_changes2, closure2) = edit_c1(&mut wb, 2.0); + assert_eq!( + wb.graph_builds(), + 2, + "no mutation happened since the last build; the cache must stay warm" + ); + assert_eq!(closure2, 1, "D1 must still be seeded as volatile"); +} + +// --------------------------------------------------------------------------- +// (b) Removing a volatile formula. +// --------------------------------------------------------------------------- + +#[test] +fn removing_a_volatile_formula_drops_it_from_the_cached_set() { + let mut wb = base_workbook(); + wb.set("Sheet1", addr("D1"), CellInput::Formula("=NOW()".into())) + .unwrap(); + wb.recalc(&ctx()); + assert_eq!(wb.graph_builds(), 1); + + // Clear D1 back to empty: it is no longer a formula cell at all. + wb.clear("Sheet1", addr("D1")); + assert!( + !wb.graph_cache_is_warm(), + "clearing a formula cell invalidates the graph cache" + ); + + let (changes, closure) = edit_c1(&mut wb, 1.0); + assert_eq!(wb.graph_builds(), 2); + assert_eq!( + closure, 0, + "D1 is gone; the cached volatile set must not retain a stale entry \ + for it" + ); + assert!( + !touched(&changes).contains(&"D1".to_string()), + "{:?}", + touched(&changes) + ); +} + +// --------------------------------------------------------------------------- +// (c) Flipping volatility on an existing formula cell, both directions. +// --------------------------------------------------------------------------- + +#[test] +fn flipping_a_formula_between_volatile_and_not_updates_the_cached_set_both_ways() { + let mut wb = base_workbook(); + wb.recalc(&ctx()); + assert_eq!(wb.graph_builds(), 1); + + // B1 = "=A1+1" is not volatile: an unrelated C1 edit has nothing to seed. + let (_changes, closure) = edit_c1(&mut wb, 1.0); + assert_eq!( + wb.graph_builds(), + 1, + "no formula changed; the graph stays warm" + ); + assert_eq!(closure, 0, "B1 is not volatile yet"); + + // Flip B1 to a volatile formula over the same cell (a formula write, so + // the graph cache invalidates). + wb.set( + "Sheet1", + addr("B1"), + CellInput::Formula("=A1+RAND()".into()), + ) + .unwrap(); + assert!(!wb.graph_cache_is_warm()); + + let (changes, closure) = edit_c1(&mut wb, 2.0); + assert_eq!(wb.graph_builds(), 2); + assert_eq!( + closure, 1, + "B1 is now volatile and must be seeded on every incremental recalc" + ); + assert!( + touched(&changes).contains(&"B1".to_string()), + "{:?}", + touched(&changes) + ); + + // Flip B1 back to a non-volatile formula. A set-membership bug that + // inserts on the add path but never evicts the old entry (or the + // reverse) would get exactly this direction wrong silently. + wb.set("Sheet1", addr("B1"), CellInput::Formula("=A1+1".into())) + .unwrap(); + assert!(!wb.graph_cache_is_warm()); + + let (_changes, closure) = edit_c1(&mut wb, 3.0); + assert_eq!(wb.graph_builds(), 3); + assert_eq!( + closure, 0, + "B1 is no longer volatile; the cached set must have evicted it" + ); +}