Skip to content

optimize manifest requests for registry based depedendcies #3929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 11, 2021
Merged

optimize manifest requests for registry based depedendcies #3929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/PackageLoading/ToolsVersionLoader.swift
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

customToolsVersion should only be used in tests, we should probably remove it from the public constructor and move it into an internal one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in #3933

let manifestLoader = try customManifestLoader ?? ManifestLoader(
toolchain: UserToolchain(destination: .hostDestination()).configuration,
cacheDir: location.sharedManifestsCacheDirectory
Expand Down
Loading