Skip to content

Commit 693b4ae

Browse files
MaxDesiatovMaxDesiatov
committed
Fix topologicalSort performance on ResolvedPackage 
1 parent 5ecf03d commit 693b4ae

File tree

8 files changed

+156
-35
lines changed

8 files changed

+156
-35
lines changed

‎Sources/Build/BuildPlan/BuildPlan+Test.swift

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ extension BuildPlan {
8585
testDiscoverySrc: Sources(paths: discoveryPaths, root: discoveryDerivedDir)
8686
)
8787
let discoveryResolvedTarget = ResolvedTarget(
88+
packageIdentity: testProduct.packageIdentity,
8889
underlying: discoveryTarget,
8990
dependencies: testProduct.targets.map { .target($0, conditions: []) },
9091
defaultLocalization: testProduct.defaultLocalization,
@@ -124,6 +125,7 @@ extension BuildPlan {
124125
testEntryPointSources: entryPointSources
125126
)
126127
let entryPointResolvedTarget = ResolvedTarget(
128+
packageIdentity: testProduct.packageIdentity,
127129
underlying: entryPointTarget,
128130
dependencies: testProduct.targets.map { .target($0, conditions: []) } + resolvedTargetDependencies,
129131
defaultLocalization: testProduct.defaultLocalization,
@@ -168,6 +170,7 @@ extension BuildPlan {
168170
testEntryPointSources: entryPointResolvedTarget.underlying.sources
169171
)
170172
let entryPointResolvedTarget = ResolvedTarget(
173+
packageIdentity: testProduct.packageIdentity,
171174
underlying: entryPointTarget,
172175
dependencies: entryPointResolvedTarget.dependencies + resolvedTargetDependencies,
173176
defaultLocalization: testProduct.defaultLocalization,

‎Sources/PackageGraph/PackageGraph+Loading.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,7 @@ private func createResolvedPackages(
373373
// Create target builders for each target in the package.
374374
let targetBuilders = package.targets.map {
375375
ResolvedTargetBuilder(
376+
packageIdentity: package.identity,
376377
target: $0,
377378
observabilityScope: packageObservabilityScope,
378379
platformVersionProvider: platformVersionProvider
@@ -861,6 +862,7 @@ private final class ResolvedProductBuilder: ResolvedBuilder<ResolvedProduct> {
861862

862863
override func constructImpl() throws -> ResolvedProduct {
863864
return ResolvedProduct(
865+
packageIdentity: packageBuilder.package.identity,
864866
product: product,
865867
targets: try targets.map{ try $0.construct() }
866868
)
@@ -882,6 +884,8 @@ private final class ResolvedTargetBuilder: ResolvedBuilder<ResolvedTarget> {
882884
/// The target reference.
883885
let target: Target
884886

887+
let packageIdentity: PackageIdentity
888+
885889
/// The target dependencies of this target.
886890
var dependencies: [Dependency] = []
887891

@@ -895,11 +899,13 @@ private final class ResolvedTargetBuilder: ResolvedBuilder<ResolvedTarget> {
895899
let platformVersionProvider: PlatformVersionProvider
896900

897901
init(
902+
packageIdentity: PackageIdentity,
898903
target: Target,
899904
observabilityScope: ObservabilityScope,
900905
platformVersionProvider: PlatformVersionProvider
901906
) {
902907
self.target = target
908+
self.packageIdentity = packageIdentity
903909
self.observabilityScope = observabilityScope
904910
self.platformVersionProvider = platformVersionProvider
905911
}
@@ -929,6 +935,7 @@ private final class ResolvedTargetBuilder: ResolvedBuilder<ResolvedTarget> {
929935
}
930936

931937
return ResolvedTarget(
938+
packageIdentity: self.packageIdentity,
932939
underlying: self.target,
933940
dependencies: dependencies,
934941
defaultLocalization: self.defaultLocalization,

‎Sources/PackageGraph/PackageGraph.swift

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
import PackageLoading
1414
import PackageModel
1515

16-
import func TSCBasic.topologicalSort
16+
import struct TSCBasic.OrderedSet
1717

1818
enum PackageGraphError: Swift.Error {
1919
/// Indicates a non-root package with no targets.
@@ -276,3 +276,75 @@ extension PackageGraphError: CustomStringConvertible {
276276
}
277277
}
278278
}
279+
280+
enum GraphError: Error {
281+
/// A cycle was detected in the input.
282+
case unexpectedCycle
283+
}
284+
285+
/// Perform a topological sort of a graph.
286+
///
287+
/// This function is optimized for use cases where cycles are unexpected, and
288+
/// does not attempt to retain information on the exact nodes in the cycle.
289+
///
290+
/// - Parameters:
291+
/// - nodes: The list of input nodes to sort.
292+
/// - successors: A closure for fetching the successors of a particular node.
293+
///
294+
/// - Returns: A list of the transitive closure of nodes reachable from the
295+
/// inputs, ordered such that every node in the list follows all of its
296+
/// predecessors.
297+
///
298+
/// - Throws: GraphError.unexpectedCycle
299+
///
300+
/// - Complexity: O(v + e) where (v, e) are the number of vertices and edges
301+
/// reachable from the input nodes via the relation.
302+
private func topologicalSort<T: Identifiable>(
303+
_ nodes: [T], successors: (T) throws -> [T]
304+
) throws -> [T] {
305+
// Implements a topological sort via recursion and reverse postorder DFS.
306+
func visit(
307+
_ node: T,
308+
_ stack: inout OrderedSet<T.ID>,
309+
_ visited: inout Set<T.ID>,
310+
_ result: inout [T.ID],
311+
_ successors: (T) throws -> [T]
312+
) throws {
313+
// Mark this node as visited -- we are done if it already was.
314+
if !visited.insert(node.id).inserted {
315+
return
316+
}
317+
318+
// Otherwise, visit each adjacent node.
319+
for succ in try successors(node) {
320+
guard stack.append(succ.id) else {
321+
// If the successor is already in this current stack, we have found a cycle.
322+
//
323+
// FIXME: We could easily include information on the cycle we found here.
324+
throw GraphError.unexpectedCycle
325+
}
326+
try visit(succ, &stack, &visited, &result, successors)
327+
let popped = stack.removeLast()
328+
assert(popped == succ.id)
329+
}
330+
331+
// Add to the result.
332+
result.append(node.id)
333+
}
334+
335+
let idsToNodes = Dictionary(uniqueKeysWithValues: nodes.map { ($0.id, $0) })
336+
337+
// FIXME: This should use a stack not recursion.
338+
var visited = Set<T.ID>()
339+
var result = [T.ID]()
340+
var stack = OrderedSet<T.ID>()
341+
for node in nodes {
342+
precondition(stack.isEmpty)
343+
stack.append(node.id)
344+
try visit(node, &stack, &visited, &result, successors)
345+
let popped = stack.removeLast()
346+
assert(popped == node.id)
347+
}
348+
349+
return result.reversed().compactMap { idsToNodes[$0] }
350+
}

‎Sources/PackageGraph/Resolution/ResolvedPackage.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,3 +87,9 @@ extension ResolvedPackage: CustomStringConvertible {
8787
return "<ResolvedPackage: \(self.identity)>"
8888
}
8989
}
90+
91+
extension ResolvedPackage: Identifiable {
92+
public var id: PackageIdentity {
93+
self.underlying.identity
94+
}
95+
}

‎Sources/PackageGraph/Resolution/ResolvedProduct.swift

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ public struct ResolvedProduct: Hashable {
2424
self.underlying.type
2525
}
2626

27+
public let packageIdentity: PackageIdentity
28+
2729
/// The underlying product.
2830
public let underlying: Product
2931

@@ -61,8 +63,9 @@ public struct ResolvedProduct: Hashable {
6163
}
6264
}
6365

64-
public init(product: Product, targets: [ResolvedTarget]) {
66+
public init(packageIdentity: PackageIdentity, product: Product, targets: [ResolvedTarget]) {
6567
assert(product.targets.count == targets.count && product.targets.map(\.name) == targets.map(\.name))
68+
self.packageIdentity = packageIdentity
6669
self.underlying = product
6770
self.targets = targets
6871

@@ -85,6 +88,7 @@ public struct ResolvedProduct: Hashable {
8588
testEntryPointPath: testEntryPointPath
8689
)
8790
return ResolvedTarget(
91+
packageIdentity: packageIdentity,
8892
underlying: swiftTarget,
8993
dependencies: targets.map { .target($0, conditions: []) },
9094
defaultLocalization: defaultLocalization ?? .none, // safe since this is a derived product

‎Sources/PackageGraph/Resolution/ResolvedTarget.swift

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,8 @@ public struct ResolvedTarget: Hashable {
129129
return underlying.sources
130130
}
131131

132+
let packageIdentity: PackageIdentity
133+
132134
/// The underlying target represented in this resolved target.
133135
public let underlying: Target
134136

@@ -148,12 +150,14 @@ public struct ResolvedTarget: Hashable {
148150

149151
/// Create a resolved target instance.
150152
public init(
153+
packageIdentity: PackageIdentity,
151154
underlying: Target,
152155
dependencies: [ResolvedTarget.Dependency],
153156
defaultLocalization: String? = nil,
154157
supportedPlatforms: [SupportedPlatform],
155158
platformVersionProvider: PlatformVersionProvider
156159
) {
160+
self.packageIdentity = packageIdentity
157161
self.underlying = underlying
158162
self.dependencies = dependencies
159163
self.defaultLocalization = defaultLocalization
@@ -190,3 +194,25 @@ extension ResolvedTarget.Dependency: CustomStringConvertible {
190194
return str
191195
}
192196
}
197+
198+
extension ResolvedTarget.Dependency: Identifiable {
199+
public struct ID: Hashable {
200+
enum Kind: Hashable {
201+
case target
202+
case product
203+
}
204+
205+
let kind: Kind
206+
let packageIdentity: PackageIdentity
207+
let name: String
208+
}
209+
210+
public var id: ID {
211+
switch self {
212+
case .target(let target, _):
213+
return .init(kind: .target, packageIdentity: target.packageIdentity, name: target.name)
214+
case .product(let product, _):
215+
return .init(kind: .product, packageIdentity: product.packageIdentity, name: product.name)
216+
}
217+
}
218+
}

‎Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -126,10 +126,12 @@ final class PackageGraphPerfTests: XCTestCasePerf {
126126
)
127127
],
128128
targets: [
129-
try .init(name: "Target\(sequenceNumber)",
130-
dependencies: dependencySequence.map {
131-
.product(name: "Target\($0)", package: "Package\($0)")
132-
})
129+
try .init(
130+
name: "Target\(sequenceNumber)",
131+
dependencies: dependencySequence.map {
132+
.product(name: "Target\($0)", package: "Package\($0)")
133+
}
134+
)
133135
]
134136
)
135137
}

‎Tests/PackageGraphTests/TargetTests.swift

Lines changed: 30 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,12 @@
1313
import XCTest
1414

1515
import PackageGraph
16-
import PackageModel
16+
@testable import PackageModel
1717

1818
private extension ResolvedTarget {
19-
init(name: String, deps: ResolvedTarget...) {
19+
init(packageIdentity: String, name: String, deps: ResolvedTarget...) {
2020
self.init(
21+
packageIdentity: .init(packageIdentity),
2122
underlying: SwiftTarget(
2223
name: name,
2324
type: .library,
@@ -48,9 +49,9 @@ class TargetDependencyTests: XCTestCase {
4849

4950
func test1() throws {
5051
testTargets {
51-
let t1 = ResolvedTarget(name: "t1")
52-
let t2 = ResolvedTarget(name: "t2", deps: t1)
53-
let t3 = ResolvedTarget(name: "t3", deps: t2)
52+
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
53+
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
54+
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
5455

5556
XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
5657
XCTAssertEqual(try t2.recursiveTargetDependencies(), [t1])
@@ -59,10 +60,10 @@ class TargetDependencyTests: XCTestCase {
5960

6061
func test2() throws {
6162
testTargets {
62-
let t1 = ResolvedTarget(name: "t1")
63-
let t2 = ResolvedTarget(name: "t2", deps: t1)
64-
let t3 = ResolvedTarget(name: "t3", deps: t2, t1)
65-
let t4 = ResolvedTarget(name: "t4", deps: t2, t3, t1)
63+
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
64+
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
65+
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2, t1)
66+
let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t2, t3, t1)
6667

6768
XCTAssertEqual(try t4.recursiveTargetDependencies(), [t3, t2, t1])
6869
XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
@@ -72,10 +73,10 @@ class TargetDependencyTests: XCTestCase {
7273

7374
func test3() throws {
7475
testTargets {
75-
let t1 = ResolvedTarget(name: "t1")
76-
let t2 = ResolvedTarget(name: "t2", deps: t1)
77-
let t3 = ResolvedTarget(name: "t3", deps: t2, t1)
78-
let t4 = ResolvedTarget(name: "t4", deps: t1, t2, t3)
76+
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
77+
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
78+
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2, t1)
79+
let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t1, t2, t3)
7980

8081
XCTAssertEqual(try t4.recursiveTargetDependencies(), [t3, t2, t1])
8182
XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
@@ -85,10 +86,10 @@ class TargetDependencyTests: XCTestCase {
8586

8687
func test4() throws {
8788
testTargets {
88-
let t1 = ResolvedTarget(name: "t1")
89-
let t2 = ResolvedTarget(name: "t2", deps: t1)
90-
let t3 = ResolvedTarget(name: "t3", deps: t2)
91-
let t4 = ResolvedTarget(name: "t4", deps: t3)
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)
9293

9394
XCTAssertEqual(try t4.recursiveTargetDependencies(), [t3, t2, t1])
9495
XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
@@ -98,12 +99,12 @@ class TargetDependencyTests: XCTestCase {
9899

99100
func test5() throws {
100101
testTargets {
101-
let t1 = ResolvedTarget(name: "t1")
102-
let t2 = ResolvedTarget(name: "t2", deps: t1)
103-
let t3 = ResolvedTarget(name: "t3", deps: t2)
104-
let t4 = ResolvedTarget(name: "t4", deps: t3)
105-
let t5 = ResolvedTarget(name: "t5", deps: t2)
106-
let t6 = ResolvedTarget(name: "t6", deps: t5, t4)
102+
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
103+
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
104+
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
105+
let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t3)
106+
let t5 = ResolvedTarget(packageIdentity: "pkg", name: "t5", deps: t2)
107+
let t6 = ResolvedTarget(packageIdentity: "pkg", name: "t6", deps: t5, t4)
107108

108109
// precise order is not important, but it is important that the following are true
109110
let t6rd = try t6.recursiveTargetDependencies()
@@ -122,12 +123,12 @@ class TargetDependencyTests: XCTestCase {
122123

123124
func test6() throws {
124125
testTargets {
125-
let t1 = ResolvedTarget(name: "t1")
126-
let t2 = ResolvedTarget(name: "t2", deps: t1)
127-
let t3 = ResolvedTarget(name: "t3", deps: t2)
128-
let t4 = ResolvedTarget(name: "t4", deps: t3)
129-
let t5 = ResolvedTarget(name: "t5", deps: t2)
130-
let t6 = ResolvedTarget(name: "t6", deps: t4, t5) // same as above, but these two swapped
126+
let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
127+
let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
128+
let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
129+
let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t3)
130+
let t5 = ResolvedTarget(packageIdentity: "pkg", name: "t5", deps: t2)
131+
let t6 = ResolvedTarget(packageIdentity: "pkg", name: "t6", deps: t4, t5) // same as above, but these two swapped
131132

132133
// precise order is not important, but it is important that the following are true
133134
let t6rd = try t6.recursiveTargetDependencies()

0 commit comments

Comments
 (0)