Skip to content

Commit fc49d53

Browse files
ahoppenahoppen
authored
Change traverseModules to return modules with their dependencies (#7977)
Companion of swiftlang/sourcekit-lsp#1683 --- Opening the SourceKit-LSP workspace using a debug build of sourcekit-lsp takes about 35s because `traverseModules` generates about 800,000 callback calls. All SourceKit-LSP needs now are the modules and their dependencies, so we can refactor `traverseModules` to just return that information avoid visiting the same modules multiple times. With theses changes getting all modules and their dependencies only takes ~0.013s. rdar://136107035
1 parent b3fc0a3 commit fc49d53

File tree

6 files changed

+108
-56
lines changed

6 files changed

+108
-56
lines changed

‎Sources/Basics/Graph/GraphAlgorithms.swift

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,6 @@ public func depthFirstSearch<T: Hashable>(
9090
private struct TraversalNode<T: Hashable>: Hashable {
9191
let parent: T?
9292
let curr: T
93-
let depth: Int
9493
}
9594

9695
/// Implements a pre-order depth-first search that traverses the whole graph and
@@ -108,28 +107,27 @@ private struct TraversalNode<T: Hashable>: Hashable {
108107
public func depthFirstSearch<T: Hashable>(
109108
_ nodes: [T],
110109
successors: (T) throws -> [T],
111-
onNext: (T, _ parent: T?, _ depth: Int) throws -> Void
110+
onNext: (T, _ parent: T?) throws -> Void
112111
) rethrows {
113112
var stack = OrderedSet<TraversalNode<T>>()
114113

115114
for node in nodes {
116115
precondition(stack.isEmpty)
117-
stack.append(TraversalNode(parent: nil, curr: node, depth: 0))
116+
stack.append(TraversalNode(parent: nil, curr: node))
118117

119118
while !stack.isEmpty {
120119
let node = stack.removeLast()
121120

122-
try onNext(node.curr, node.parent, node.depth)
121+
try onNext(node.curr, node.parent)
123122

124123
for succ in try successors(node.curr) {
125124
stack.append(
126125
TraversalNode(
127126
parent: node.curr,
128-
curr: succ,
129-
depth: node.depth + 1
127+
curr: succ
130128
)
131129
)
132130
}
133131
}
134132
}
135-
}
133+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ extension BuildPlan {
215215
}
216216

217217
return result.filter { uniqueNodes.insert($0).inserted }
218-
} onNext: { node, _, _ in
218+
} onNext: { node, _ in
219219
switch node {
220220
case .package: break
221221
case .product(let product, let destination):

‎Sources/Build/BuildPlan/BuildPlan.swift

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1074,12 +1074,16 @@ extension BuildPlan {
10741074
package func traverseModules(
10751075
_ onModule: (
10761076
(ResolvedModule, BuildParameters.Destination),
1077-
_ parent: (ResolvedModule, BuildParameters.Destination)?,
1078-
_ depth: Int
1077+
_ parent: (ResolvedModule, BuildParameters.Destination)?
10791078
) -> Void
10801079
) {
1080+
var visited = Set<TraversalNode>()
1081+
10811082
func successors(for package: ResolvedPackage) -> [TraversalNode] {
1082-
package.modules.compactMap {
1083+
guard visited.insert(.package(package)).inserted else {
1084+
return []
1085+
}
1086+
return package.modules.compactMap {
10831087
if case .test = $0.underlying.type,
10841088
!self.graph.rootPackages.contains(id: package.id)
10851089
{
@@ -1093,7 +1097,10 @@ extension BuildPlan {
10931097
for module: ResolvedModule,
10941098
destination: Destination
10951099
) -> [TraversalNode] {
1096-
module.dependencies.reduce(into: [TraversalNode]()) { partial, dependency in
1100+
guard visited.insert(.module(module, destination)).inserted else {
1101+
return []
1102+
}
1103+
return module.dependencies.reduce(into: [TraversalNode]()) { partial, dependency in
10971104
switch dependency {
10981105
case .product(let product, conditions: _):
10991106
let parent = TraversalNode(product: product, context: destination)
@@ -1115,7 +1122,7 @@ extension BuildPlan {
11151122
case .product:
11161123
[]
11171124
}
1118-
} onNext: { current, parent, depth in
1125+
} onNext: { current, parent in
11191126
let parentModule: (ResolvedModule, BuildParameters.Destination)? = switch parent {
11201127
case .package, .product, nil:
11211128
nil
@@ -1128,7 +1135,7 @@ extension BuildPlan {
11281135
break
11291136

11301137
case .module(let module, let destination):
1131-
onModule((module, destination), parentModule, depth)
1138+
onModule((module, destination), parentModule)
11321139
}
11331140
}
11341141
}
@@ -1177,7 +1184,7 @@ extension BuildPlan {
11771184
case .package:
11781185
[]
11791186
}
1180-
} onNext: { module, _, _ in
1187+
} onNext: { module, _ in
11811188
switch module {
11821189
case .package:
11831190
break

‎Sources/SourceKitLSPAPI/BuildDescription.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -168,17 +168,17 @@ public struct BuildDescription {
168168
}
169169

170170
public func traverseModules(
171-
callback: (any BuildTarget, _ parent: (any BuildTarget)?, _ depth: Int) -> Void
171+
callback: (any BuildTarget, _ parent: (any BuildTarget)?) -> Void
172172
) {
173-
self.buildPlan.traverseModules { module, parent, depth in
173+
self.buildPlan.traverseModules { module, parent in
174174
let parentDescription: (any BuildTarget)? = if let parent {
175175
getBuildTarget(for: parent.0, destination: parent.1)
176176
} else {
177177
nil
178178
}
179179

180180
if let description = getBuildTarget(for: module.0, destination: module.1) {
181-
callback(description, parentDescription, depth)
181+
callback(description, parentDescription)
182182
}
183183
}
184184
}

‎Tests/BuildTests/BuildPlanTraversalTests.swift

Lines changed: 2 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ final class BuildPlanTraversalTests: XCTestCase {
2626
struct Result {
2727
let parent: (ResolvedModule, Dest)?
2828
let module: (ResolvedModule, Dest)
29-
let depth: Int
3029
}
3130

3231
func getResults(
@@ -62,20 +61,6 @@ final class BuildPlanTraversalTests: XCTestCase {
6261
}.sorted()
6362
}
6463

65-
func getUniqueOccurrences(
66-
in results: [Result],
67-
for module: String,
68-
destination: Dest? = nil
69-
) -> [Int] {
70-
self.getResults(
71-
for: module,
72-
with: destination,
73-
in: results
74-
).reduce(into: Set<Int>()) {
75-
$0.insert($1.depth)
76-
}.sorted()
77-
}
78-
7964
func testTrivialTraversal() async throws {
8065
let destinationTriple = Triple.arm64Linux
8166
let toolsTriple = Triple.x86_64MacOS
@@ -97,16 +82,12 @@ final class BuildPlanTraversalTests: XCTestCase {
9782

9883
var results: [Result] = []
9984
plan.traverseModules {
100-
results.append(Result(parent: $1, module: $0, depth: $2))
85+
results.append(Result(parent: $1, module: $0))
10186
}
10287

10388
XCTAssertEqual(self.getParents(in: results, for: "app"), [])
10489
XCTAssertEqual(self.getParents(in: results, for: "lib"), ["app", "test"])
10590
XCTAssertEqual(self.getParents(in: results, for: "test"), [])
106-
107-
XCTAssertEqual(self.getUniqueOccurrences(in: results, for: "app"), [1])
108-
XCTAssertEqual(self.getUniqueOccurrences(in: results, for: "lib"), [1, 2])
109-
XCTAssertEqual(self.getUniqueOccurrences(in: results, for: "test"), [1])
11091
}
11192

11293
func testTraversalWithDifferentDestinations() async throws {
@@ -130,18 +111,13 @@ final class BuildPlanTraversalTests: XCTestCase {
130111

131112
var results: [Result] = []
132113
plan.traverseModules {
133-
results.append(Result(parent: $1, module: $0, depth: $2))
114+
results.append(Result(parent: $1, module: $0))
134115
}
135116

136117
XCTAssertEqual(self.getParents(in: results, for: "MMIO"), ["HAL"])
137118
XCTAssertEqual(self.getParents(in: results, for: "SwiftSyntax", destination: .host), ["MMIOMacros"])
138119
XCTAssertEqual(self.getParents(in: results, for: "HAL", destination: .target), ["Core", "HALTests"])
139120
XCTAssertEqual(self.getParents(in: results, for: "HAL", destination: .host), [])
140-
141-
XCTAssertEqual(self.getUniqueOccurrences(in: results, for: "MMIO"), [1, 2, 3, 4])
142-
XCTAssertEqual(self.getUniqueOccurrences(in: results, for: "SwiftSyntax", destination: .target), [1])
143-
XCTAssertEqual(self.getUniqueOccurrences(in: results, for: "SwiftSyntax", destination: .host), [2, 3, 4, 5, 6])
144-
XCTAssertEqual(self.getUniqueOccurrences(in: results, for: "HAL"), [1, 2, 3])
145121
}
146122

147123
func testRecursiveDependencyTraversal() async throws {

‎Tests/SourceKitLSPAPITests/SourceKitLSPAPITests.swift

Lines changed: 83 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -141,24 +141,95 @@ final class SourceKitLSPAPITests: XCTestCase {
141141
)
142142
let description = BuildDescription(buildPlan: plan)
143143

144-
struct Result {
145-
let parent: (any BuildTarget)?
146-
let module: any BuildTarget
147-
let depth: Int
144+
struct Result: Equatable {
145+
let moduleName: String
146+
let moduleDestination: BuildDestination
147+
let parentName: String?
148+
let parentDestination: BuildDestination?
148149
}
149150

150151
var results: [Result] = []
151-
description.traverseModules { current, parent, depth in
152-
results.append(Result(parent: parent, module: current, depth: depth))
152+
description.traverseModules { current, parent in
153+
results.append(
154+
Result(
155+
moduleName: current.name,
156+
moduleDestination: current.destination,
157+
parentName: parent?.name,
158+
parentDestination: parent?.destination
159+
)
160+
)
153161
}
154162

155-
XCTAssertEqual(results.count, 6)
163+
XCTAssertEqual(
164+
results,
165+
[
166+
Result(moduleName: "lib", moduleDestination: .target, parentName: nil, parentDestination: nil),
167+
Result(moduleName: "plugin", moduleDestination: .host, parentName: nil, parentDestination: nil),
168+
Result(moduleName: "exe", moduleDestination: .host, parentName: "plugin", parentDestination: .host),
169+
Result(moduleName: "lib", moduleDestination: .host, parentName: "exe", parentDestination: .host),
170+
Result(moduleName: "exe", moduleDestination: .target, parentName: nil, parentDestination: nil),
171+
Result(moduleName: "lib", moduleDestination: .target, parentName: "exe", parentDestination: .target),
172+
]
173+
)
174+
}
175+
176+
func testModuleTraversalRecordsDependencyOfVisitedNode() async throws {
177+
let fs = InMemoryFileSystem(
178+
emptyFiles:
179+
"/Pkg/Sources/exe/main.swift",
180+
"/Pkg/Sources/lib/lib.swift"
181+
)
182+
183+
let observability = ObservabilitySystem.makeForTesting()
184+
let graph = try loadModulesGraph(
185+
fileSystem: fs,
186+
manifests: [
187+
Manifest.createRootManifest(
188+
displayName: "Pkg",
189+
path: "/Pkg",
190+
targets: [
191+
TargetDescription(name: "exe", dependencies: ["lib"]),
192+
TargetDescription(name: "lib", dependencies: [])
193+
]
194+
),
195+
],
196+
observabilityScope: observability.topScope
197+
)
198+
XCTAssertNoDiagnostics(observability.diagnostics)
199+
200+
let plan = try await BuildPlan(
201+
destinationBuildParameters: mockBuildParameters(
202+
destination: .target,
203+
shouldLinkStaticSwiftStdlib: true
204+
),
205+
toolsBuildParameters: mockBuildParameters(
206+
destination: .host,
207+
shouldLinkStaticSwiftStdlib: true
208+
),
209+
graph: graph,
210+
fileSystem: fs,
211+
observabilityScope: observability.topScope
212+
)
213+
let description = BuildDescription(buildPlan: plan)
156214

157-
// "lib" is the most interesting here because it appears on multiple depths due to
158-
// "exe" being a dependency of the "plugin".
159-
XCTAssertEqual(results.filter { $0.module.name == "lib" }.reduce(into: Set<Int>()) {
160-
$0.insert($1.depth)
161-
}.sorted(), [1, 2, 3])
215+
struct Result: Equatable {
216+
let moduleName: String
217+
let parentName: String?
218+
}
219+
220+
var results: [Result] = []
221+
description.traverseModules { current, parent in
222+
results.append(Result(moduleName: current.name, parentName: parent?.name))
223+
}
224+
225+
XCTAssertEqual(
226+
results,
227+
[
228+
Result(moduleName: "lib", parentName: nil),
229+
Result(moduleName: "exe", parentName: nil),
230+
Result(moduleName: "lib", parentName: "exe"),
231+
]
232+
)
162233
}
163234
}
164235

0 commit comments

Comments
 (0)