Skip to content

perf(workbook): cache formula-cell volatility with the dependency graph - #987

Merged
hhimanshu merged 3 commits into
mainfrom
perf/983-volatile-cache
Sep 2, 2026
Merged

hhimanshu merged 3 commits into
mainfrom
perf/983-volatile-cache

Conversation

@hhimanshu

@hhimanshu hhimanshu commented Sep 2, 2026 •

Copy link
Copy Markdown
Member

closes #983

Problem

recalc_incremental's seeding step re-derived every formula cell's volatility (Workbook::is_volatile, scanning the formula text against VOLATILE_FUNCTIONS) from scratch on every incremental recalc call — an O(formula cells) pass paid regardless of how small the edit was, alongside the two other O(formula cells) passes the same call already pays (snapshot_formula_values, spill-occupancy seeding).

Fix

A formula cell's volatility cannot change without its formula text changing, and any formula-text write already invalidates the dependency-graph cache (sheets_mut/names_mut/etc.). So the volatile set is exactly as cacheable as the graph itself: CachedGraph now carries a volatile: BTreeSet<CellRef> computed once, at graph-build time, alongside order/cycle. recalc_incremental's seeding step reads that cached set instead of re-scanning every formula cell's text.

This makes that one term O(1) per incremental recalc (amortized across cache-warm calls); the two other O(formula cells) passes are untouched and still scale linearly — see the benchmark comment for why the recorded numbers below aren't flat.

Correctness

Three new behavioral tests in crates/workbook/tests/recalc_volatile_cache_tests.rs, asserting through Workbook::graph_builds() (proves whether a rebuild actually happened) and the returned dirty-closure size (exactly what a stale entry would get wrong), not through the private field itself:

  • Adding a new volatile formula puts it in the cached set, and the cache stays warm (no rebuild) across repeated unrelated edits.
  • Removing a volatile formula cell drops it from the set — no stale membership.
  • Flipping an existing formula cell volatile and back again updates the cached set correctly both directions (catches an add-but-never-evict bug or its mirror).

Benchmarks

New group incremental_recalc/row_totals_volatile_seed (a formula-heavy sheet with zero actually-volatile functions, timing one unrelated literal write) at n = 100 / 10,000 / 100,000. Independently re-measured myself, before vs. after, on the same machine, same binary otherwise identical except for this change (pre-fix baseline built from a worktree at the parent commit with only the new bench file copied over):

n before after delta
100 317 µs 277 µs -13% (mostly noise at this size)
10,000 33.6 ms 29.0 ms -14%
100,000 406 ms 364 ms -10%

The win is a constant-factor reduction, not an asymptotic one — total time is still O(formula cells) at this n because the other two seeding passes are untouched — matching what the in-file benchmark comment already says. baselines.json re-recorded wholesale for issue #983 (see its note field for methodology: best of 2 quiet runs, machine/rustc versions, and why 2 rather than the file's usual 3/5).

Verification performed independently

  • cargo fmt scoped to the touched files: clean (a pre-existing, unrelated rustfmt-version drift exists in crates/core on main itself — confirmed there too, not introduced here).
  • cargo clippy --workspace -- -D warnings: clean.
  • cargo test -p truecalc-workbook: 468 passed (54 suites), including the 3 new tests.
  • cargo test --workspace --exclude truecalc-python: 4173 passed (114 suites). (truecalc-python fails to link on this machine — missing system python3.9 — confirmed pre-existing on main too, unrelated to this change.)
  • Benchmark numbers above measured directly, not taken from the branch's own claims.

Rebased onto latest main (which had moved: v9.1.0 release + the AxisMove primitive) before verification; rebase was clean.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Update: rebased onto main after #984 (spill-anchor caching) merged —
both touched recalc_incremental's seeding pass and benches/ workbook_perf.rs/baselines.json. The code merge was clean (non-overlapping
edits in recalc.rs); the two new benchmark groups in workbook_perf.rs
were kept side by side (a duplicate group.finish() omission from the
merge was also fixed). baselines.json was NOT hand-merged — a fresh,
single, internally-consistent baseline was recorded from one real
cargo bench run of the fully-combined code (both #983 and #984's fixes
present together), so every entry comes from the same run rather than
splicing two different machines/times.

The remaining O(formula cells) floor this PR's own benchmark comments
already document (snapshot_formula_values, seed_spill_sensitive) is now
tracked as its own issue: #991.

@hhimanshu hhimanshu self-assigned this Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Test Coverage by Category

