Skip to content

Commit f0d56ac

Browse files
refactor(calc): unify engine — delete legacy per-sheet cascade (-810 LOC)
Removes the per-sheet `recalculate_dependents` BFS cascade and the legacy cross-sheet propagation maps. The workbook-wide unified graph (WorkbookGraph + cell_purities + structural_targets) is now the single source of truth for dependency tracking and propagation. What goes away: * `Spreadsheet::dependents` + `dependencies` fields (was per-sheet dep maps); `Spreadsheet::recalculate_dependents` (BFS cascade); `add_cell_dependencies` / `remove_cell_dependencies`; `rebuild_dependencies`. `Spreadsheet::set_cell` / `clear_cell` / `set_many` / `clear_many` are now pure writes (no propagation). `recalculate_cell` merges into `refresh_cell_value`. * `Workbook::cross_sheet_dependents` / `cross_sheet_dependencies` legacy cross-sheet maps; `cells_with_qualified_refs` fast-path set; `needs_workbook_context()` predicate that guarded the workbook-clone optimization. The single-clone snapshot is now unconditional (cheap next to the recalc that follows). * `Workbook::propagate_cross_sheet_changes` / `*_batch` (full BFS impls, ~200 LOC each); `propagate_active_cell`. * Per-sheet cascade tests in spreadsheet.rs that asserted dependents auto-update from `Spreadsheet::set_cell` — that contract no longer holds at the Spreadsheet level; coverage shifted to workbook-level and to the scenario framework. What replaces it: * Every workbook-level mutator (`set_cell_on_active`, `clear_cell_on_active`, `write_cells_on_active`, `clear_cells_on_active`, structural row/col insert/delete, `rename_sheet`, `set_name`, `remove_name`) writes the cell(s), updates the unified graph via `register_cross_sheet_deps`, marks dirty, and runs ONE `recalc_via_graph_result()`. Callers see a fresh empty dirty set after each call. * `register_cross_sheet_deps` simplified to just refresh unified-graph edges + purity + structural_targets. The 80-line cross_sheet_* map maintenance is gone. * `rebuild_cross_sheet_deps` shrunk to a wipe + per-cell re-register (was wiping legacy maps PLUS unified state). * `would_create_cross_sheet_cycle` rewritten to consult the unified graph's `transitive_dependents` instead of walking the legacy maps. * `propagate_cell_change` (App-level) kept as a thin compatibility helper for bespoke callers; just marks dirty + recalcs. Iterative-calc semantics change: Self-referencing formulas (=A1+1 in A1) used to get silently dropped from the graph (`link` filtered self-edges) on the assumption that the user-facing `would_create_circular_reference` check would reject them at the door. With the legacy cascade gone, that filtering meant self-cycles got a single eval against their pre-write value and never reached iterative-calc. Now: self-edges are RETAINED in the graph. `topo_levels_from_seeds` counts them in in-degree (no `p != n` filter), so self-loops land in the cyclic remainder. `iterative_calc_cyclic` runs the cycle up to `iter_max`. Non-convergence is now strict (per design decision): cycles that hit iter_max get `#NUM!` written to every cyclic cell + DidNotConverge in status_message. The previous behavior of writing the iter_max'th iteration value was misleading — that "answer" is an artifact of the iter_max setting, not a meaningful result. Legitimate fixed-point iteration (=A1/2+1 → 2) still gets the converged value, no error. Diffstat: +453 / -1263 = net -810 LOC. Six bookkeeping caches (dependents, dependencies, cross_sheet_dependents, cross_sheet_dependencies, cells_with_qualified_refs, graph) collapsed to two (graph, dirty). Perf (cargo bench --bench calc_engine): deep_small 275µs → 92µs (~3x faster on tiny workloads from removing the workbook-clone overhead); deep_large 25.4ms → 24.3ms (unchanged on large — already dominated by eval work); wide/fanout unchanged or slightly improved. Tests: 588 lib (was 584, +4 net: replaced 8 legacy-cascade tests with new contract tests covering divergent iterative calc → #NUM!, convergent → fixed point, unified-recalc trigger discipline). All 12 PTY scenarios + 11 existing PTY suites green. CLAUDE.md updated to describe the new "one engine, one graph" world.
1 parent 7ca3a7e commit f0d56ac

10 files changed

Lines changed: 453 additions & 1263 deletions

File tree

CLAUDE.md

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ src/
3838

3939
**Formula Functions**: Registered in `FunctionRegistry` in `parser.rs`. All take `&[Value]` and return `Result<Value, String>`. The `Value` enum supports dual number/string types with `.to_number()`, `.to_string()`, and `.is_truthy()` conversions.
4040

41-
**Cell Dependencies**: `Spreadsheet` tracks cell dependencies bidirectionally. When a cell changes via `set_cell()`, dependent cells automatically recalculate. Dependencies are not serialized - call `rebuild_dependencies()` after loading.
41+
**Cell Dependencies**: A single workbook-wide `WorkbookGraph` on `Workbook` tracks all dependencies (same-sheet AND cross-sheet) keyed by `NodeKey = (SheetId, row, col)`. Not serialized — lazily built by `Workbook::build_dep_graph_from_scratch` on first recalc. Per-sheet `dependents`/`dependencies` maps no longer exist (deleted with the legacy cascade). `Spreadsheet::set_cell` is a pure write — propagation goes through `Workbook::recalc_via_graph_result`, which the public `set_cell_on_active` / `write_cells_on_active` / `clear_*_on_active` mutators call internally. See "Calc engine architecture" below.
4242

4343
**App Modes**: `AppMode` enum drives UI state. Each mode has a corresponding handler in `InputHandler` and rendering logic in `ui.rs`. State transitions go through methods on `App`.
4444

@@ -50,9 +50,9 @@ src/
5050

5151
**Cross-sheet structural edits**: `Workbook::insert_row_on_active`, `delete_row_on_active`, `insert_col_on_active`, `delete_col_on_active` perform the same-sheet structural mutation and also walk every OTHER sheet's formulas to shift any sheet-qualified refs to the mutated sheet (e.g. inserting a row at Sheet1!A5 shifts `=Sheet1!A5` to `=Sheet1!A6` on every other sheet). Refs to a deleted sheet's removed row/col become `#REF!`. Routes through `App::insert_row`/`delete_row`/etc.
5252

53-
**Sheet rename/delete**: `Workbook::rename_sheet` rewrites all formula refs (and named-range values) from old to new name, then triggers cross-sheet propagation so dependents recompute. `Workbook::remove_sheet` rewrites every dangling `=GoneSheet!A1` on surviving sheets to `=#REF!` (Excel-equivalent), then purges the dep graph.
53+
**Sheet rename/delete**: `Workbook::rename_sheet` rewrites all formula refs (and named-range values) from old to new name, marks formula cells dirty, then triggers `recalc_via_graph_result()`. `Workbook::remove_sheet` rewrites every dangling `=GoneSheet!A1` on surviving sheets to `=#REF!` (Excel-equivalent), then calls `rebuild_cross_sheet_deps` (which wipes + rebuilds the unified graph from scratch).
5454

55-
**Cross-sheet propagation helper**: `App::propagate_cell_change(row, col)` runs `register_cross_sheet_deps` + `propagate_cross_sheet_changes` on the workbook for the current sheet. Use it from any mutation path (cut, paste, replace_all, vim delete, undo/redo) that writes/clears cells outside of `set_cell_with_undo` / `clear_cell_with_undo`, which already call it internally.
55+
**Mutation API**: Single source of truth is `Workbook::set_cell_on_active` / `clear_cell_on_active` / `write_cells_on_active` / `clear_cells_on_active`. Each writes the cell(s) via `Spreadsheet::set_cell`/etc (pure writes — no cascade), updates the unified graph, marks dirty, and runs `recalc_via_graph_result()` to flush dependents. Callers outside the workbook should never call `Spreadsheet::set_cell` directly — they'd write the value but skip propagation. The application-level helper `App::propagate_cell_change(row, col)` is kept for bespoke paths that need to manually flush but is rarely needed (the standard mutators auto-flush).
5656

5757
## Testing
5858

@@ -85,27 +85,35 @@ Gotchas when writing new PTY tests:
8585

8686
## Calc engine architecture
8787

88-
tshts ships two recalc engines that produce identical results:
88+
tshts has ONE recalc engine: the unified graph-driven level executor. (The legacy per-sheet `recalculate_dependents` cascade and the `propagate_cross_sheet_changes*` family were deleted — see "Mutation discipline" below.)
8989

90-
**Legacy per-sheet cascade**: `Spreadsheet::set_cell` triggers `recalculate_dependents` which BFSes the per-sheet `dependents` HashMap. Cross-sheet propagation runs as a separate `Workbook::propagate_cross_sheet_changes` pass. This is the path most user edits flow through (every `set_cell_on_active` / `write_cells_on_active` call).
90+
**The graph**: `Workbook::graph` is a workbook-wide `WorkbookGraph` keyed by `NodeKey = (SheetId, row, col)`. Bidirectional: `dependencies[N]` is the set of cells `N` reads, `dependents[N]` is the set of cells that read `N`. Same-sheet AND cross-sheet edges live in this one structure. Built lazily via `Workbook::build_dep_graph_from_scratch` (called from `register_cross_sheet_deps` on the first write, on load, and after structural edits) and maintained incrementally by `register_cross_sheet_deps` per write.
9191

92-
**Graph-driven level executor**: `Workbook::recalc_via_graph` builds the unified workbook-level dep graph (`WorkbookGraph`, keyed by stable `SheetId(u32)`), drains the dirty set, computes topological levels via Kahn's algorithm, and walks levels in order. Each level evaluates against an immutable workbook snapshot; results merge back at the level boundary. Used by `:recalc` (`App::recalc_all`).
92+
**`Workbook::cell_purities`** is the parallel-keyed cache of per-cell `FunctionPurity` classifications. Pure cells are stored implicitly (absent from the map). Volatile / side-effecting cells are explicit.
93+
94+
**`Workbook::structural_targets`** is the per-`VolatileStructural`-cell cache of the dynamic targets (INDIRECT/OFFSET resolved cells) recorded after the last eval. Smart auto-seed compares against the user-dirty closure to decide whether to re-seed.
95+
96+
**Mutation discipline**: `Workbook::set_cell_on_active` / `clear_cell_on_active` / `write_cells_on_active` / `clear_cells_on_active` are the only public single-source-of-truth mutation APIs. Each one writes the cell(s) via `Spreadsheet::set_cell`/`clear_cell`/`set_many`/`clear_many` (which are pure writes — no cascade), updates the unified graph via `register_cross_sheet_deps`, marks dirty, and triggers a single `recalc_via_graph_result()` that propagates to all dependents. `Spreadsheet::set_cell` directly is reserved for low-level test fixtures and intra-engine machinery (the iterative-cyclic loop).
97+
98+
**`Workbook::recalc_via_graph_result`** is the entry point — drains `dirty`, builds `seeds`, computes `transitive_dependents` + topological levels via Kahn's algorithm, dispatches to an executor. Self-loops are retained in the graph (they're cyclic — see "Iterative calc" below).
9399

