Reduce key path COW copies in Graph forEach and compactMapValues#1725
Conversation
Motivation: `_forEach` and `_compactMapValues` previously built each descendant's key path with `var childKeyPath = keyPath; childKeyPath.append(key)`. Because `childKeyPath` is COW-shared with `keyPath`, the subsequent `append` always triggers a copy of the whole key-path array. For a graph with `N` nodes at average depth `d`, this turns traversal into O(N * d) instead of the O(N) it should be. The fix threads a single `inout [K]` through the recursion and uses append/removeLast around the recursive call, so the key path is mutated in place. The public `forEach` and `compactMapValues` APIs are unchanged. Across isolated benchmarks at depths 3-6 with branching factor 3-5, traversal is 3-5x faster, with the speedup growing with depth - exactly as expected from the O(N * d) -> O(N) change. Adds a perf test gated by `performanceTestsEnabled` that verifies forEach traversal scales linearly with node count, mirroring the pattern in swiftlang#1626 (the symmetric fix on the insertion side).
inju2403
requested review from
briancroom,
grynspan,
harlanhaskins,
jerryjrchen and
stmontgomery
as code owners
|
The algorithmic improvements look sound here, and assuming it passes the existing test suite I think it’s a good change. Thanks for looking into this! |
The SD-based scaling check (mirrored from swiftlang#1626) doesn't yet behave reliably across CI invocations, per @harlanhaskins' review.
|
Makes sense to me. Perf test aside, do you have any metrics showing an improvement? |
Update the docstrings for the four `_forEach` / `_compactMapValues` overloads to describe the reused inout key path and the COW copy it avoids, per @harlanhaskins' review.
|
Thanks, @grynspan! There's a benchmark table in the PR description with isolated measurements (3-5× speedup, scaling with depth) — let me copy it here for visibility:
These are from an isolated micro-benchmark of just the keyPath copy pattern (release build, best of 20 runs, warmup ×5). The speedup grows roughly with depth — consistent with the O(N·d) → O(N) reduction. Happy to put together |
|
Thanks, sorry! Looks like it wasn't rendering in the GitHub app. |
Drop the inout-reuse rationale from the four `_forEach` / `_compactMapValues` overloads and keep just a one-line description of what the keyPath is.
|
@inju2403 Thanks! Feel free to merge. It should be set as the default, but if not, make sure it's set to "Squash and merge" |
|
I think you need to do that @harlanhaskins. |
|
Ah, sorry. I figured with approval they'd be able to. But at any rate, thanks @inju2403! |
3 participants
This avoids an accidentally-superlinear COW copy of the key path during
Graph.forEachandGraph.compactMapValuestraversal.Motivation:
While reading @harlanhaskins' #1626 (which fixes a quadratic COW issue on
Graph.insertValue), I noticed that_forEachand_compactMapValuescarry a related — though less dramatic — version of the same pattern. Each recursive call builds the descendant's key path withvar childKeyPath = keyPath; childKeyPath.append(key), and becausechildKeyPathis COW-shared withkeyPath, theappendalways triggers a copy of the whole key-path array. For a graph withNnodes at average depthd, this turns traversal into O(N · d) instead of the O(N) it shouldbe.
In isolated benchmarks of the key-path copy pattern at depths 3–6 with branching factor 3–5, the new inout-based version is consistently 3–5× faster, with the speedup growing with depth — exactly as expected from the O(N ·
d) → O(N) change:
Modifications:
The four
_forEach/_compactMapValuesoverloads (sync + async for each) now thread a singleinout [K]through the recursion. After invoking the body, the child's key is appended once, recursion descends, and the key ispopped on the way back up — mutating the key path in place rather than copying it on every descent. The public
forEach(_:)andcompactMapValues(_:)wrappers create the initialvar keyPath: [K] = []and pass it through; thepublic API surface is unchanged.
All six current
forEachcallsites (two inRunner.Plan, four inAdvancedConsoleOutputRecorder) consume the key path inline — none of them escape it — so the existing semantics are preserved. The existingGraphTestsforforEach,forEach (async), andcompactMapValuespass unchanged.Checklist: