Repository navigation
perf(workbook): remove two O(formula-cell-count) recalc floors (issue #991) - #995
Merged
Merged
Conversation
Contributor
Test Coverage by Category
✓ = 100% passing · ⚠ = known deviation · The ~79,739 total counts formula evaluations (each conformance row and each property case = 1). GitHub Checks reports 4,191 Rust test functions: 3,090 unit + 159 property functions (shown as cases above) + 942 conformance/integration. |
…eed (issue #991 prereq) incremental_recalc/row_totals_volatile_seed clones the full 900,000-cell template inside its timed b.iter() closure, alongside the set + recalc_incremental under test. At n=100,000 that clone is a real, possibly dominant, share of the reported number, and neither of #991's two fixes touches clone cost at all. Add row_totals_volatile_seed_clone_only at the same three scales so both fixes can be judged on (raw − clone_only), not the raw number alone.
…991) recalc_incremental_measured paid two costs proportional to total formula-cell count on every call, regardless of how small the actual edit was: 1. snapshot_formula_values cloned every formula cell's value into a BTreeMap before the spill-widen loop ran, purely so a widened retry could rewind to the pre-edit grid and report correct pre-edit `old` values. 2. seed_spill_sensitive rebuilt its AuthoredCellIndex fresh on every call that touched any range precedent (~900,000-cell sweep at n=100,000 in the row_totals fixture), even though the authored-cell set itself rarely changes between calls. Design A (fix 1): delete the upfront snapshot. Accumulate the pre-image map lazily instead, folding it in first-wins from each recompute pass's own returned Vec<Change> (apply_changes is the only code path that writes to the grid, and every write it makes is recorded there with the pre-write value). The final diff_against_snapshot step is unchanged and still runs, so the returned change list is byte-identical to before, just computed without ever touching a formula cell the edit didn't reach. Fix 2 (safer fallback, not full "Design B"): cache only the built AuthoredCellIndex on the workbook (authored_cell_index_cache.rs), mirroring spill_anchor_cache.rs's own pattern, invalidated only when the authored-cell SET actually changes (Workbook::set introducing a new cell, Workbook::clear removing one) or a sheet-structure operation runs (sheets_mut/sheet_mut/ insert_sheet/remove_sheet/rename_sheet — not move_sheet, which changes neither the folded-name keys nor any sheet's authored cells). Verified apply_changes can never change the authored-cell set (it only rewrites cells that already have formula text), so recalc's own value write-back needs no invalidation. seed_spill_sensitive_built_index (the existing test instrumentation in authored_cell_index_tests.rs) is deliberately left uncached, so its "did this call need to build the index" answer keeps its existing meaning regardless of the workbook cache's warmth. A full "Design B" (caching seed_spill_sensitive's entire derived seed set, not just the AuthoredCellIndex build) was considered and rejected for this PR as a strictly bigger, less-confident correctness surface; tracked separately as issue #992. Also adds pre_image_stats.rs: an exact-count instrumentation counter (pre_image_count()) proving a one-cell edit into a 1,000-formula workbook records exactly one pre-image, not one per formula cell. Measured on incremental_recalc/row_totals_volatile_seed, clone-subtracted: n=100: 208µs -> 188µs n=10,000: 17.2ms -> 11.2ms (~35% faster) n=100,000: 229ms -> 162ms (~29% faster) Ablating the AuthoredCellIndex-cache fallback alone (Design A kept) showed no measurable difference on this fixture: the benchmark clones a fresh workbook every iteration, so the cross-call cache this fallback targets is never warm across more than one call in that harness, and the O(formula-cells x range-width) sweep this fallback does not touch dominates what remains. A direct probe issuing 20 repeated incremental calls on one live instance (no inter-call clone) confirms the fallback behaves exactly as designed (authored_index_builds() stays at 1 across all 20 calls) with a small, noise-level wall-clock difference on this fixture's cost profile; its real win is on workloads with a much larger authored-cell-to-formula-cell ratio, or many more back-to-back incremental calls than this benchmark issues. Out of scope, tracked at #985: seed_spills_from_grid / GridSpillIndex::build's own remaining full-grid-scan cost on any workbook with at least one real spill (only the all-empty case is handled there). Tests: recalc_differential_tests.rs (existing sweeps unchanged, plus a new named rewind test asserting Change.old always reflects the true pre-recalc grid); recalc_work_tests.rs (new exact pre-image count assertion); spill_incremental_recalc_tests.rs (three new adversarial cases: a scalar edit creating a new spill several cells downstream, one removing an existing spill, and a from_json-loaded workbook recalculated incrementally on its first call, never having run a full recalc); authored_cell_index_cache_tests.rs (cold build, warm reuse, the negative "safe edit stays warm" case, one rebuild test per real invalidation condition, move_sheet's deliberate exemption, and a from_json cold-start case). authored_cell_index_tests.rs and the full recalc_incremental(edits) == recalc() property suite pass unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WfhRbND7tjFJ4JeGZ9gHL5
6061075fd's commit message says the deferred full "Design B" (caching seed_spill_sensitive's entire derived seed set) is "tracked separately as issue #992" — #992 is the #985 spill-scan-residual PR, not an issue. The actual filed follow-up is #993 ("recalc: cache seed_spill_sensitive's full derived seed set (Design B, deferred from #991)"). Recording the correction here since repo convention is to add a follow-up commit rather than rewrite an already-shared commit's message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WfhRbND7tjFJ4JeGZ9gHL5
#946 (spill_chain bench fixture) and its own baselines.json re-record merged to main while this PR's CI was running, conflicting with the baseline this PR had just recorded. Rebased onto origin/main and recorded one fresh combined baseline from a real, complete bench run of the fully-rebased code via check_perf_regression.py --record, per repo convention — not a hand-merge of the two recordings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WfhRbND7tjFJ4JeGZ9gHL5
hhimanshu
force-pushed
the
perf/991-recalc-floors
branch
from
September 3, 2026 01:12
2b87f4d to
39a4fc6
Compare
Contributor
Test Coverage by Category
✓ = 100% passing · ⚠ = known deviation · The ~79,739 total counts formula evaluations (each conformance row and each property case = 1). GitHub Checks reports 4,192 Rust test functions: 3,090 unit + 159 property functions (shown as cases above) + 943 conformance/integration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Workbook::recalc_incremental_measuredpaid two costs proportional tototal formula-cell count, not to the dirty-closure size, on every call
regardless of how small the edit was:
snapshot_formula_valuescloned every formula cell's current valueinto a
BTreeMapup front, purely so a widened spill retry could rewindto the pre-edit grid and so returned
Changeevents could report correctpre-edit
oldvalues across multiple internal recompute passes.seed_spill_sensitiverebuilt anAuthoredCellIndexfrom scratch onevery call that examined any range precedent, even though the
authored-cell set rarely changes between calls.
Fix 1 (Design A): delete the upfront snapshot. Accumulate the pre-image
map lazily instead, folding it in first-wins from each recompute pass's own
returned
Vec<Change>—apply_changesis the only code path that writes tothe grid, and every write it makes already carries the pre-write value as
old. The final diff step is unchanged, so the returned change list isbyte-identical to before; it's just computed without ever touching a formula
cell the edit didn't reach.
Fix 2 (a narrower, safer fallback — not the full "Design B"): cache just
the built
AuthoredCellIndexon the workbook (authored_cell_index_cache.rs),mirroring
spill_anchor_cache.rs's own pattern. Invalidated only when theauthored-cell set actually changes (
Workbook::setintroducing a new cell,Workbook::clearremoving one) or a sheet-structure operation runs(
sheets_mut/sheet_mut/insert_sheet/remove_sheet/rename_sheet—move_sheetis deliberately exempt, since reordering tabs changes neither thefolded-name keys nor any sheet's authored cells).
apply_changesneverinvalidates it: it only ever rewrites a cell that already carries formula
text, so it can change a value but never adds or removes an authored entry.
Also adds
pre_image_stats.rs: exact-count instrumentation(
pre_image_count()) proving a one-cell edit into a 1,000-formula workbookrecords exactly one pre-image, not one per formula cell.
Measured results
Apple M1 Max (10 core), macOS 14.4, rustc 1.98.0, release profile.
incremental_recalc/row_totals_volatile_seed, before (origin/main, medianof 3 real bench runs) vs after (this branch, fully rebased onto #946 — the
single combined run also recorded as this PR's baseline, see below):
Raw (includes the template clone the benchmark's timed closure pays
every iteration alongside
set+recalc_incremental):Clone-subtracted (subtracting
row_totals_volatile_seed_clone_only, anew control-group benchmark this PR adds at the same three scales, built
identically but doing nothing except
template.clone(); before uses thesame after-run's clone-only numbers, since neither this fix nor #946 touches
clone cost):
Why both numbers matter: at n=100,000 the benchmark clones a 900,000-cell
workbook inside its timed closure on every iteration, and neither of this
PR's fixes touches clone cost at all — so the raw number understates what
this PR actually removed from
recalc_incremental_measureditself. Theclone-subtracted number isolates that; the raw number is the direct answer to
"does calling
set+recalc_incrementalactually get faster." Both pointthe same direction here, on a quiet machine with a single clean combined run.
Baseline update
crates/workbook/benches/baselines.jsonis re-recorded in this PR (twochore(bench)commits — the branch had to be rebased mid-review, see below),via
check_perf_regression.py --recordfrom a real, complete bench run ofthe fully-rebased branch each time — not a hand-edit. It needed updating for
two independent reasons:
row_totals_volatile_seed_clone_onlycontrol group this PR addshas no prior baseline entry, which the gate's coverage-drift check fails on
its own (
UNGATED).unrelated spill-chain bench fixture) merged to
main— and re-recordedbaselines.jsonitself — while this PR's CI was still running, conflictingwith the baseline this PR had just recorded. The final commit rebases onto
that and records one fresh combined baseline on top, so this PR's baseline
update carries both The perf gate has no spill fixtures, so a 20-30% incremental-recalc cost passed invisibly #946's and recalc_incremental pays two O(formula-cell-count) fixed costs every call: snapshot_formula_values and seed_spill_sensitive #991's new groups together, from one real
run of the code that will actually land on
main.Deferred scope
seed_spill_sensitive'sO(formula cells × precedents)sweep — checkingevery formula cell's precedents for spill-sensitivity on every call — is
untouched by this PR. Fix 2 only removes the cost of building the
AuthoredCellIndexthat sweep consults; the sweep itself still runs in fullevery call.
A full "Design B" — caching the entire derived seed set
seed_spill_sensitiveproduces, not just the index it consults — wasconsidered and rejected for this PR as a strictly bigger, less-confident
correctness surface, and filed separately as
#993 for its own dedicated review cycle. Quoting the original
design investigation's own confidence caveats directly, since they're exactly
why this was deferred rather than rushed in here:
Also out of scope, already tracked and merged separately at #992:
seed_spills_from_grid/GridSpillIndex::build's own full-grid-scan cost.Tests
recalc_differential_tests.rs: existing random differential sweepsunchanged; new
changes_report_true_pre_recalc_old_values_not_intermediate_widen_valuespins that
Change.oldis always the true pre-recalc value, never anintermediate widen-pass value — with an explicit investigation note that
the widen loop's rewind branch (
pass > 0) could not be forced to executeanywhere in this suite (including a 3,000-seed sweep), so the invariant is
asserted in its general form rather than gated on that branch.
recalc_work_tests.rs: new exact pre-image-count assertion.spill_incremental_recalc_tests.rs: three new adversarial cases — a scalaredit creating a new spill several cells downstream, one removing an
existing spill, and a
from_json-loaded workbook recalculatedincrementally on its very first call.
authored_cell_index_cache_tests.rs(new file): cold build, warm reuse,the negative "safe edit stays warm" case, one rebuild test per real
invalidation condition,
move_sheet's deliberate exemption, and afrom_jsoncold-start case.authored_cell_index_tests.rsand the fullrecalc_incremental(edits) == recalc()property suite pass unchanged.Independent verification performed for this PR
cargo fmt --all -- --checkon every touched file: cleancargo clippy --workspace --exclude truecalc-python -- -D warnings: cleancargo test --workspace --exclude truecalc-python: full suite greenpython3 .github/scripts/check_perf_regression.pyagainst the recordedbaseline:
Perf OKpython3 .github/scripts/test_check_perf_regression.py: all cases passcloses #991