94100
The executor is pluggable via the `RecalcExecutor` trait (`src/domain/services/executor.rs`):
95-
- `SequentialExecutor` — single-threaded reference impl.
96-
- `ParallelExecutor` — rayon-based; partitions each level by function purity, dispatches pure cells via `par_iter().with_min_len(64)`, runs structural-volatile (`INDIRECT`, `OFFSET`) and side-effecting (`GET`) cells serially within the level barrier. Falls back to sequential below `parallel_threshold`.
101+
- `SequentialExecutor` — single-threaded reference impl. One workbook snapshot for the whole recalc, mutated between levels so the next level reads fresh values from prior levels.
102+
- `ParallelExecutor` — rayon-based; partitions each level by function purity, dispatches pure cells via `par_iter().with_min_len(K)`, runs structural-volatile (`INDIRECT`, `OFFSET`) and side-effecting (`GET`) cells serially within the level barrier. Uses `Arc<Workbook>` snapshot reused across levels via `Arc::make_mut` at the level boundary (refcount = 1 after `par_iter().collect()`).
97103

98-
`recalc_via_graph` auto-selects between the two: Parallel when any level has ≥ `TSHTS_PAR_THRESHOLD` cells (default 512), otherwise Sequential. Below that the per-level workbook clone dominates the parallel savings.
104+
`recalc_via_graph` auto-selects: Parallel when any level has ≥ `TSHTS_PAR_THRESHOLD` cells (default 512), otherwise Sequential.
99105

