Skip to content

Commit e542e7a

Browse files
MaxDesiatovMaxDesiatov
committed
Use topologicalSort with Identifiable on ResolvedTarget
Fix fixes a serious performance regression introduced for deep target dependency graphs in #7160. After converting `ResolvedTarget` to a value type, its synthesized `Hashable` conformance traversed the whole dependency tree multiple times when using `topologicalSort`. This made package resolution seemingly hang for packages that had a lot of nested target graphs. We already have an implementation of `topologicalSort` on `Identifiable`, thus we should use that instead. I've also added a performance test for this in `PackageGraphPerfTests` to prevent future regressions in this area.
1 parent 5f85bff commit e542e7a

File tree

6 files changed

+109
-74
lines changed

6 files changed

+109
-74
lines changed

‎Sources/PackageGraph/GraphLoadingNode.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,3 +51,7 @@ extension GraphLoadingNode: CustomStringConvertible {
5151
}
5252
}
5353
}
54+
55+
extension GraphLoadingNode: Identifiable {
56+
public var id: PackageIdentity { self.identity }
57+
}

‎Sources/PackageGraph/PackageGraph+Loading.swift

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import OrderedCollections
1515
import PackageLoading
1616
import PackageModel
1717

18-
import func TSCBasic.topologicalSort
1918
import func TSCBasic.bestMatch
2019

