Repository navigation
Conversation
Calc::rebuild and Calc::cells_changed called Formula::expr(), which deep-copies each formula's syntax tree only to read its references. Use expr_arc() (already used when formulas are calculated), which hands out the cached Arc without copying. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Provide expressions through callbacks crates/model/src/cell.rs |
Formula::with_expr invokes a callback with a cached expression or parses the formula text first. It returns None if parsing fails. |
Build precedents through the callback crates/calc/src/recalc.rs, crates/calc/src/tests.rs |
Calc::rebuild and Calc::cells_changed compute precedents inside with_expr callbacks. The regression test compares results for cached and text-only formulas, including parse failure and updates after edits. |
Priority:
Estimated code review effort: 2 (Simple) | ~8 minutes
Change: Refactor · Severity of issue fixed: Low
Possibly related issues
Suggested reviewers: echelon
Merge Risk: 
b6bbd
-
No merge-blocking risk was identified; normal checks remain appropriate.
Security architecture risk: b6bbd
-
The change avoids copying formula trees without expanding formula privileges or changing dependency resolution. Cached expressions, text-only formulas and parse failures retain their previous behavior. No material security risk was identified in the changed contract.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The changed behavior is confined to expression access during dependency tracking for the supplied workbook. References still resolve through existing workbook sheet selection; the change does not introduce a new destination or authority for formula-controlled references.
Trust Boundaries and Controls
- observed — Formula text remains input to the same parser and precedent analyzer, not a source of callback code. The program supplies the callback, which receives only an immutable expression borrow. Evaluation continues through the unchanged expr_arc path.
Resilience and Maintainability Implications
- observed — Precedent analysis still completes before node insertion. Rebuild retains full-reset recovery, while cells_changed retains replacement/removal, reverse-index cleanup, spill cleanup and dirty-dependent recalculation. Exclusive mutable access and unchanged transition sequencing provide no new concurrency or interruption window attributable to this PR; parse-failure behavior also predates it.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the main change: Calc::rebuild now builds the dependency graph from shared syntax trees via Formula::with_expr instead of cloning trees with expr(). It also matches the PR objective of… Linked Issues check Issue #2685requires removal of formula-tree deep copies inCalc::rebuildandCalc::cells_changed. The PR usesFormula::with_exprat both call sites.with_exprborrows cachedExprvalues and…Out of Scope Changes check The changes stay within issue #2685. TheFormula::with_exprhelper enables the required no-copy behavior, and the graph-equivalence test supports that change. No unrelated production behavior or unr…Full details: Title check
-
Explanation
The title names the main change: Calc::rebuild now builds the dependency graph from shared syntax trees via Formula::with_expr instead of cloning trees with expr(). It also matches the PR objective of avoiding a tree copy and Arc allocation. The title is short, specific, and in one sentence.
Full details: Linked Issues check
-
Explanation
Issue
#2685requires removal of formula-tree deep copies inCalc::rebuildandCalc::cells_changed. The PR usesFormula::with_exprat both call sites.with_exprborrows cachedExprvalues and parses text-only formulas without cloning or allocating anArc. The new regression test compares cached and text-only formulas through rebuild, edits, andcells_changed. The optionalGraph::cell_depsoptimization is not required.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#2685. TheFormula::with_exprhelper enables the required no-copy behavior, and the graph-equivalence test supports that change. No unrelated production behavior or unrelated tests appear in the reviewed diff.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 3 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
This PR is one of five speed-ups from an audit of the
Results are identical in every case. #2686 compares against the old full-sort code bit for bit (lengths 0 to 4096, ties, -0.0, NaN, inf). #2691 compares against a plain fold, bit for bit, over full recalculations and 30 edit rounds and against the unmodified build on random workbooks with errors, text, spills and RANDBETWEEN. #2694 and #2696 compare byte-identical dumps of all values, Open PRs that touch the same code and will need a small merge: #2319 and #2252 ( 🤖 Generated with Claude Code |
FredApps
left a comment
FredApps
left a comment
There was a problem hiding this comment.
Reviewed both call-site changes together with Formula::expr, Formula::expr_arc, precedents and graph update/removal. The cached-tree and deserialized/no-cache branches obtain the same Expr; the graph visitor borrows it and does not mutate it. Invalid formulas still return None and follow the existing removal/error handling. I found no new concrete correctness defect or dependency on the held-back lookup/name optimizations.
CI evidence: inspected the successful gate job log: 2,524 tests passed, one timing-sensitive calc scaling test skipped; all ten gate steps passed, including fmt, Clippy and wasm. I have not represented these as local tests.
The 31% instruction reduction remains an author benchmark for rebuilding 100,000 relatively complex IF formulas. It targets avoided AST allocation/clone/drop work; it does not establish the same benefit for short formulas, deserialized formulas without cached trees, or a full edit/recalc operation. I did not reproduce the timing or instruction counts.
Local Cargo validation was blocked before compilation: the pinned egui source was unavailable offline, and the isolated crate build could not download missing crates because the TLS client credentials were unavailable. No security settings were changed. This is a COMMENT because local integration validation and independent performance reproduction remain incomplete.
|
I measured this on one Windows machine with a temporary release-mode
The direct parent comparison supports a speedup for cached complex formulas: −31% wall time here, versus the reported −18%; short and uncached cases did not improve. The current |
…nly formulas Formula::with_expr runs a closure on the cached tree, or on a freshly parsed one that is dropped afterwards, so rebuild and cells_changed need neither a tree copy nor an Arc allocation / reference count. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for measuring this, it was useful. I repeated it on Linux with a release harness, 100,000 formulas, pinned to one core, counting instructions (
What I confirmed: the cached-AST gains match yours (-15% short, -31% complex), and the old PR text said -18% for a different formula and with wall time, so I corrected it. The uncached path did get slightly worse: What changed: I pushed b6bbdcd. |
FredApps
left a comment
FredApps
left a comment
There was a problem hiding this comment.
Re-review at b6bbdcd (previously commented at c567511).
Since then: a merge of main and one commit (b6bbdcd) that replaces the expr_arc use with a new Formula::with_expr. I read the whole current diff (3 files).
Checked:
with_exprruns the closure on the cached tree when there is one, and otherwise on a tree parsed from the text that is dropped afterwards; it returnsNonewhen the text does not parse, so an invalid formula still takes the same removal path incells_changedand is skipped inrebuild, as it did throughexpr().- Both call sites borrow the workbook immutably inside the closure (
precedents(wb, si, e)only reads), so the new borrow shape is fine and nothing mutates the tree. A text-only formula no longer costs anArcallocation, which is what the author's no-cache rows were about. - The new test builds the same workbook with cached and with text-only formulas (including one that does not parse), compares the values before and after an edit, and then adds a text-only formula through
cells_changedand checks it joins the graph (F1becomes 107 whenA1is set to 7).
No findings. I did not run the tests or the measurements; the instruction counts are the author's. Approval still needs a run on the current head and target.
Fixes #2685
What
Calc::rebuildandCalc::cells_changedusedFormula::expr(), which deep-copies the whole syntax tree only to hand&Exprtoprecedents. They now call a newFormula::with_expr(|e| ...), which lends the cached tree to the closure, or, for a formula that only has its text (for example after loading a file), parses it and drops the tree afterwards. No copy, noArcallocation and no reference count change. (The first version of this PR usedexpr_arc(); a reviewer's measurements showed it adds anArcallocation on the text-only path, so it was replaced.)Why
Every file open (
recalc_allrunsrebuild), every undo/redo and every structural edit rebuilds the graph, so each one paid for a full copy and drop of every formula's tree (about a fifth of a rebuild in the profile).Numbers
Linux, release build, one pinned core (
taskset), 100,000 formulas in column C. Short is=A{r}+B{r}, complex is=IF(A{r}>B{r},A{r}*B{r}+ROUND(A{r}/3,2),MAX(A{r},B{r})).perf stat -e instructions:u, difference of an 11-rebuild and a 1-rebuild run divided by 10 (millions of instructions per rebuild). "Text only" meansFormula.expr = None. Single edit: parse and store one complex formula pluscells_changed, thousand instructions per edit. Instruction counts are repeatable; wall times on this shared machine were too noisy to quote.The text-only rows are dominated by parsing (the parent-to-main drift is of the same size as the difference), so they are unchanged within noise. The earlier head of this PR (
expr_arc()) measured 940.3 / 3430.1 on the parent base, that is +0.9% / +0.25% on those rows, from the extraArcallocation per parsed formula.How verified
The graph is built from the same tree, so it is unchanged. New test
graph_built_from_text_only_formulas_matches_cachedbuilds the same workbook with cached trees and with text-only formulas and checks identical values afterrebuild, an input edit, and acells_changedof a text-only formula.cargo test -p gridcraft-calc(91 pass),cargo fmt --all --check, andcargo clippy -p gridcraft-calc -p gridcraft-model --lib --tests -- -D warningsare clean.I left the other rebuild cost mentioned in the issue (a
HashSetper precedent cell inGraph::cell_deps) alone to keep this change minimal.🤖 Generated with Claude Code
Summary by CodeRabbit