100-
**Tuning**: set `TSHTS_PAR_THRESHOLD=N` to change the parallel-dispatch cutoff. `RAYON_NUM_THREADS=N` controls worker count. `cargo bench --bench calc_engine` runs the archetype benchmarks (wide/deep/fanout × small/medium/large) so you can pick a threshold that matches your workload.
106+
**Tuning**: `TSHTS_PAR_THRESHOLD=N` for parallel-dispatch cutoff. `RAYON_NUM_THREADS=N` for worker count. `cargo bench --bench calc_engine` runs the archetype benchmarks.
101107

102108
**Volatile semantics** (matched to Excel/OpenFormula):
103109
- `NOW`/`TODAY` read a clock snapshot captured at recalc start (via the `RECALC_CLOCK` thread-local published by each executor's `run`). Two clock-volatile cells in the same pass return identical values; calls outside a recalc fall back to `SystemTime::now()`.
104-
- `RAND`/`RANDBETWEEN` use a thread-local PRNG; cross-worker non-determinism is the intended trade — within a pass each worker's outputs are independent.
105-
- `OFFSET`/`INDIRECT` are tagged `VolatileStructural` and auto-seeded into the dirty set on every recalc, so changes to their value-derived targets propagate through their static dependents within one pass (matches Excel's "always recompute volatile").
106-
- `GET` is `SideEffecting`; serialized via the existing HTTP-fetcher worker (no executor changes needed).
110+
- `RAND`/`RANDBETWEEN` use a thread-local PRNG.
111+
- `OFFSET`/`INDIRECT` are tagged `VolatileStructural` and consulted via the smart auto-seed: a structural cell is seeded into the dirty set only when its recorded `structural_targets` intersect the user-dirty closure.
112+
- `GET` is `SideEffecting`; serialized via the HTTP-fetcher worker (cache-seeded for tests via `fetcher::test_hooks`).
113+
114+
**Iterative calc**: `Workbook::iterative_calc_cyclic` runs over the cyclic remainder from `topo_levels_from_seeds`. Gauss-Seidel-style: each pass evaluates against a fresh snapshot, mutates the live workbook between passes. Uses the highest `iter_max` and tightest `iter_epsilon` across participating sheets. Two-pass string stability for non-numeric flip-flop detection.
107115

108-
**Cross-sheet cycles**: handled by `Workbook::iterative_calc_cyclic` — a workbook-level Gauss-Seidel loop that walks every cyclic cell across all sheets per pass. Uses the highest `iter_max` and tightest `iter_epsilon` across participating sheets. Detects non-convergence (returns `Err(iter_max)`) and handles non-numeric flip-flop via two-pass string stability. Per-pass post-write maintenance (CF cache, spill ghosts, `maybe_spill`) is performed inside `with_recalc_context` to mirror the acyclic path.
116+
**Non-convergence is strict**: cycles that exhaust `iter_max` without converging get `#NUM!` written to every cyclic cell (rather than the iter_max'th iteration value, which would be a misleading artifact of the setting). `CalcError::DidNotConverge` is returned and bubbles to `App::status_message` via `recalc_via_graph_result`. Convergent cycles (legitimate fixed-point iteration like `=A1/2+1 → 2`) get the converged value, no error. Self-loops (`=A1+1` written to A1) are normal cycles: routed to `iterative_calc_cyclic` like any other cyclic remainder.
109117

110118
## Dependencies
111119

src/application/state/command.rs

Lines changed: 44 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -863,23 +863,50 @@ impl App {
863863
}
864864
}
865865
["trace", "dependents"] => {
866-
let pos = (self.selected_row, self.selected_col);
867-
let deps = self.workbook.current_sheet().dependents.get(&pos).cloned();
868-
match deps {
869-
Some(set) if !set.is_empty() => {
870-
let s: Vec<String> = set
871-
.iter()
872-
.map(|(r, c)| {
873-
format!(
874-
"{}{}",
875-
crate::domain::Spreadsheet::column_label(*c),
876-
r + 1
877-
)
878-
})
879-
.collect();
880-
self.status_message = Some(format!("Dependents: {}", s.join(", ")));
881-
}
882-
_ => self.status_message = Some("(no dependents)".to_string()),
866+
// Walk the unified workbook graph for cells that read
867+
// (sheet, row, col). The graph is keyed by NodeKey =
868+
// (SheetId, row, col) and is the single source of truth
869+
// post-cascade-removal.
870+
let sheet_idx = self.workbook.active_sheet;
871+
let sheet_id = self.workbook.sheet_ids[sheet_idx];
872+
let node = (sheet_id, self.selected_row, self.selected_col);
873+
let mut seed = std::collections::HashSet::new();
874+
seed.insert(node);
875+
let downstream: Vec<_> = self
876+
.workbook
877+
.graph
878+
.transitive_dependents(&seed)
879+
.into_iter()
880+
.filter(|d| *d != node) // strip self
881+
.collect();
882+
if downstream.is_empty() {
883+
self.status_message = Some("(no dependents)".to_string());
884+
} else {
885+
let mut labels: Vec<String> = downstream
886+
.iter()
887+
.map(|(sid, r, c)| {
888+
let sheet_name = self
889+
.workbook
890+
.sheet_ids
891+
.iter()
892+
.position(|s| s == sid)
893+
.map(|i| self.workbook.sheet_names[i].clone())
894+
.unwrap_or_else(|| "?".to_string());
895+
let same_sheet = *sid == sheet_id;
896+
let cell_label = format!(
897+
"{}{}",
898+
crate::domain::Spreadsheet::column_label(*c),
899+
r + 1
900+
);
901+
if same_sheet {
902+
cell_label
903+
} else {
904+
format!("{}!{}", sheet_name, cell_label)
905+
}
906+
})
907+
.collect();
908+
labels.sort();
909+
self.status_message = Some(format!("Dependents: {}", labels.join(", ")));
883910
}
884911
}
885912
["table", "list"] => {

src/application/state/mod.rs

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -368,11 +368,12 @@ fn restore_workbook(workbook: &mut Workbook, pre: &Workbook) {
368368
fn restore_cell(workbook: &mut Workbook, row: usize, col: usize, data: Option<&CellData>) {
369369
// Route through the workbook chokepoints so the dirty set is
370370
// populated; undo/redo were previously skipping dirty entirely.
371+
// `set_cell_on_active` / `clear_cell_on_active` both run a single
372+
// graph-driven recalc internally, so no separate propagate call.
371373
match data {
372374
Some(d) => workbook.set_cell_on_active(row, col, d.clone()),
373375
None => workbook.clear_cell_on_active(row, col),
374376
}
375-
workbook.propagate_active_cell(row, col);
376377
}
377378

378379
/// Bulk-apply CellModified-style entries from a batch in O(N) total —
@@ -426,7 +427,6 @@ where
426427
// The legacy per-sheet dep graph needs rebuilding because we
427428
// bypassed `add_cell_dependencies`. The unified graph gets
428429
// rebuilt lazily inside recalc_via_graph_result.
429-
workbook.sheets[active_idx].rebuild_dependencies();
430430
workbook.rebuild_cross_sheet_deps();
431431
workbook.build_dep_graph_from_scratch();
432432
// Use the Result variant — the eprintln-swallowing wrapper would
@@ -740,7 +740,6 @@ impl App {
740740
// Keep the per-sheet dep graph in sync — the legacy cross-
741741
// sheet engine still reads it, and `build_dep_graph_from_scratch`
742742
// uses the per-sheet refs as input.
743-
sheet.rebuild_dependencies();
744743
// Mark every formula cell on this sheet dirty.
745744
let name = self.workbook.sheet_names[idx].clone();
746745
for (&(r, c), cd) in &sheet.cells {
@@ -810,11 +809,18 @@ impl App {
810809
}
811810
}
812811

813-
/// Cross-sheet propagation hook for cell mutations that don't go through
814-
/// `Workbook::write_cells_on_active`. Forwards to the workbook, which
815-
/// owns the actual dep registration + propagation logic.
812+
/// Propagation hook for cell mutations that don't go through
813+
/// `Workbook::write_cells_on_active`. With the unified graph executor
814+
/// as the single source of truth, propagation is just "mark dirty
815+
/// + recalc," which `set_cell_on_active` already does. This helper
816+
/// stays for callers that wrote a cell via some bespoke path and
817+
/// then explicitly want to flush; today it just runs a recalc.
816818
pub(crate) fn propagate_cell_change(&mut self, row: usize, col: usize) {
817-
self.workbook.propagate_active_cell(row, col);
819+
// Mark the cell dirty (the bespoke caller may not have done it)
820+
// and run the unified recalc.
821+
let sheet_name = self.workbook.sheet_names[self.workbook.active_sheet].clone();
822+
self.workbook.mark_dirty(&sheet_name, row, col);
823+
let _ = self.workbook.recalc_via_graph_result();
818824
}
819825

820826
pub fn redo(&mut self) {

0 commit comments

Comments
 (0)