2120
extension PackageGraph {

‎Sources/PackageGraph/Resolution/ResolvedTarget.swift

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@
1212

1313
import PackageModel
1414

15-
import func TSCBasic.topologicalSort
16-
1715
/// Represents a fully resolved target. All the dependencies for this target are also stored as resolved.
1816
public struct ResolvedTarget: Hashable {
1917
/// Represents dependency of a resolved target.
@@ -72,7 +70,7 @@ public struct ResolvedTarget: Hashable {
7270

7371
/// The name of this target.
7472
public var name: String {
75-
return underlying.name
73+
self.underlying.name
7674
}
7775

7876
/// Returns dependencies which satisfy the input build environment, based on their conditions.
@@ -84,49 +82,49 @@ public struct ResolvedTarget: Hashable {
8482

8583
/// Returns the recursive dependencies, across the whole package-graph.
8684
public func recursiveDependencies() throws -> [Dependency] {
87-
return try TSCBasic.topologicalSort(self.dependencies) { $0.dependencies }
85+
try topologicalSort(self.dependencies) { $0.dependencies }
8886
}
8987

9088
/// Returns the recursive target dependencies, across the whole package-graph.
9189
public func recursiveTargetDependencies() throws -> [ResolvedTarget] {
92-
return try TSCBasic.topologicalSort(self.dependencies) { $0.dependencies }.compactMap { $0.target }
90+
try topologicalSort(self.dependencies) { $0.dependencies }.compactMap { $0.target }
9391
}
9492

9593
/// Returns the recursive dependencies, across the whole package-graph, which satisfy the input build environment,
9694
/// based on their conditions.
9795
/// - Parameters:
9896
/// - environment: The build environment to use to filter dependencies on.
9997
public func recursiveDependencies(satisfying environment: BuildEnvironment) throws -> [Dependency] {
100-
return try TSCBasic.topologicalSort(dependencies(satisfying: environment)) { dependency in
101-
return dependency.dependencies.filter { $0.satisfies(environment) }
98+
try topologicalSort(dependencies(satisfying: environment)) { dependency in
99+
dependency.dependencies.filter { $0.satisfies(environment) }
102100
}
103101
}
104102

105103
/// The language-level target name.
106104
public var c99name: String {
107-
return underlying.c99name
105+
self.underlying.c99name
108106
}
109107

110108
/// Module aliases for dependencies of this target. The key is an
111109
/// original target name and the value is a new unique name mapped
112110
/// to the name of its .swiftmodule binary.
113111
public var moduleAliases: [String: String]? {
114-
return underlying.moduleAliases
112+
self.underlying.moduleAliases
115113
}
116114

117115
/// Allows access to package symbols from other targets in the package
118116
public var packageAccess: Bool {
119-
return underlying.packageAccess
117+
self.underlying.packageAccess
120118
}
121119

122120
/// The "type" of target.
123121
public var type: Target.Kind {
124-
return underlying.type
122+
self.underlying.type
125123
}
126124

127125
/// The sources for the target.
128126
public var sources: Sources {
129-
return underlying.sources
127+
self.underlying.sources
130128
}
131129

132130
let packageIdentity: PackageIdentity

‎Sources/SPMTestSupport/ResolvedTarget+Mock.swift

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the Swift open source project
4+
//
5+
// Copyright (c) 2014-2017 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See http://swift.org/LICENSE.txt for license information
9+
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
import PackageGraph
14+
import PackageModel
15+
16+
extension ResolvedTarget {
17+
public static func mock(
18+
packageIdentity: PackageIdentity,
19+
name: String,
20+
deps: ResolvedTarget...,
21+
conditions: [PackageCondition] = []
22+
) -> ResolvedTarget {
23+
ResolvedTarget(
24+
packageIdentity: packageIdentity,
25+
underlying: SwiftTarget(
26+
name: name,
27+
type: .library,
28+
path: .root,
29+
sources: Sources(paths: [], root: "/"),
30+
dependencies: [],
31+
packageAccess: false,
32+
swiftVersion: .v4,
33+
usesUnsafeFlags: false
34+
),
35+
dependencies: deps.map { .target($0, conditions: conditions) },
36+
defaultLocalization: nil,
37+
supportedPlatforms: [],
38+
platformVersionProvider: .init(implementation: .minimumDeploymentTargetDefault)
39+
)
40+
}
41+
}
42+

‎Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//
33
// This source file is part of the Swift open source project
44
//
5-
// Copyright (c) 2014-2017 Apple Inc. and the Swift project authors
5+
// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
66
// Licensed under Apache License v2.0 with Runtime Library Exception
77
//
88
// See http://swift.org/LICENSE.txt for license information
@@ -161,4 +161,22 @@ final class PackageGraphPerfTests: XCTestCasePerf {
161161
}
162162
}
163163
}
164+
165+
func testRecursiveDependencies() throws {
166+
var resolvedTarget = ResolvedTarget.mock(packageIdentity: "pkg", name: "t0")
167+
for i in 1..<1000 {
168+
resolvedTarget = ResolvedTarget.mock(packageIdentity: "pkg", name: "t\(i)", deps: resolvedTarget)
169+
}
170+
171+
let N = 100
172+
measure {
173+
do {
174+
for _ in 0..<N {
175+
_ = try resolvedTarget.recursiveTargetDependencies()
176+
}
177+
} catch {
178+
XCTFail("Loading package graph is not expected to fail in this test.")
179+
}
180+
}
181+
}
164182
}

‎Tests/PackageGraphTests/TargetTests.swift renamed to ‎Tests/PackageGraphTests/ResolvedTargetTests.swift

Lines changed: 34 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//
33
// This source file is part of the Swift open source project
44
//
5-
// Copyright (c) 2014-2018 Apple Inc. and the Swift project authors
5+
// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
66
// Licensed under Apache License v2.0 with Runtime Library Exception
77
//
88
// See http://swift.org/LICENSE.txt for license information
@@ -14,84 +14,58 @@ import XCTest
1414

1515
import PackageGraph
1616
@testable import PackageModel
17+
import SPMTestSupport
1718

18-
extension ResolvedTarget {
19-
fileprivate init(
20-
packageIdentity: String,
21-
name: String,
22-
deps: ResolvedTarget...,
23-
conditions: [PackageCondition] = []
24-
) {
25-
self.init(
26-
packageIdentity: .init(packageIdentity),
27-
underlying: SwiftTarget(
28-
name: name,
29-
type: .library,
30-
path: .root,
31-
sources: Sources(paths: [], root: "/"),
32-
dependencies: [],
33-
packageAccess: false,
34-
swiftVersion: .v4,
35-
usesUnsafeFlags: false
36-
),
37-
dependencies: deps.map { .target($0, conditions: conditions) },
38-
defaultLocalization: nil,
39-
supportedPlatforms: [],
40-
platformVersionProvider: .init(implementation: .minimumDeploymentTargetDefault)
41-
)
42-
}
43-
}
44-
45-
class TargetDependencyTests: XCTestCase {
19+
final class ResolvedTargetDependencyTests: XCTestCase {
4620
func test1() throws {
47-
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
48-
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
49-
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
21+
let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
22+
let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
23+
let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2)
5024

5125
XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
5226
XCTAssertEqual(try t2.recursiveTargetDependencies(), [t1])
5327
}
5428

5529
func test2() throws {
56-
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
57-
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
58-
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2, t1)
59-
let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t2, t3, t1)
30+
let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
31+
let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
32+
let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2, t1)
33+
let t4 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t4", deps: t2, t3, t1)
6034

6135
XCTAssertEqual(try t4.recursiveTargetDependencies(), [t3, t2, t1])
6236
XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
6337
XCTAssertEqual(try t2.recursiveTargetDependencies(), [t1])
6438
}
6539

