Range edges: memoized expansion, size-aware union, mutable reverse-edge builder (GH-522)

[arcaputo3]

Fixes #522. Stacked on #521 (base: fix/518-519-kahn-alloc-sigterm); #520's wave-parallel
evaluation stacks on top of this.

The defect

The workbook-level dependency graph materializes a RANGE reference as one QualifiedRef edge
per covered cell, per referencing formula. The financial-model shape — many formulas each
aggregating the same driver column — multiplies the two: 9,900 formulas × SUM($A$1:$A$5000) =
~50M edge objects. JFR on one recalc of that book (on top of #521, so none of this is the
Kahn quadratic): 135.8 GB total allocation, of which the actual SUM math is 12.3 GB — ~70%
is graph build/traversal churn on duplicated range edges. This is the shape of the deployed
51k-formula lender models (session sesn_01HdDiU7hyCyL7hqdLhCqYMs).

Three contained fixes (no structural change)

1. memoizedCells — one graph build expands any given (sheet, range) exactly once; every
   referencing formula shares one immutable Set instance. Applied to fromWorkbookBounded
   (whole-book recalc + write-verb dirty cone) and fromWorkbook (recalculateDependents, the
   -i put path). Sound because expansion is a pure function of (sheet, range) within one
   build — bounds are computed once up front.
