Skip to content

Commit 7b8a90a

Browse files
tomerdtomerd
authored
optimize manifest requests for registry based depedendcies (#3929)
motivation: improve performance by reducing redundant http calls changes: * save main manifest on first request * cache known manifest across calls in RegistryPackageContainer * add RegistryPackageContainer tests * fix a bug in workspace where custom tools version is not propogated to the toolsLoader rdar://86218181
1 parent e0b1bd2 commit 7b8a90a

File tree

8 files changed

+618
-123
lines changed

8 files changed

+618
-123
lines changed

‎Sources/PackageLoading/ToolsVersionLoader.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ public struct ToolsVersionLoader: ToolsVersionLoaderProtocol {
328328

329329
public func load(at path: AbsolutePath, fileSystem: FileSystem) throws -> ToolsVersion {
330330
// The file which contains the tools version.
331-
let file = try Manifest.path(atPackagePath: path, currentToolsVersion: currentToolsVersion, fileSystem: fileSystem)
331+
let file = try Manifest.path(atPackagePath: path, currentToolsVersion: self.currentToolsVersion, fileSystem: fileSystem)
332332
guard fileSystem.isFile(file) else {
333333
// FIXME: We should return an error from here but Workspace tests rely on this in order to work.
334334
// This doesn't really cause issues (yet) in practice though.

‎Sources/PackageRegistry/RegistryClient.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ public final class RegistryClient {
185185
timeout: DispatchTimeInterval? = .none,
186186
observabilityScope: ObservabilityScope,
187187
callbackQueue: DispatchQueue,
188-
completion: @escaping (Result<[String: ToolsVersion], Error>) -> Void
188+
completion: @escaping (Result<[String: (toolsVersion: ToolsVersion, content: String?)], Error>) -> Void
189189
) {
190190
let completion = self.makeAsync(completion, on: callbackQueue)
191191

@@ -227,13 +227,13 @@ public final class RegistryClient {
227227
throw RegistryError.invalidResponse
228228
}
229229

230-
var result = [String: ToolsVersion]()
230+
var result = [String: (toolsVersion: ToolsVersion, content: String?)]()
231231
let toolsVersion = try ToolsVersionLoader().load(utf8String: manifestContent)
232-
result[Manifest.filename] = toolsVersion
232+
result[Manifest.filename] = (toolsVersion: toolsVersion, content: manifestContent)
233233

234234
let alternativeManifests = try response.headers.get("Link").map { try parseLinkHeader($0) }.flatMap { $0 }
235235
for alternativeManifest in alternativeManifests {
236-
result[alternativeManifest.filename] = alternativeManifest.toolsVersion
236+
result[alternativeManifest.filename] = (toolsVersion: alternativeManifest.toolsVersion, content: .none)
237237
}
238238
return result
239239
}.mapError {

‎Sources/Workspace/RegistryPackageContainer.swift

Lines changed: 99 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ public class RegistryPackageContainer: PackageContainer {
3030
private var toolsVersionsCache = ThreadSafeKeyValueStore<Version, ToolsVersion>()
3131
private var validToolsVersionsCache = ThreadSafeKeyValueStore<Version, Bool>()
3232
private var manifestsCache = ThreadSafeKeyValueStore<Version, Manifest>()
33+
private var availableManifestsCache = ThreadSafeKeyValueStore<Version, (manifests: [String: (toolsVersion: ToolsVersion, content: String?)], fileSystem: FileSystem)>()
3334

3435
public init(
3536
package: PackageReference,
@@ -65,24 +66,10 @@ public class RegistryPackageContainer: PackageContainer {
6566

6667
public func toolsVersion(for version: Version) throws -> ToolsVersion {
6768
try self.toolsVersionsCache.memoize(version) {
68-
let manifests = try temp_await {
69-
self.registryClient.getAvailableManifests(
70-
package: self.package.identity,
71-
version: version,
72-
observabilityScope: self.observabilityScope,
73-
callbackQueue: .sharedConcurrent,
74-
completion: $0
75-
)
76-
}
77-
78-
// ToolsVersionLoader is designed to scan files to decide which is the best tools-version
79-
// as such, this writes a fake manifest based on the information returned by the registry
80-
// with only the header line which is all that is needed by ToolsVersionLoader
81-
let fileSystem = InMemoryFileSystem()
82-
for manifest in manifests {
83-
try fileSystem.writeFileContents(AbsolutePath.root.appending(component: manifest.key), string: "// swift-tools-version:\(manifest.value)")
69+
let result = try temp_await {
70+
self.getAvailableManifestsFilesystem(version: version, completion: $0)
8471
}
85-
return try self.toolsVersionLoader.load(at: .root, fileSystem: fileSystem)
72+
return try self.toolsVersionLoader.load(at: .root, fileSystem: result.fileSystem)
8673
}
8774
}
8875

@@ -125,95 +112,124 @@ public class RegistryPackageContainer: PackageContainer {
125112
return self.package
126113
}
127114

128-
private func loadManifest(version: Version) throws -> Manifest {
115+
// internal for testing
116+
internal func loadManifest(version: Version) throws -> Manifest {
129117
return try self.manifestsCache.memoize(version) {
130118
try temp_await {
131-
loadManifest(version: version, completion: $0)
119+
self.loadManifest(version: version, completion: $0)
132120
}
133121
}
134122
}
135123

136-
// FIXME: make this DRYer with toolsVersion(for:)
137124
private func loadManifest(version: Version, completion: @escaping (Result<Manifest, Error>) -> Void) {
138-
self.registryClient.getAvailableManifests(
139-
package: self.package.identity,
140-
version: version,
141-
observabilityScope: self.observabilityScope,
142-
callbackQueue: .sharedConcurrent
143-
) { result in
125+
self.getAvailableManifestsFilesystem(version: version) { result in
144126
switch result {
145127
case .failure(let error):
146128
return completion(.failure(error))
147-
case .success(let manifests):
129+
case .success(let result):
148130
do {
149-
let fileSystem = InMemoryFileSystem()
131+
let manifests = result.manifests
132+
let fileSystem = result.fileSystem
133+
150134
// first, decide the tools-version we should use
151-
// ToolsVersionLoader is designed to scan files to decide which is the best tools-version
152-
// as such, this writes a fake manifest based on the information returned by the registry
153-
// with only the header line which is all that is needed by ToolsVersionLoader
154-
for manifest in manifests {
155-
try fileSystem.writeFileContents(AbsolutePath.root.appending(component: manifest.key), string: "// swift-tools-version:\(manifest.value)")
156-
}
157-
guard let mainToolsVersion = manifests.first(where: { $0.key == Manifest.filename })?.value else {
135+
let preferredToolsVersion = try self.toolsVersionLoader.load(at: .root, fileSystem: fileSystem)
136+
// validate preferred the tools version is compatible with the current toolchain
137+
try preferredToolsVersion.validateToolsVersion(
138+
self.currentToolsVersion,
139+
packageIdentity: self.package.identity
140+
)
141+
// load the manifest content
142+
guard let defaultManifestToolsVersion = manifests.first(where: { $0.key == Manifest.filename })?.value.toolsVersion else {
158143
throw StringError("Could not find the '\(Manifest.filename)' file for '\(self.package.identity)' '\(version)'")
159144
}
160-
let preferredToolsVersion = try self.toolsVersionLoader.load(at: .root, fileSystem: fileSystem)
161-
let customToolsVersion = preferredToolsVersion != mainToolsVersion ? preferredToolsVersion : nil
162-
163-
// now that we know the tools version we need, fetch the manifest content
164-
self.registryClient.getManifestContent(
165-
package: self.package.identity,
166-
version: version,
167-
customToolsVersion: customToolsVersion,
168-
observabilityScope: self.observabilityScope,
169-
callbackQueue: .sharedConcurrent
170-
) { result in
171-
switch result {
172-
case .failure(let error):
173-
return completion(.failure(error))
174-
case .success(let manifestContent):
175-
do {
176-
// replace the fake manifest with the real manifest content
177-
let filename: String
178-
if let toolsVersion = customToolsVersion {
179-
filename = Manifest.basename + "@swift-\(toolsVersion).swift"
180-
} else {
181-
filename = Manifest.filename
182-
}
183-
try fileSystem.writeFileContents(AbsolutePath.root.appending(component: filename), string: manifestContent)
184-
185-
// validate the tools version.
186-
try preferredToolsVersion.validateToolsVersion(
187-
self.currentToolsVersion,
188-
packageIdentity: self.package.identity
189-
)
190-
191-
// finally, load the manifest
192-
self.manifestLoader.load(
193-
at: .root,
194-
packageIdentity: self.package.identity,
195-
packageKind: self.package.kind,
196-
packageLocation: self.package.locationString,
197-
version: version,
198-
revision: nil,
199-
toolsVersion: preferredToolsVersion,
200-
identityResolver: self.identityResolver,
201-
fileSystem: fileSystem,
202-
observabilityScope: self.observabilityScope,
203-
on: .sharedConcurrent,
204-
completion: completion
205-
)
206-
} catch {
145+
if preferredToolsVersion == defaultManifestToolsVersion {
146+
// default tools version - we already have the content on disk from getAvailableManifestsFileSystem()
147+
self.manifestLoader.load(
148+
at: .root,
149+
packageIdentity: self.package.identity,
150+
packageKind: self.package.kind,
151+
packageLocation: self.package.locationString,
152+
version: version,
153+
revision: nil,
154+
toolsVersion: preferredToolsVersion,
155+
identityResolver: self.identityResolver,
156+
fileSystem: result.fileSystem,
157+
observabilityScope: self.observabilityScope,
158+
on: .sharedConcurrent,
159+
completion: completion
160+
)
161+
} else {
162+
// custom tools-version, we need to fetch the content from the server
163+
self.registryClient.getManifestContent(
164+
package: self.package.identity,
165+
version: version,
166+
customToolsVersion: preferredToolsVersion,
167+
observabilityScope: self.observabilityScope,
168+
callbackQueue: .sharedConcurrent
169+
) { result in
170+
switch result {
171+
case .failure(let error):
207172
return completion(.failure(error))
173+
case .success(let manifestContent):
174+
do {
175+
// replace the fake manifest with the real manifest content
176+
let manifestPath = AbsolutePath.root.appending(component: Manifest.basename + "@swift-\(preferredToolsVersion).swift")
177+
try fileSystem.removeFileTree(manifestPath)
178+
try fileSystem.writeFileContents(manifestPath, string: manifestContent)
179+
// finally, load the manifest
180+
self.manifestLoader.load(
181+
at: .root,
182+
packageIdentity: self.package.identity,
183+
packageKind: self.package.kind,
184+
packageLocation: self.package.locationString,
185+
version: version,
186+
revision: nil,
187+
toolsVersion: preferredToolsVersion,
188+
identityResolver: self.identityResolver,
189+
fileSystem: fileSystem,
190+
observabilityScope: self.observabilityScope,
191+
on: .sharedConcurrent,
192+
completion: completion
193+
)
194+
} catch {
195+
return completion(.failure(error))
196+
}
208197
}
209-
}
198+
}
210199
}
211200
} catch {
212201
return completion(.failure(error))
213202
}
214203
}
215204
}
216205
}
206+
207+
private func getAvailableManifestsFilesystem(version: Version, completion: @escaping (Result<(manifests: [String: (toolsVersion: ToolsVersion, content: String?)], fileSystem: FileSystem), Error>) -> Void) {
208+
// try cached first
209+
if let availableManifests = self.availableManifestsCache[version] {
210+
return completion(.success(availableManifests))
211+
}
212+
// get from server
213+
self.registryClient.getAvailableManifests(
214+
package: self.package.identity,
215+
version: version,
216+
observabilityScope: self.observabilityScope,
217+
callbackQueue: .sharedConcurrent
218+
) { result in
219+
completion(result.tryMap { manifests in
220+
// ToolsVersionLoader is designed to scan files to decide which is the best tools-version
221+
// as such, this writes a fake manifest based on the information returned by the registry
222+
// with only the header line which is all that is needed by ToolsVersionLoader
223+
let fileSystem = InMemoryFileSystem()
224+
for manifest in manifests {
225+
let content = manifest.value.content ?? "// swift-tools-version:\(manifest.value.toolsVersion)"
226+
try fileSystem.writeFileContents(AbsolutePath.root.appending(component: manifest.key), string: content)
227+
}
228+
self.availableManifestsCache[version] = (manifests: manifests, fileSystem: fileSystem)
229+
return (manifests: manifests, fileSystem: fileSystem)
230+
})
231+
}
232+
}
217233
}
218234

219235
// MARK: - CustomStringConvertible

‎Sources/Workspace/Workspace.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,7 @@ public class Workspace {
291291
) throws {
292292
// defaults
293293
let currentToolsVersion = customToolsVersion ?? ToolsVersion.currentToolsVersion
294-
let toolsVersionLoader = ToolsVersionLoader()
294+
let toolsVersionLoader = ToolsVersionLoader(currentToolsVersion: currentToolsVersion)
295295
let manifestLoader = try customManifestLoader ?? ManifestLoader(
296296
toolchain: UserToolchain(destination: .hostDestination()).configuration,
297297
cacheDir: location.sharedManifestsCacheDirectory

0 commit comments

Comments
 (0)