Skip to content

Commit

Permalink
optimize manifest requests for registry based depedendcies
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@tomerd
tomerd committed Dec 10, 2021
1 parent e4de038 commit a8a0e22
Show file tree
Hide file tree
Showing 8 changed files with 618 additions and 123 deletions.
2 changes: 1 addition & 1 deletion Sources/PackageLoading/ToolsVersionLoader.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ public struct ToolsVersionLoader: ToolsVersionLoaderProtocol {

public func load(at path: AbsolutePath, fileSystem: FileSystem) throws -> ToolsVersion {
// The file which contains the tools version.
let file = try Manifest.path(atPackagePath: path, currentToolsVersion: currentToolsVersion, fileSystem: fileSystem)
let file = try Manifest.path(atPackagePath: path, currentToolsVersion: self.currentToolsVersion, fileSystem: fileSystem)
guard fileSystem.isFile(file) else {
// FIXME: We should return an error from here but Workspace tests rely on this in order to work.
// This doesn't really cause issues (yet) in practice though.
Expand Down
8 changes: 4 additions & 4 deletions Sources/PackageRegistry/RegistryClient.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ public final class RegistryClient {
timeout: DispatchTimeInterval? = .none,
observabilityScope: ObservabilityScope,
callbackQueue: DispatchQueue,
completion: @escaping (Result<[String: ToolsVersion], Error>) -> Void
completion: @escaping (Result<[String: (toolsVersion: ToolsVersion, content: String?)], Error>) -> Void
) {
let completion = self.makeAsync(completion, on: callbackQueue)

Expand Down Expand Up @@ -227,13 +227,13 @@ public final class RegistryClient {
throw RegistryError.invalidResponse
}

var result = [String: ToolsVersion]()
var result = [String: (toolsVersion: ToolsVersion, content: String?)]()
let toolsVersion = try ToolsVersionLoader().load(utf8String: manifestContent)
result[Manifest.filename] = toolsVersion
result[Manifest.filename] = (toolsVersion: toolsVersion, content: manifestContent)

let alternativeManifests = try response.headers.get("Link").map { try parseLinkHeader($0) }.flatMap { $0 }
for alternativeManifest in alternativeManifests {
result[alternativeManifest.filename] = alternativeManifest.toolsVersion
result[alternativeManifest.filename] = (toolsVersion: alternativeManifest.toolsVersion, content: .none)
}
return result
}.mapError {
Expand Down
182 changes: 99 additions & 83 deletions Sources/Workspace/RegistryPackageContainer.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public class RegistryPackageContainer: PackageContainer {
private var toolsVersionsCache = ThreadSafeKeyValueStore<Version, ToolsVersion>()
private var validToolsVersionsCache = ThreadSafeKeyValueStore<Version, Bool>()
private var manifestsCache = ThreadSafeKeyValueStore<Version, Manifest>()
private var availableManifestsCache = ThreadSafeKeyValueStore<Version, (manifests: [String: (toolsVersion: ToolsVersion, content: String?)], fileSystem: FileSystem)>()

public init(
package: PackageReference,
Expand Down Expand Up @@ -65,24 +66,10 @@ public class RegistryPackageContainer: PackageContainer {

public func toolsVersion(for version: Version) throws -> ToolsVersion {
try self.toolsVersionsCache.memoize(version) {
let manifests = try temp_await {
self.registryClient.getAvailableManifests(
package: self.package.identity,
version: version,
observabilityScope: self.observabilityScope,
callbackQueue: .sharedConcurrent,
completion: $0
)
}

// ToolsVersionLoader is designed to scan files to decide which is the best tools-version
// as such, this writes a fake manifest based on the information returned by the registry
// with only the header line which is all that is needed by ToolsVersionLoader
let fileSystem = InMemoryFileSystem()
for manifest in manifests {
try fileSystem.writeFileContents(AbsolutePath.root.appending(component: manifest.key), string: "// swift-tools-version:\(manifest.value)")
let result = try temp_await {
self.getAvailableManifestsFilesystem(version: version, completion: $0)
}
return try self.toolsVersionLoader.load(at: .root, fileSystem: fileSystem)
return try self.toolsVersionLoader.load(at: .root, fileSystem: result.fileSystem)
}
}

Expand Down Expand Up @@ -125,95 +112,124 @@ public class RegistryPackageContainer: PackageContainer {
return self.package
}

private func loadManifest(version: Version) throws -> Manifest {
// internal for testing
internal func loadManifest(version: Version) throws -> Manifest {
return try self.manifestsCache.memoize(version) {
try temp_await {
loadManifest(version: version, completion: $0)
self.loadManifest(version: version, completion: $0)
}
}
}

// FIXME: make this DRYer with toolsVersion(for:)
private func loadManifest(version: Version, completion: @escaping (Result<Manifest, Error>) -> Void) {
self.registryClient.getAvailableManifests(
package: self.package.identity,
version: version,
observabilityScope: self.observabilityScope,
callbackQueue: .sharedConcurrent
) { result in
self.getAvailableManifestsFilesystem(version: version) { result in
switch result {
case .failure(let error):
return completion(.failure(error))
case .success(let manifests):
case .success(let result):
do {
let fileSystem = InMemoryFileSystem()
let manifests = result.manifests
let fileSystem = result.fileSystem

// first, decide the tools-version we should use
// ToolsVersionLoader is designed to scan files to decide which is the best tools-version
// as such, this writes a fake manifest based on the information returned by the registry
// with only the header line which is all that is needed by ToolsVersionLoader
for manifest in manifests {
try fileSystem.writeFileContents(AbsolutePath.root.appending(component: manifest.key), string: "// swift-tools-version:\(manifest.value)")
}
guard let mainToolsVersion = manifests.first(where: { $0.key == Manifest.filename })?.value else {
let preferredToolsVersion = try self.toolsVersionLoader.load(at: .root, fileSystem: fileSystem)
// validate preferred the tools version is compatible with the current toolchain
try preferredToolsVersion.validateToolsVersion(
self.currentToolsVersion,
packageIdentity: self.package.identity
)
// load the manifest content
guard let defaultManifestToolsVersion = manifests.first(where: { $0.key == Manifest.filename })?.value.toolsVersion else {
throw StringError("Could not find the '\(Manifest.filename)' file for '\(self.package.identity)' '\(version)'")
}
let preferredToolsVersion = try self.toolsVersionLoader.load(at: .root, fileSystem: fileSystem)
let customToolsVersion = preferredToolsVersion != mainToolsVersion ? preferredToolsVersion : nil

// now that we know the tools version we need, fetch the manifest content
self.registryClient.getManifestContent(
package: self.package.identity,
version: version,
customToolsVersion: customToolsVersion,
observabilityScope: self.observabilityScope,
callbackQueue: .sharedConcurrent
) { result in
switch result {
case .failure(let error):
return completion(.failure(error))
case .success(let manifestContent):
do {
// replace the fake manifest with the real manifest content
let filename: String
if let toolsVersion = customToolsVersion {
filename = Manifest.basename + "@swift-\(toolsVersion).swift"
} else {
filename = Manifest.filename
}
try fileSystem.writeFileContents(AbsolutePath.root.appending(component: filename), string: manifestContent)

// validate the tools version.
try preferredToolsVersion.validateToolsVersion(
self.currentToolsVersion,
packageIdentity: self.package.identity
)

// finally, load the manifest
self.manifestLoader.load(
at: .root,
packageIdentity: self.package.identity,
packageKind: self.package.kind,
packageLocation: self.package.locationString,
version: version,
revision: nil,
toolsVersion: preferredToolsVersion,
identityResolver: self.identityResolver,
fileSystem: fileSystem,
observabilityScope: self.observabilityScope,
on: .sharedConcurrent,
completion: completion
)
} catch {
if preferredToolsVersion == defaultManifestToolsVersion {
// default tools version - we already have the content on disk from getAvailableManifestsFileSystem()
self.manifestLoader.load(
at: .root,
packageIdentity: self.package.identity,
packageKind: self.package.kind,
packageLocation: self.package.locationString,
version: version,
revision: nil,
toolsVersion: preferredToolsVersion,
identityResolver: self.identityResolver,
fileSystem: result.fileSystem,
observabilityScope: self.observabilityScope,
on: .sharedConcurrent,
completion: completion
)
} else {
// custom tools-version, we need to fetch the content from the server
self.registryClient.getManifestContent(
package: self.package.identity,
version: version,
customToolsVersion: preferredToolsVersion,
observabilityScope: self.observabilityScope,
callbackQueue: .sharedConcurrent
) { result in
switch result {
case .failure(let error):
return completion(.failure(error))
case .success(let manifestContent):
do {
// replace the fake manifest with the real manifest content
let manifestPath = AbsolutePath.root.appending(component: Manifest.basename + "@swift-\(preferredToolsVersion).swift")
try fileSystem.removeFileTree(manifestPath)
try fileSystem.writeFileContents(manifestPath, string: manifestContent)
// finally, load the manifest
self.manifestLoader.load(
at: .root,
packageIdentity: self.package.identity,
packageKind: self.package.kind,
packageLocation: self.package.locationString,
version: version,
revision: nil,
toolsVersion: preferredToolsVersion,
identityResolver: self.identityResolver,
fileSystem: fileSystem,
observabilityScope: self.observabilityScope,
on: .sharedConcurrent,
completion: completion
)
} catch {
return completion(.failure(error))
}
}
}
}
}
} catch {
return completion(.failure(error))
}
}
}
}

private func getAvailableManifestsFilesystem(version: Version, completion: @escaping (Result<(manifests: [String: (toolsVersion: ToolsVersion, content: String?)], fileSystem: FileSystem), Error>) -> Void) {
// try cached first
if let availableManifests = self.availableManifestsCache[version] {
return completion(.success(availableManifests))
}
// get from server
self.registryClient.getAvailableManifests(
package: self.package.identity,
version: version,
observabilityScope: self.observabilityScope,
callbackQueue: .sharedConcurrent
) { result in
completion(result.tryMap { manifests in
// ToolsVersionLoader is designed to scan files to decide which is the best tools-version
// as such, this writes a fake manifest based on the information returned by the registry
// with only the header line which is all that is needed by ToolsVersionLoader
let fileSystem = InMemoryFileSystem()
for manifest in manifests {
let content = manifest.value.content ?? "// swift-tools-version:\(manifest.value.toolsVersion)"
try fileSystem.writeFileContents(AbsolutePath.root.appending(component: manifest.key), string: content)
}
self.availableManifestsCache[version] = (manifests: manifests, fileSystem: fileSystem)
return (manifests: manifests, fileSystem: fileSystem)
})
}
}
}

// MARK: - CustomStringConvertible
Expand Down
2 changes: 1 addition & 1 deletion Sources/Workspace/Workspace.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ public class Workspace {
) throws {
// defaults
let currentToolsVersion = customToolsVersion ?? ToolsVersion.currentToolsVersion
let toolsVersionLoader = ToolsVersionLoader()
let toolsVersionLoader = ToolsVersionLoader(currentToolsVersion: currentToolsVersion)
let manifestLoader = try customManifestLoader ?? ManifestLoader(
toolchain: UserToolchain(destination: .hostDestination()).configuration,
cacheDir: location.sharedManifestsCacheDirectory
Expand Down
Loading

0 comments on commit a8a0e22

Please sign in to comment.