Skip to content

Commit bd820f8

Browse files
authored
Reduce key path COW copies in Graph forEach and compactMapValues (#1725)
This avoids an accidentally-superlinear COW copy of the key path during `Graph.forEach` and `Graph.compactMapValues` traversal. ### Motivation: While reading [@harlanhaskins](https://github.com/harlanhaskins)' [#1626](#1626) (which fixes a quadratic COW issue on `Graph.insertValue`), I noticed that `_forEach` and `_compactMapValues` carry a related — though less dramatic — version of the same pattern. Each recursive call builds the descendant's key path with `var childKeyPath = keyPath; childKeyPath.append(key)`, and because `childKeyPath` is COW-shared with `keyPath`, the `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. 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: | Config | Nodes | Old | New | Speedup | | --- | --- | --- | --- | --- | | depth 3, branch 3 | 40 | 0.006 ms | 0.002 ms | 3.51× | | depth 4, branch 4 | 341 | 0.059 ms | 0.012 ms | 4.84× | | depth 4, branch 5 | 781 | 0.138 ms | 0.029 ms | 4.84× | | depth 5, branch 4 | 1,365 | 0.242 ms | 0.046 ms | 5.21× | | depth 6, branch 4 | 5,461 | 0.830 ms | 0.159 ms | 5.23× | ### Modifications: The four `_forEach` / `_compactMapValues` overloads (sync + async for each) now thread a single `inout [K]` through the recursion. After invoking the body, the child's key is appended once, recursion descends, and the key is popped on the way back up — mutating the key path in place rather than copying it on every descent. The public `forEach(_:)` and `compactMapValues(_:)` wrappers create the initial `var keyPath: [K] = []` and pass it through; the public API surface is unchanged. All six current `forEach` callsites (two in `Runner.Plan`, four in `AdvancedConsoleOutputRecorder`) consume the key path inline — none of them escape it — so the existing semantics are preserved. The existing `GraphTests` for `forEach`, `forEach (async)`, and `compactMapValues` pass unchanged. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
1 parent ae4bccb commit bd820f8

1 file changed

Lines changed: 30 additions & 30 deletions

File tree

Sources/Testing/Support/Graph.swift

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -389,36 +389,34 @@ extension Graph {
389389
/// The recursive implementation of `forEach(_:)`.
390390
///
391391
/// - Parameters:
392-
/// - keyPath: The key path to use for the root node when passing it to
393-
/// `body`.
392+
/// - keyPath: The key path of the current node, passed to `body`.
394393
/// - body: A closure that is invoked once per element in the graph. The key
395394
/// path and leaf value of each node are passed to the closure.
396395
///
397396
/// - Throws: Whatever is thrown by `body`.
398-
private func _forEach<E>(keyPath: [K], _ body: (Element) throws(E) -> Void) throws(E) {
397+
private func _forEach<E>(keyPath: inout [K], _ body: (Element) throws(E) -> Void) throws(E) {
399398
try body((keyPath, value))
400399
for (key, child) in children {
401-
var childKeyPath = keyPath
402-
childKeyPath.append(key)
403-
try child._forEach(keyPath: childKeyPath, body)
400+
keyPath.append(key)
401+
try child._forEach(keyPath: &keyPath, body)
402+
keyPath.removeLast()
404403
}
405404
}
406405

407406
/// The recursive implementation of `forEach(_:)`.
408407
///
409408
/// - Parameters:
410-
/// - keyPath: The key path to use for the root node when passing it to
411-
/// `body`.
409+
/// - keyPath: The key path of the current node, passed to `body`.
412410
/// - body: A closure that is invoked once per element in the graph. The
413411
/// key path and leaf value of each node are passed to the closure.
414412
///
415413
/// - Throws: Whatever is thrown by `body`.
416-
private func _forEach<E>(keyPath: [K], _ body: (Element) async throws(E) -> Void) async throws(E) {
414+
private func _forEach<E>(keyPath: inout [K], _ body: (Element) async throws(E) -> Void) async throws(E) {
417415
try await body((keyPath, value))
418416
for (key, child) in children {
419-
var childKeyPath = keyPath
420-
childKeyPath.append(key)
421-
try await child._forEach(keyPath: childKeyPath, body)
417+
keyPath.append(key)
418+
try await child._forEach(keyPath: &keyPath, body)
419+
keyPath.removeLast()
422420
}
423421
}
424422

@@ -432,7 +430,8 @@ extension Graph {
432430
///
433431
/// This function iterates depth-first.
434432
func forEach<E>(_ body: (Element) throws(E) -> Void) throws(E) {
435-
try _forEach(keyPath: []) { (element) throws(E) in
433+
var keyPath: [K] = []
434+
try _forEach(keyPath: &keyPath) { (element) throws(E) in
436435
try body(element)
437436
}
438437
}
@@ -447,7 +446,8 @@ extension Graph {
447446
///
448447
/// This function iterates depth-first.
449448
func forEach<E>(_ body: (Element) async throws(E) -> Void) async throws(E) {
450-
try await _forEach(keyPath: []) { (element) async throws(E) in
449+
var keyPath: [K] = []
450+
try await _forEach(keyPath: &keyPath) { (element) async throws(E) in
451451
try await body(element)
452452
}
453453
}
@@ -546,16 +546,16 @@ extension Graph {
546546
///
547547
/// This function iterates depth-first.
548548
func compactMapValues<U, E>(_ transform: (Element) throws(E) -> (U, recursivelyApply: Bool)?) throws(E) -> Graph<K, U>? {
549-
try _compactMapValues(keyPath: []) { (element) throws(E) in
549+
var keyPath: [K] = []
550+
return try _compactMapValues(keyPath: &keyPath) { (element) throws(E) in
550551
try transform(element)
551552
}
552553
}
553554

554555
/// The recursive implementation of `compactMapValues(_:)`.
555556
///
556557
/// - Parameters:
557-
/// - keyPath: The key path to use for the root node when passing it to
558-
/// `transform`.
558+
/// - keyPath: The key path of the current node, passed to `transform`.
559559
/// - transform: A closure that is invoked once per element in the graph.
560560
/// The key path and leaf value of each node are passed to this closure.
561561
/// The result of the closure is a tuple containing the new value and
@@ -565,21 +565,21 @@ extension Graph {
565565
/// child nodes are omitted from the new graph.
566566
///
567567
/// - Throws: Whatever is thrown by `transform`.
568-
private func _compactMapValues<U, E>(keyPath: [K], _ transform: (Element) throws(E) -> (U, recursivelyApply: Bool)?) throws(E) -> Graph<K, U>? {
568+
private func _compactMapValues<U, E>(keyPath: inout [K], _ transform: (Element) throws(E) -> (U, recursivelyApply: Bool)?) throws(E) -> Graph<K, U>? {
569569
guard let (newValue, recursivelyApply) = try transform((keyPath, value)) else {
570570
return nil
571571
}
572572

573573
var newChildren = [K: Graph<K,U>]()
574574
newChildren.reserveCapacity(children.count)
575575
for (key, child) in children {
576-
var childKeyPath = keyPath
577-
childKeyPath.append(key)
576+
keyPath.append(key)
577+
defer { keyPath.removeLast() }
578578

579579
if recursivelyApply {
580-
newChildren[key] = child._compactMapValues(keyPath: childKeyPath) { _ in (newValue, true) }
580+
newChildren[key] = child._compactMapValues(keyPath: &keyPath) { _ in (newValue, true) }
581581
} else {
582-
newChildren[key] = try child._compactMapValues(keyPath: childKeyPath, transform)
582+
newChildren[key] = try child._compactMapValues(keyPath: &keyPath, transform)
583583
}
584584
}
585585

@@ -606,16 +606,16 @@ extension Graph {
606606
///
607607
/// This function iterates depth-first.
608608
func compactMapValues<U, E>(_ transform: (Element) async throws(E) -> (U, recursivelyApply: Bool)?) async throws(E) -> Graph<K, U>? {
609-
try await _compactMapValues(keyPath: []) { (element) async throws(E) in
609+
var keyPath: [K] = []
610+
return try await _compactMapValues(keyPath: &keyPath) { (element) async throws(E) in
610611
try await transform(element)
611612
}
612613
}
613614

614615
/// The recursive implementation of `compactMapValues(_:)`.
615616
///
616617
/// - Parameters:
617-
/// - keyPath: The key path to use for the root node when passing it to
618-
/// `transform`.
618+
/// - keyPath: The key path of the current node, passed to `transform`.
619619
/// - transform: A closure that is invoked once per element in the graph.
620620
/// The key path and leaf value of each node are passed to this closure.
621621
/// The result of the closure is a tuple containing the new value and
@@ -625,21 +625,21 @@ extension Graph {
625625
/// child nodes are omitted from the new graph.
626626
///
627627
/// - Throws: Whatever is thrown by `transform`.
628-
private func _compactMapValues<U, E>(keyPath: [K], _ transform: (Element) async throws(E) -> (U, recursivelyApply: Bool)?) async throws(E) -> Graph<K, U>? {
628+
private func _compactMapValues<U, E>(keyPath: inout [K], _ transform: (Element) async throws(E) -> (U, recursivelyApply: Bool)?) async throws(E) -> Graph<K, U>? {
629629
guard let (newValue, recursivelyApply) = try await transform((keyPath, value)) else {
630630
return nil
631631
}
632632

633633
var newChildren = [K: Graph<K,U>]()
634634
newChildren.reserveCapacity(children.count)
635635
for (key, child) in children {
636-
var childKeyPath = keyPath
637-
childKeyPath.append(key)
636+
keyPath.append(key)
637+
defer { keyPath.removeLast() }
638638

639639
if recursivelyApply {
640-
newChildren[key] = child._compactMapValues(keyPath: childKeyPath) { _ in (newValue, true) }
640+
newChildren[key] = child._compactMapValues(keyPath: &keyPath) { _ in (newValue, true) }
641641
} else {
642-
newChildren[key] = try await child._compactMapValues(keyPath: childKeyPath, transform)
642+
newChildren[key] = try await child._compactMapValues(keyPath: &keyPath, transform)
643643
}
644644
}
645645

0 commit comments

Comments
 (0)