Category Unit Tests Google Sheets Conformance Property Cases Total
Array 42 552/552 ✓ 1,000 (2×500) 1,594
Database 35 182/182 ✓ 3,500 (7×500) 3,717
Date 373 418/418 ✓ 2,500 (5×500) 3,291
Engineering 245 886/888 ⚠ 5,500 (11×500) 6,633
Filter 11 81/81 ✓ 4,500 (9×500) 4,592
Financial 149 1,208/1,208 ✓ 2,000 (4×500) 3,357
Info 0 256/256 ✓ 4,500 (9×500) 4,756
Logical 121 267/267 ✓ 3,500 (7×500) 3,888
Lookup 69 393/393 ✓ 1,000 (2×500) 1,462
Math 545 2,006/2,006 ✓ 8,000 (16×500) 10,551
Operator 87 251/251 ✓ 7,500 (15×500) 7,838
Parser 83 93/93 ✓ 4,000 (8×500) 4,176
Query 37 — — 37
Statistical 529 3,191/3,191 ✓ 5,000 (10×500) 8,720
Text 327 803/804 ⚠ 4,000 (8×500) 5,131
Timezone 47 — — 47
Volatile 0 — 3,500 (7×500) 3,500
Web 29 59/59 ✓ 6,000 (12×500) 6,088
Total 3,090 10,646/10,649 66,000 (132×500) ~79,739

✓ = 100% passing · ⚠ = known deviation · The ~79,739 total counts formula evaluations (each conformance row and each property case = 1). GitHub Checks reports 4,164 Rust test functions: 3,090 unit + 159 property functions (shown as cases above) + 915 conformance/integration.

hhimanshu and others added 2 commits September 3, 2026 08:43
…#983)

`recalc_incremental`'s volatile-cell seeding step re-derived every formula
cell's volatility from scratch on every call, allocating an uppercase copy
of each formula string and substring-scanning it against
`Registry::VOLATILE_FUNCTIONS` — O(total formula cells) work paid on every
edit, however small the actual dirty closure. Measured at 100,000 formula
cells with a dirty closure of exactly 1 cell: 367ms per edit.

`is_volatile`'s result cannot change without the formula text changing,
which already invalidates `CachedGraph`, so this adds a `volatile:
BTreeSet<CellRef>` field computed once, in the same pass that already
computes `order`/`cycle` when the graph is built. The incremental seeding
step now reads that cached set directly instead of re-scanning
`formula_cells()`. `is_volatile` itself is unchanged — it now has exactly
one caller (graph build) instead of one per incremental recalc.

Isolated, same-process timing of the seeding step alone (immune to
clone/allocation noise elsewhere in a full recalc):

  formula cells   old (full rescan)   new (cached read)
  1,000           315.7 µs/call       6 ns/call
  10,000          3.64 ms/call        4 ns/call
  100,000         47.3 ms/call        10 ns/call

A full `incremental_recalc/row_totals_volatile_seed` benchmark (new, added
to workbook_perf.rs) reproduces the issue's own measurement shape end to
end; on this machine (best-of-1, some run-to-run variance from shared
load):

  n         before      after
  100       505 µs      272 µs
  10,000    38.0 ms     27.3 ms
  100,000   603 ms      369 ms

The full-call numbers move less than the isolated seeding cost because the
remaining time is dominated by other O(n) per-recalc work (workbook clone,
pre-edit snapshot, spill seeding) that this change does not touch.

New test file `recalc_volatile_cache_tests.rs` covers cache correctness
across a graph rebuild: adding a volatile formula, removing one, and
flipping an existing formula's volatility both directions, each asserting
the resulting dirty-closure size via `recalc_incremental_measured` rather
than any internal flag. `recalc_incremental_property_tests` and
`recalc_volatile_frontier_tests` re-verified explicitly and pass unchanged,
confirming this is a pure caching optimization with no semantic change.

`crates/workbook/benches/baselines.json` is intentionally left unchanged:
this dev machine had confirmed CPU contention from another concurrent
benchmark process during this session, which makes it unsafe to bake in
recorded numbers (the regression gate's own docs warn against recording
from an unconfirmed/noisy run). A maintainer should run

  cargo bench -p truecalc-workbook --bench workbook_perf -- \
      --output-format bencher | python3 .github/scripts/check_perf_regression.py --record

on a quiet machine (confirming the result reproduces on a second run, per
that script's own guidance) before merging, both to add an entry for the
new benchmark and because several existing incremental_recalc entries may
now cross the gate's 40% "unexpected improvement" threshold.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WfhRbND7tjFJ4JeGZ9gHL5
…h comment (issue #983 review)

Addresses two real findings from adversarial review of the volatile-cell
cache PR, both confirmed independently rather than taken on faith:

- crates/workbook/benches/baselines.json had no entry for the new
  incremental_recalc/row_totals_volatile_seed benchmark group, which
  check_perf_regression.py's own docstring calls out as a hard CI
  failure ("a benchmark with no baseline entry ... added but never
  recorded"). Re-recorded the full baseline set (the file's own
  documented workflow for this) from two full bench runs on a
  confirmed-quiet machine, taking the per-benchmark minimum of the two
  (safe: contention only slows a run down). Verified both raw runs now
  pass the gate with zero failures, and the gate's own self-test suite
  still passes. Updated recorded_on/note to describe this recording
  accurately instead of leaving five-runs-ago provenance text in place.

- The comment above row_totals_volatile_seed claimed the fix "makes
  this O(1) in formula count," but the benchmark's own numbers
  (280us/27ms/362ms across n=100/10k/100k) are not flat - two other
  O(formula cells) passes in the same call (snapshot_formula_values,
  seed_spill_sensitive) are untouched by issue #983 and still scale
  with n. Corrected the comment to say what actually became O(1) (the
  volatile-rescan term alone) so a reader doesn't misread the
  still-scaling numbers as the fix having failed.

No production code changed; is_volatile, CachedGraph, and
recalc_incremental are untouched by this commit.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WfhRbND7tjFJ4JeGZ9gHL5
@hhimanshu
hhimanshu force-pushed the perf/983-volatile-cache branch from 0362be6 to edfefc0 Compare September 2, 2026 20:58
@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Test Coverage by Category

Category Unit Tests Google Sheets Conformance Property Cases Total
Array 42 552/552 ✓ 1,000 (2×500) 1,594
Database 35 182/182 ✓ 3,500 (7×500) 3,717
Date 373 418/418 ✓ 2,500 (5×500) 3,291
Engineering 245 886/888 ⚠ 5,500 (11×500) 6,633
Filter 11 81/81 ✓ 4,500 (9×500) 4,592
Financial 149 1,208/1,208 ✓ 2,000 (4×500) 3,357
Info 0 256/256 ✓ 4,500 (9×500) 4,756
Logical 121 267/267 ✓ 3,500 (7×500) 3,888
Lookup 69 393/393 ✓ 1,000 (2×500) 1,462
Math 545 2,006/2,006 ✓ 8,000 (16×500) 10,551
Operator 87 251/251 ✓ 7,500 (15×500) 7,838
Parser 83 93/93 ✓ 4,000 (8×500) 4,176
Query 37 — — 37
Statistical 529 3,191/3,191 ✓ 5,000 (10×500) 8,720
Text 327 803/804 ⚠ 4,000 (8×500) 5,131
Timezone 47 — — 47
Volatile 0 — 3,500 (7×500) 3,500
Web 29 59/59 ✓ 6,000 (12×500) 6,088
Total 3,090 10,646/10,649 66,000 (132×500) ~79,739

✓ = 100% passing · ⚠ = known deviation · The ~79,739 total counts formula evaluations (each conformance row and each property case = 1). GitHub Checks reports 4,173 Rust test functions: 3,090 unit + 159 property functions (shown as cases above) + 924 conformance/integration.

Merged workbook_perf.rs's two new benchmark groups side by side (fixed a
missing group.finish() the merge introduced). baselines.json is a fresh,
single-run recording of the fully-combined code rather than a hand-merge of
two separately-recorded baselines, so every entry is internally consistent.
@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Test Coverage by Category

Category Unit Tests Google Sheets Conformance Property Cases Total
Array 42 552/552 ✓ 1,000 (2×500) 1,594
Database 35 182/182 ✓ 3,500 (7×500) 3,717
Date 373 418/418 ✓ 2,500 (5×500) 3,291
Engineering 245 886/888 ⚠ 5,500 (11×500) 6,633
Filter 11 81/81 ✓ 4,500 (9×500) 4,592
Financial 149 1,208/1,208 ✓ 2,000 (4×500) 3,357
Info 0 256/256 ✓ 4,500 (9×500) 4,756
Logical 121 267/267 ✓ 3,500 (7×500) 3,888
Lookup 69 393/393 ✓ 1,000 (2×500) 1,462
Math 545 2,006/2,006 ✓ 8,000 (16×500) 10,551
Operator 87 251/251 ✓ 7,500 (15×500) 7,838
Parser 83 93/93 ✓ 4,000 (8×500) 4,176
Query 37 — — 37
Statistical 529 3,191/3,191 ✓ 5,000 (10×500) 8,720
Text 327 803/804 ⚠ 4,000 (8×500) 5,131
Timezone 47 — — 47
Volatile 0 — 3,500 (7×500) 3,500
Web 29 59/59 ✓ 6,000 (12×500) 6,088
Total 3,090 10,646/10,649 66,000 (132×500) ~79,739

✓ = 100% passing · ⚠ = known deviation · The ~79,739 total counts formula evaluations (each conformance row and each property case = 1). GitHub Checks reports 4,173 Rust test functions: 3,090 unit + 159 property functions (shown as cases above) + 924 conformance/integration.

@hhimanshu
hhimanshu merged commit f39263d into main Sep 2, 2026
9 checks passed
@hhimanshu
hhimanshu deleted the perf/983-volatile-cache branch September 2, 2026 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cache each formula cell's volatility alongside the dependency graph — O(total formulas) → O(1) per incremental recalc

1 participant