6640
func test3() throws {
67-
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
68-
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
69-
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2, t1)
70-
let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t1, t2, t3)
41+
let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
42+
let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
43+
let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2, t1)
44+
let t4 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t4", deps: t1, t2, t3)
7145

7246
XCTAssertEqual(try t4.recursiveTargetDependencies(), [t3, t2, t1])
7347
XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
7448
XCTAssertEqual(try t2.recursiveTargetDependencies(), [t1])
7549
}
7650

7751
func test4() throws {
78-
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
79-
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
80-
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
81-
let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t3)
52+
let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
53+
let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
54+
let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2)
55+
let t4 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t4", deps: t3)
8256

8357
XCTAssertEqual(try t4.recursiveTargetDependencies(), [t3, t2, t1])
8458
XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
8559
XCTAssertEqual(try t2.recursiveTargetDependencies(), [t1])
8660
}
8761

8862
func test5() throws {
89-
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
90-
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
91-
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
92-
let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t3)
93-
let t5 = ResolvedTarget(packageIdentity: "pkg", name: "t5", deps: t2)
94-
let t6 = ResolvedTarget(packageIdentity: "pkg", name: "t6", deps: t5, t4)
63+
let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
64+
let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
65+
let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2)
66+
let t4 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t4", deps: t3)
67+
let t5 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t5", deps: t2)
68+
let t6 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t6", deps: t5, t4)
9569

9670
// precise order is not important, but it is important that the following are true
9771
let t6rd = try t6.recursiveTargetDependencies()
@@ -108,12 +82,12 @@ class TargetDependencyTests: XCTestCase {
10882
}
10983

11084
func test6() throws {
111-
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
112-
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
113-
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
114-
let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t3)
115-
let t5 = ResolvedTarget(packageIdentity: "pkg", name: "t5", deps: t2)
116-
let t6 = ResolvedTarget(
85+
let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
86+
let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
87+
let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2)
88+
let t4 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t4", deps: t3)
89+
let t5 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t5", deps: t2)
90+
let t6 = ResolvedTarget.mock(
11791
packageIdentity: "pkg",
11892
name: "t6",
11993
deps: t4,
@@ -135,10 +109,10 @@ class TargetDependencyTests: XCTestCase {
135109
}
136110

137111
func testConditions() throws {
138-
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
139-
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
140-
let t2NoConditions = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
141-
let t2WithConditions = ResolvedTarget(
112+
let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
113+
let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
114+
let t2NoConditions = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
115+
let t2WithConditions = ResolvedTarget.mock(
142116
packageIdentity: "pkg",
143117
name: "t2",
144118
deps: t1,

0 commit comments

Comments
 (0)