2. union — extractQualifiedDependencies merged Set(oneRef) ++ rangeSet, iterating the
   5,000-element side once per operator node above a range ref. Union is commutative, and any
   result above 4 elements is a CHAMP set whose iteration order is a function of its contents
   alone — so iterating the smaller side into the larger cannot move any downstream order.
   Results of ≤4 elements keep the left-to-right build (small sets are insertion-ordered, and
   Kahn's emitted order feeds off their iteration).
3. reverseEdges — the dependents fold allocated a fresh outer-Map node chain per edge, 50M
   times; replaced by a local mutable accumulator with the identical insertion sequence (so
   every per-key set, small insertion-ordered ones included, iterates exactly as before). Shared
   by fromWorkbookBounded and DependentRecalculation.buildDependentsMap.

Measured (JVM assembly, M-series; "heavy book" = 9,900 formulas × SUM over a 5,000-row driver column)

metric	#521 base	this PR	change
wall	47.9 s	27.9 s	1.7×
user CPU	102.9 s (GC ≈ a full core)	35.2 s	2.9× less total work
peak RSS	6.0 GB	1.7 GB	3.4×

Books whose ranges are small (the 200k-formula chain book) are unchanged (~20 s), as expected —
the fix targets the range-fan-out shape specifically.

What remains on the heavy book is eval-side work (per-evaluation range reads) plus the
O(formulas × range-size) graph traversals (Tarjan/Kahn/cone). Range-compressed edges — a
range NODE in the graph, O(formulas + ranges) — stay the structural end-state, tracked in #522.

Verification

Full evaluator suite green (the ordering-pinned gates — GH-491 twin exactness, GH-492
condensation determinism — are the point: every one of these changes was constrained to keep
iteration orders bit-identical). Full __.test run green on the stack tip.

🤖 Generated with Claude Code

[claude]

Review: GH-522 range-edge allocation (memoized expansion, size-aware union, mutable reverse-edge builder)

Read the full diff plus the surrounding call sites in DependencyGraph, DependentRecalculation, StructuralEditor, and DataTableSeeder. I could not run ./mill in my sandbox, so I'm taking the green-suite claim at face value; everything below is from reading.

Overall: strong. Three contained, well-motivated fixes with the determinism reasoning written down where the next reader will need it. The three big things I checked and agree with:

• union's guard is sound. Flipping only when b.size > 4 guarantees the result has >= 5 elements, so both branches produce a CHAMP HashSet over identical contents. Sets of <= 4 keep the left-to-right build, which is where insertion order is actually observable. Correct.
• The memo is keyed by sheet, so fromWorkbookBounded's per-sheet clipping stays correct, and routing locCells through cellsFor means cross-sheet ranges (TExpr.SheetRange, RangeLocation.CrossSheet) memoize too — easy to miss, and you didn't.
• private[formula] on reverseEdges is the right visibility for the DependentRecalculation reuse, and the delegation there is behavior-preserving per-key.

Findings below, roughly by value.

---

1. The memo key includes anchors, which the expansion ignores — avoidable misses

CellRange is a final case class (start, end, startAnchor, endAnchor), so startAnchor/endAnchor participate in equals/hashCode. But neither expansion path looks at them: unboundedQualifiedCells calls range.cells (coordinates only, CellRange.scala:163), and the bounded path calls range.intersect(b) which explicitly discards anchors (CellRange.scala:103, "Intersection loses anchor info") before .cells.

So SUM($A$1:$A$5000) and SUM(A1:A5000) on the same sheet are two cache entries producing two identical 5,000-element Sets. Your motivating book happens to be anchor-uniform so it hits 100%, but a real model that mixes anchored aggregates with unanchored ones splits the cache up to 16 ways per geometric range — and each miss is exactly the 5,000-element allocation the PR exists to remove.

Cheap fix, no correctness impact:

val cache = scala.collection.mutable.HashMap.empty[(SheetName, ARef, ARef), Set[QualifiedRef]]
(sheet, range) => cache.getOrElseUpdate((sheet, range.start, range.end), expand(sheet, range))

2. reverseEdges still allocates one immutable Set per edge

The fix removed the outer-Map node chain (the 8.5 GB), but acc(dep) = acc.getOrElse(dep, Set.empty) + ref still builds a fresh immutable Set per edge. On the motivating shape each of the 5,000 driver cells accumulates 9,900 dependents, so that is still ~50M intermediate HashSets — plausibly the largest single line item left in graph build.

A mutable.LinkedHashSet accumulator per key removes it and is order-identical by your own argument: LinkedHashSet iterates in insertion order, and Set.newBuilder yields Set1..Set4 in add order for <= 4 elements and a content-ordered HashSet above that — exactly what Set.empty + r1 + ... + rn produces today.

val acc = mutable.HashMap.empty[QualifiedRef, mutable.LinkedHashSet[QualifiedRef]]
dependencies.foreach { case (ref, deps) =>
  deps.foreach { dep => acc.getOrElseUpdate(dep, mutable.LinkedHashSet.empty) += ref }
}
acc.view.mapValues(_.toSet).toMap

Also worth presizing — mutable.HashMap.empty starts at 16 and rehashes all the way up to millions of keys. new mutable.HashMap(dependencies.size, mutable.HashMap.defaultLoadFactor) costs nothing.

(Both are follow-ups, not blockers — happy to see them land separately or in #522's structural work.)

3. The reverseEdges docstring overclaims (not a bug today, but a live constraint)

"The insertion sequence is identical to the fold it replaces ... so every per-key set ... iterates exactly as before."

The per-key claim is right. The outer map's claim isn't: acc.toMap reconstructs the immutable Map by iterating the mutable.HashMap in bucket order, and MapBuilderImpl keeps <= 4 entries as insertion-ordered Map1..Map4. So for a graph with <= 4 distinct dependency targets, the returned map's keySet/iteration order can differ from the old fold's.

I traced every consumer and it is not observable today:

• kahnOrder — dependents.getOrElse(node, ...), per-key only
• transitiveDependentsOf — per-key only
• DataTableSeeder:428 — ctx.dependents.getOrElse, per-key
• StructuralEditor:299 — the one outer iteration (dependents.keySet.filter), but staleCaches is consumed solely via .contains (:333), so order can't escape

Since the whole PR rests on order arguments being auditable, I'd rather the comment say what is actually guaranteed: per-key sets are bit-identical; the outer map's iteration order is not part of the contract and no consumer may depend on it. That's a note the next person adding a consumer needs.

4. memoizedCells is not thread-safe, and #520 is parallel

The closure captures an unsynchronized mutable.HashMap. Fine right now — both builds are sequential flatMap over workbook.sheets. But the PR body says #520's wave-parallel evaluation stacks on top of this, and concurrent getOrElseUpdate on a mutable.HashMap can lose updates or corrupt the table outright. One scaladoc line ("single-threaded within one build; the returned closure must not be shared across threads") makes the constraint explicit before someone parallelizes the sheet loop.

5. @SuppressWarnings(Array("org.wartremover.warts.Var")) on reverseEdges is a no-op

There is no var in the body — the mutable state is a mutable.HashMap, which Var doesn't flag, and build.mill:60 has Var as warn-only anyway. memoizedCells uses the same kind of mutable state with no annotation, so the two are inconsistent. Drop it (compare kahnOrder, which genuinely does use var/while and needs its suppression).

6. Minor: the union docstring's "function of its CONTENTS alone"

True for the CHAMP trie, with one exception: elements landing in a HashCollisionSetNode (full 32-bit improve(hashCode) collision) are stored in a Vector whose order is insertion-dependent, so a ++ b and b ++ a could order them differently. Astronomically unlikely for QualifiedRef and not adversarially reachable — but since the sentence is stated absolutely in a comment future readers will lean on, a "(modulo hash-collision nodes)" parenthetical would be honest.

7. Test coverage — nothing added

This is the main gap. There is precedent in this exact suite: GH-518 landed RecalcPerfSpec "topologicalSort on a 100k-cell chain is linear, ordered, and inside the budget". Three tests worth having:

1. reverseEdges == the naive fold — a ScalaCheck property over arbitrary Map[QualifiedRef, Set[QualifiedRef]] asserting equality with the old foldLeft. Cheap, and it pins the contract now that two call sites share the helper.
2. Memoized == unmemoized graph — build a workbook where the same range is referenced from many formulas (and from two sheets with different used ranges, and with mixed anchors) and assert fromWorkbookBounded equals a reference build with memoizedCells bypassed. Finding Add Claude Code GitHub Workflow #1 shows the cache key is subtler than it looks; a future edit to expand that makes it depend on something outside (sheet, range) would silently return stale sets, and nothing currently catches that.
3. A budgeted perf test for the target shape — N formulas x SUM($A$1:$A$5000), same posture as the evaluator: kahnOrder allocates O(V²) — topological sort is 81% of recalc allocation (28 GB garbage on a 50k-formula book) #518 test. The existing ordering gates prove you didn't break anything; nothing proves the fix stays in place.

---

Nice work on the measurement discipline in the description — the RSS and user-CPU numbers alongside wall clock are what make the 1.7x credible rather than noise. None of the above blocks merge; #1 and #7 are the two I'd want before this is considered done.

[claude]

Review — GH-522 range-edge allocation

Read the diff against DependencyGraph.scala and traced every consumer of the changed outputs. The three fixes are sound and I found no correctness bug. The determinism arguments hold where it matters, and the one place the PR body's claim is stronger than what's true turns out to be unobservable today. Detail below, ordered by what I'd act on.

Worth calling out up front: three local edits, no signature churn outside the file, and both immutable→mutable conversions are fully encapsulated behind a private / private[formula] boundary — the purity charter is intact.

---

1. The -i put path is only half-fixed — fromSheet still has all three shapes

recalculateDependentsWithClock builds the workbook graph (now fixed) and then, inside the per-sheet fold at DependentRecalculation.scala:131, calls DependencyGraph.fromSheet(sheet) — the untouched ARef-level twin, which still has literally the same three patterns:

• extractDependenciesBounded re-expands the same (bounds, range) per formula (DependencyGraph.scala:507-510) — no memo hook;
• left-to-right ++ on all 12 binary operators (:529-550), the exact Set(oneRef) ++ rangeSet shape fix Add P6 Cell-level Codecs + P31 Ergonomics & Purity Enhancements #2 targets;
• the immutable reverse-edge fold at :119-123, byte-for-byte the fold reverseEdges replaces.

So on the shape this PR is about — 9,900 formulas × SUM($A$1:$A$5000) on one sheet — the -i put path pays the fixed cost once at workbook level and then the unfixed cost again at sheet level. fromSheet also feeds SheetEvaluator:282,531 and three ReadCommands sites.

reverseEdges is trivially genericizable ([K] — no QualifiedRef in the body) and would drop straight into :119. The memo needs a boundRange closure hoisted out of extractDependenciesBounded the way cellsFor already is here, which is a bigger edit — reasonable to defer, but I'd either take the free reverseEdges[K] reuse now or say explicitly in the PR body that the sheet-level twin is out of scope. As written the body reads as though recalculateDependents is covered end to end.

2. Memo cache is unbounded and pinned for the whole build

memoizedCells retains every expansion until the build returns. For the target shape that's the point (one entry, 9,900 sharers). For the anchored-drag shape — =SUM($A$1:A2) filled down, very common in the same financial models — every row is a distinct key, so hit rate is zero and the cache is pure retention.

It is bounded in practice: when the range set is the whole dependency set (single-arg SUM) the memo entry and the map entry are the same instance, so nothing extra is held. The extra retention is only the Set spine for ranges merged into a larger result (=SUM($A$1:A2)*B2) — elements are shared. At 9,900 × ~5,000 that is still a few hundred MB of CHAMP spine that previously died in the nursery. Not a blocker, and likely still net-positive against the 6.0 GB → 1.7 GB you measured, but it is the one shape where this can move peak RSS the wrong way. Worth a line in the scaladoc (or a range.size >= threshold guard on insertion) so the next person profiling a drag-heavy book knows to look here.

3. memoizedCells returns a non-thread-safe closure and nothing says so

The closure captures a bare scala.collection.mutable.HashMap and is threaded all the way down through extractQualifiedDependencies, including the NameRef/SheetNameRef recursion (:1342, :1366). Every caller is single-threaded today — I grepped, there is no .par/Future in xl-evaluator/src. But #520 stacks wave-parallel evaluation directly on top of this, and if the graph build ever fans out over workbook.sheets, an unsynchronized HashMap resize race is a silent lost entry or an infinite loop, not an exception. One scaladoc sentence — "one instance per single-threaded build; the returned function is not thread-safe" — is enough to stop that.

4. reverseEdges scaladoc: the outer map order does change (verified harmless)

the insertion sequence is identical to the fold it replaces […] so every per-key set […] iterates exactly as before

Per-key sets: agreed, exactly right — same outer iteration, same inner iteration, same + ref sequence. But the outer map is now mutable.HashMap.toMap, and for a result of ≤4 keys that is Map1..Map4 built in the mutable table's bucket order, not first-insertion order. (≥5 keys is CHAMP on both sides, so those genuinely match.)

I chased it and it is unobservable: kahnOrder seeds from dependencies.keySet and only ever looks up dependents (:852-880), and the other four consumers — DependentRecalculation:120 → transitiveDependentsQualified, StructuralEditor:290 → .keySet.filter into a Set, WriteCommands:957, DataTableSeeder:268 — all funnel it into order-insensitive Set operations. So: correct the comment, not the code. That comment is exactly what someone will trust the day they add a consumer that does iterate the dependents map.

5. union's CHAMP argument has one exception: collision nodes

any result with more than 4 elements is a CHAMP HashSet whose iteration order is a function of its CONTENTS alone

True except for elements with fully-equal hashCode, which land in a HashCollisionNode whose backing Vector is insertion-ordered — so a ++ b and b ++ a can differ in the relative order of colliding refs. At this scale that is not hypothetical: ~1M QualifiedRefs over a 32-bit hash space is a near-certain collision by birthday.

No code change needed in my view — hashCode is stable, so output stays deterministic run-to-run and topologically valid, and a tie-break between two colliding refs is exactly the kind of order the API does not promise. But "cannot move any downstream order" is stronger than what holds; I would soften to "…except among refs with identical 32-bit hashCodes, where relative order may differ but remains deterministic."

Separately: the guard reads correctly to me in all four size regimes — when b.size > 4, both a ++ b and b ++ a yield a ≥5-element HashSet with identical contents; when it does not fire, the left-to-right build is untouched.

6. Nit: dead WartRemover suppression

@SuppressWarnings(Array("org.wartremover.warts.Var")) on reverseEdges — the method has no var and no while, and MutableDataStructures is not among the traversers in build.mill:48-64, so nothing is being suppressed. Drop it; as written it signals "there is a var below" to the next reader. (detectCrossSheetCycles just above does need its suppressions, which is probably where this got copied from.)

---

Test coverage — nothing new, and the two cheapest tests are the highest-value ones

The PR's entire safety case is "iteration orders are bit-identical", and right now that argument lives in a commit message. The existing GH-491/GH-492 gates are property tests over graph values (DependencyGraphSpec:837-909) — they pin the sort's behavior given a graph, not that this build path produces the same graph, which is what changed. Two suggestions:

1. Memo-key correctness. Two sheets whose used ranges differ (say Sheet1 A1:A10, Sheet2 A1:A3) with one formula referencing the same literal CellRange on both — =SUM(Sheet1!A1:A100)+SUM(Sheet2!A1:A100) — asserting 10 + 3 edges. A memo keyed on range alone (an easy future refactor slip, and the one way this change could go badly wrong) would return 10 + 10, and nothing in the current suite would notice.
2. Order-equality property. ScalaCheck: reverseEdges(g) equals the naive fold on generated graphs, comparing .toList per key; and extractQualifiedDependencies with memo + union matches a non-memoized left-to-right reference. Cheap, and it converts the reasoning in the PR body into something CI enforces — which matters most for the follow-up in evaluator: range edges are materialized per formula — N×SUM(col) books allocate ~10× the actual evaluation work in graph churn #522 that rewrites this area again.

Also worth a RecalcPerfSpec-style guard on the shape itself if that suite is the right home: otherwise nothing stops a future refactor from quietly reintroducing per-formula expansion, and the symptom is allocation, not wrong answers — invisible to the functional suite.

---

Verdict: approve-with-nits on correctness. #1 is the one I would want addressed or explicitly scoped out before merge, since it affects whether the PR delivers what its description claims for the -i put path. #2–#6 are docs/hardening; the tests are the durable part.

[claude]

Review — GH-522 range-edge allocation

Read the diff against DependencyGraph.scala and DependentRecalculation.scala, plus the surrounding call sites (fromSheet, kahnOrder, WorkbookEvaluator, StructuralEditor, WriteCommands). Overall this is a well-targeted, well-argued change: three contained fixes, each with the determinism argument written into the scaladoc rather than left implicit. The PR body's reasoning holds up on the two things that matter most — memoizedCells returning a shared instance can't move any order, and the union guard (b.size > 4) is correct because the result is a superset of b, hence always a CHAMP HashSet whose iteration order is contents-determined. Nice touch that locCells routes cross-sheet ranges through the same memoized cellsFor, so SheetRange / RangeLocation.CrossSheet get the win too.

A few things worth a look before merge.

1. reverseEdges: the "identical insertion sequence" claim doesn't survive acc.toMap for small maps (DependencyGraph.scala:1230)

The per-key Sets are exactly as claimed — same growth sequence, same iteration. The outer map isn't. The old fold built Map.empty.updated(...) incrementally, so for ≤4 distinct keys the result is a Map1..Map4 in first-insertion order. The new code accumulates into a mutable.HashMap and calls .toMap, which feeds Map.newBuilder from the mutable map's bucket order — so for ≤4 entries you get the same contents in a generally different position order. Above 4 entries both are immutable HashMap and canonical, so it is identical there.

I traced the consumers and I don't think anything observes it today: kahnOrder (:847) only does dependents.getOrElse(node, …) lookups and iterates dependencies.keySet, and StructuralEditor / WriteCommands / WorkbookEvaluator funnel the map through keySet.filter / qualifiedTransitiveDependents, both of which land back in Sets. So: a doc-accuracy issue rather than a live bug — but this PR's entire safety argument is "iteration orders are bit-identical," and this one isn't, which makes it worth either weakening the comment or closing the gap. Closing it is a one-word change: use mutable.LinkedHashMap instead of mutable.HashMap. Insertion-ordered iteration means .toMap reproduces the old Map1..4 ordering exactly, and for >4 entries it's canonical anyway — so the invariant becomes literally true at essentially no cost.

2. The larger remaining allocation in reverseEdges is the inner sets, not the outer map (DependencyGraph.scala:1227)

acc(dep) = acc.getOrElse(dep, Set.empty) + ref still allocates a fresh CHAMP path per edge. On the heavy book that is plausibly the dominant term, not the outer chain you removed: each of the 5,000 driver cells accumulates a 9,900-element dependents set built by 9,900 successive immutable +, i.e. ~50M copy-on-write set updates with an O(log n) node path each. The outer-map fix (8.5 GB by your JFR) is real, but this may well be bigger.

The same LinkedHashSet trick applies and is order-exact for the same reason: accumulate into mutable.LinkedHashSet and freeze with .toSet at the end — ≤4 elements gives insertion-ordered Set1..4 (identical to repeated +), >4 gives a canonical HashSet (identical to repeated +). Turns O(edges × log) path copying into O(edges) appends. Worth measuring on the heavy book; if it lands, it is a second multiple on top of the 3.4× RSS.

3. fromSheet / extractDependenciesBounded got none of the three fixes (DependencyGraph.scala:96, :505)

The sheet-level (unqualified ARef) path has all three of the same defects, verbatim: boundRange re-expands per referencing formula with no memo, the binary operators are left-to-right ++, and :118 is the identical immutable foldLeft reverse-edge build you just replaced. This is not a cold path — DependentRecalculation.recalculateDependentsWithClock (:132) calls fromSheet per affected sheet inside the recalc loop, right after the fromWorkbook you did fix. So the -i put path still pays the range fan-out on the second graph build.

The blocker is that memoizedCells / union / reverseEdges are all typed at QualifiedRef. Generifying them to [K] is mechanical and would let fromSheet share the machinery — the union guard argument is type-independent, and reverseEdges already reads generically. Fine as a follow-up if you would rather keep this PR contained; worth a line in #522 either way so it does not get lost behind "range-compressed edges are the end state."

4. memoizedCells hands a mutable.HashMap into a closure that escapes down the recursion (DependencyGraph.scala:1199)

Correct today — both fromWorkbook and fromWorkbookBounded drive the extraction through sequential flatMap, and cellsFor is constructed fresh per build so nothing leaks across calls. But the closure gets threaded all the way down through extractQualifiedDependencies → locCells → the recursive NameRef / SheetNameRef re-entry, and the PR body says #520's wave-parallel evaluation stacks on top of this. If any future change parallelizes the per-sheet or per-formula extraction (.par, parTraverse), an unsynchronized mutable.HashMap corrupts silently — lost entries or a spin in getOrElseUpdate, not an exception. Given the stacking order, please state the single-threaded contract explicitly in the scaladoc ("the returned function is NOT thread-safe; one instance per sequential build"). Cheap insurance against exactly the change that is queued behind this one.

Minor, same function: the (SheetName, CellRange) key allocates a Tuple2 per lookup — ~50M on the heavy book. Trivially dwarfed by what you are saving, but a nested HashMap[SheetName, HashMap[CellRange, …]] would drop it if you are already in there.

5. Retention tradeoff on the unbounded fromWorkbook path (DependencyGraph.scala:976)

memoizedCells(unboundedQualifiedCells) caches unclipped expansions for the lifetime of the build. For a workbook where each formula references a distinct large range (SUM(B2:B5000), SUM(C2:C5000), … — an ordinary column-wise model), the memo takes zero hits and is pure retention: the expanded set stays live in the cache and in the deps map. Worse for a whole-column SUM(A:A), which is 1,048,576 refs unclipped and now pinned for the whole build.

Mitigating: your union change actually helps here — union(Set.empty, rangeSet) hits the swap branch and HashSet.concat with an empty argument returns this, so the single-range case shares one instance rather than duplicating. So it is narrow. But it does mean the memo can raise peak RSS on a shape it does not speed up, and fromWorkbook is the unbounded variant used by recalculateDependents and ReadCommands.scala:255. Two options: bound fromWorkbook's expansion the way fromWorkbookBounded does (probably a separate issue — it is a semantic change), or just note the tradeoff in the scaladoc so the next person measuring RSS on a column-wise book is not surprised.

6. Test coverage — the main gap

No tests in the diff. Every invariant this PR rests on is cheap to pin, and the repo's law-based-testing convention (CLAUDE.md) is a natural fit:

• union law — forAll { (a, b) => union(a, b) == a ++ b && union(a, b).toList == (a ++ b).toList }. The .toList half is the one that matters: it is the order claim, and it is the claim a reader has to take on faith right now. (Heads up if you write this: the invariant is "identical modulo full-32-bit hash collisions." Colliding elements land in a HashCollisionSetNode backed by an insertion-ordered Vector, so b ++ a can reorder within a collision bucket. Astronomically unlikely for QualifiedRef and harmless — any resulting Kahn order is still a valid topological order — but a ScalaCheck generator with a deliberately collision-prone key type would find it, so scope the property to QualifiedRef.)
• reverseEdges equivalence — property test against the naive fold it replaces, asserting both == and per-key .toList equality. This would have surfaced finding Add Claude Code GitHub Workflow #1 immediately if extended to the outer map's .toList.
• Memoization actually fires — wrap the expand argument in a counting function and assert one expansion per distinct (sheet, range) for a workbook with N formulas over the same range. Without this, a future refactor that moves val cellsFor inside the flatMap silently reverts the whole PR with every test still green.
• Regression guard — RecalcPerfSpec already has wall-clock budget tests in exactly this spirit (GH-518: topologicalSort on a 100k-cell chain…). A GH-522 sibling building the range-fanout shape (say 500 formulas × a 2,000-row driver column) and asserting graph build stays inside a budget would protect the 1.7× / 3.4× you measured. Right now nothing in CI would notice if it regressed.

The @SuppressWarnings(Array("…warts.Var")) on reverseEdges matches the existing convention in this file (:723, :791, :821, :846 all use it for mutable-collection code with no actual var), so no complaint there. Local mutation confined to a total function is fine under the purity charter, and you documented why — good.

---

Summary: #4 (thread-safety contract) I would want before merge given #520 stacks on top. #1 and #6 I would want because the PR's whole argument is a determinism claim, and neither the claim's one gap nor the claim itself is currently pinned by a test. #2 is a likely second win worth measuring. #3 and #5 are fine as follow-ups on #522.

Note: reviewed statically — the sandbox here blocked ./mill xl-evaluator.compile, so I did not independently re-run the suite.

[claude]

Review — GH-522 range-edge allocation

Nicely scoped work. Three independent, contained changes, each with the ordering argument written down next to the code rather than left implicit — that is the right way to land a perf fix in a codebase where kahnOrder/condensation determinism is a pinned gate. The measurements are convincing and the "what remains structural" note (range nodes) sets honest expectations.

I could not execute the suite in this environment (mill invocation is not permitted here), so the notes below are from static review; I am taking the reported green run at face value.

1. The sheet-level twin of the same defect is untouched

All three fixes land on the workbook-qualified path. The ARef path has the identical triple and keeps it:

• DependencyGraph.scala:119 — the reverse-edge fold, verbatim the one deleted at 1032.
• DependencyGraph.scala:505 extractDependenciesBounded — boundRange re-expands per occurrence with no memo, and every binary operator is still left ++ right.

That path is not cold: fromSheet drives SheetEvaluator.recalculate (SheetEvaluator.scala:282, :531), DependentRecalculation single-sheet (:59, :131), and the CLI --eval render path (ReadCommands.scala:435/500/586). A single-sheet financial model — the common shape — hits the same 9,900 × 5,000 expansion and gets none of the win.

reverseEdges needs nothing from QualifiedRef; making it private[formula] def reverseEdges[K](dependencies: Map[K, Set[K]]): Map[K, Set[K]] is a type-parameter edit that immediately covers line 119 too. The memo and the union are a bigger lift there (extractDependenciesBounded is a top-level recursive function, not a closure over a per-build cache), so splitting that into a follow-up is reasonable — but worth saying so in the PR body, since as written the description reads as if the shape is fixed, and it is fixed for fromWorkbook* only.

2. reverseEdges does not preserve the outer map iteration order for ≤4-key results

The doc comment claims the insertion sequence is identical "so every per-key set ... iterates exactly as before". The per-key sets, yes — same outer iteration, same + sequence. But the outer map is now mutable.HashMap.toMap, and toMap replays the mutable map bucket order into Map.newBuilder. Above 4 entries the result is a CHAMP HashMap and order is content-determined, so it matches. At ≤4 entries the result is Map1..Map4, which is insertion-ordered — and that insertion order is now bucket order, not the original fold order.

That is exactly the regime the condensation comment at DependencyGraph.scala:1122 flags: "small Scala Maps and Sets are insertion-ordered, which is exactly where test fixtures live."

I traced the consumers and do not think it is observable today — every one does a keyed getOrElse (kahnOrder:875, transitiveDependentsOf:710, and WorkbookEvaluator:250 prunedDependents, which feeds Kahn by lookup only), and the single place that touches the outer key set, StructuralEditor.scala:299 dependents.keySet.filter(...), funnels into .contains at :333. So: not a bug I can demonstrate. But the comment asserts an invariant stronger than what holds, in a file where that invariant is load-bearing. Either narrow the wording to per-key sets and note the outer order is bucket order (and why that is safe — all consumers are lookups), or keep the claim and pin it with a test.

3. Dead wart suppression

@SuppressWarnings(Array("org.wartremover.warts.Var")) at :1220 — there is no var in reverseEdges, and MutableDataStructures is not in the enabled list in build.mill:49-64, so nothing needed suppressing. Unlike the other five suppressions in this file, which all sit on real var/while bodies, this one will teach the next reader that mutable collections require a suppression here. Drop it.

4. Thread-safety of the two new mutable accumulators

The HashMap in memoizedCells and the one in reverseEdges are unsynchronized. Both are strictly sequential today. What makes me raise it: cellsFor is a closure that escapes into a recursive traversal, and the PR body says #520 stacks wave-parallel evaluation on top. getOrElseUpdate on a concurrently-resized mutable.HashMap does not just lose a memo entry, it can corrupt the table. A one-line "single-threaded per build — do not share across parallel traversals" on memoizedCells costs nothing and is the kind of comment that gets read at the moment it matters.

5. Memo retention window

The cache lives for the whole build, so any expanded range stays reachable until the graph is done. For the single-range case this is free — union(Set.empty, big) returns big itself, so the memoized instance is what lands in the deps map. Where it does cost is multi-range formulas over many distinct wide ranges (SUM(A1:A5000) + SUM(B1:B5000) × N): the union result is a fresh set and the cache additionally pins both operands, where previously they were transient. Bounded at roughly 2× and clearly dominated by the win you measured — but worth one sentence in the doc comment so the tradeoff is on the record rather than rediscovered.

6. union: make the empty short-circuit explicit

union(a, b) with a empty currently routes to b ++ a, and the zero-copy outcome depends on HashSet.concat detecting an empty non-HashSet operand and returning this. That fast path is what makes the common SUM(A1:A5000) fold share the memoized instance instead of rebuilding a 5,000-element trie — a good chunk of the RSS win rests on a collections-library implementation detail that nothing in the repo pins. Cheap to make explicit:

private def union(a: Set[QualifiedRef], b: Set[QualifiedRef]): Set[QualifiedRef] =
  if a.isEmpty then b
  else if b.isEmpty then a
  else if a.size < b.size && b.size > 4 then b ++ a
  else a ++ b

Order-safe by the same argument already given (identity on either side), and it also removes the Set.empty ++ smallSet rebuild in the ≤4 branch. Separately, the size-swap logic itself reads correct to me: the swap only fires when b.size > 4, the result is a superset of b and therefore always a CHAMP set, so no result that could have been an insertion-ordered SetN ever changes hands.

7. Test coverage

This is the gap I would most want closed before merge. Three changes justified entirely by (a) an ordering invariant and (b) an allocation profile, and neither is asserted anywhere — the existing suite passing is evidence, but evidence that will silently stop applying the next time someone touches this code. RecalcPerfSpec already houses exactly this kind of gate (GH-492 condensation budget, GH-518 100k-chain linearity). Suggested siblings:

1. Identity property — reverseEdges(g) equals the immutable fold it replaced, over ScalaCheck-generated Map[QualifiedRef, Set[QualifiedRef]]. Cheap, and it pins the contract that the mutable builder is a pure refactor. Deliberately include small (≤4-key) graphs.
2. Instance sharing — two formulas over the same range, then assert the two dependency sets are reference-equal (eq). Nothing currently guards the memo; someone could delete memoizedCells and the whole suite stays green while RSS goes back to 6 GB.
3. Range-fan-out budget — N formulas × one wide range through fromWorkbookBounded, with a wall/heap budget, in the shape of the evaluator: kahnOrder allocates O(V²) — topological sort is 81% of recalc allocation (28 GB garbage on a 50k-formula book) #518 gate. Without this the exact regression just fixed reappears unnoticed.

Minor

• DependentRecalculation.buildDependentsMap is now a one-line delegate to DependencyGraph.reverseEdges; consider inlining it at :120 and dropping the wrapper.
• memoizedCells allocates a Tuple2 and hashes the SheetName string per lookup. Immaterial against 5,000 avoided QualifiedRefs, just noting it in case the memo ever gets used on a hot small-range path.

---

Net: this looks mergeable. Items 1 (sheet-level coverage) and 7 (regression gates) are the ones I would want addressed either here or as a tracked follow-up; 2, 3, and 6 are small and worth folding in now.

🤖 Generated with Claude Code