Skip to content

dependency resolution performance #4007

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
Jan 13, 2022
Merged

dependency resolution performance #4007

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
170 changes: 7 additions & 163 deletions Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1035,13 +1035,6 @@ internal final class PubGrubPackageContainer {
/// Reference to the pins map.
private let pinsMap: PinsStore.PinsMap

/// The map of dependencies to version set that indicates the versions that have had their
/// incompatibilities emitted.
private var emittedIncompatibilities = ThreadSafeKeyValueStore<DependencyResolutionNode, VersionSetSpecifier>()

/// Whether we've emitted the incompatibilities for the pinned versions.
private var emittedPinnedVersionIncompatibilities = ThreadSafeKeyValueStore<DependencyResolutionNode, Bool>()

init(underlying: PackageContainer, pinsMap: PinsStore.PinsMap) {
self.underlying = underlying
self.pinsMap = pinsMap
Expand Down Expand Up @@ -1189,55 +1182,17 @@ internal final class PubGrubPackageContainer {
continue
}

// Skip if we already emitted incompatibilities for this dependency such that the selected
// falls within the previously computed bounds.
if emittedIncompatibilities[node]?.contains(version) != true {
for node in dep.nodes() {
constraints.append(
PackageContainerConstraint(
package: node.package,
requirement: dep.requirement,
products: node.productFilter
)
for node in dep.nodes() {
constraints.append(
PackageContainerConstraint(
package: node.package,
requirement: dep.requirement,
products: node.productFilter
)
}
}
}

// Emit the dependencies at the pinned version if we haven't emitted anything else yet.
if version == pinnedVersion, emittedIncompatibilities.isEmpty {
// We don't need to emit anything if we already emitted the incompatibilities at the
// pinned version.
if self.emittedPinnedVersionIncompatibilities[node] ?? false {
return []
}

self.emittedPinnedVersionIncompatibilities[node] = true

// Since the pinned version is most likely to succeed, we don't compute bounds for its
// incompatibilities.
return try constraints.flatMap { constraint -> [Incompatibility] in
// We only have version-based requirements at this point.
guard case .versionSet(let constraintRequirement) = constraint.requirement else {
throw InternalError("Unexpected unversioned requirement: \(constraint)")
}
return try constraint.nodes().map { dependencyNode in
var terms: OrderedSet<Term> = []
// the package version requirement
terms.append(Term(node, .exact(version)))
// the dependency's version requirement
terms.append(Term(not: dependencyNode, constraintRequirement))
return try Incompatibility(terms, root: root, cause: .dependency(node: node))
}
)
}
}

let (lowerBounds, upperBounds) = try self.computeBounds(
for: node,
constraints: constraints,
startingWith: version
)

return try constraints.flatMap { constraint -> [Incompatibility] in
// We only have version-based requirements at this point.
guard case .versionSet(let constraintRequirement) = constraint.requirement else {
Expand All @@ -1249,13 +1204,6 @@ internal final class PubGrubPackageContainer {
return nil
}

// Make a record for this dependency so we don't have to recompute the bounds when the selected version falls within the bounds.
if let lowerBound = lowerBounds[constraintNode], let upperBound = upperBounds[constraintNode] {
assert(lowerBound < upperBound)
let requirement: VersionSetSpecifier = .range(lowerBound ..< upperBound)
self.emittedIncompatibilities[constraintNode] = requirement.union(emittedIncompatibilities[constraintNode] ?? .empty)
}

var terms: OrderedSet<Term> = []
// the package version requirement
terms.append(Term(node, .exact(version)))
Expand All @@ -1266,110 +1214,6 @@ internal final class PubGrubPackageContainer {
}
}
}

/// Method for computing bounds of the given dependencies.
///
/// This will return a dictionary which contains mapping of a package dependency to its bound.
/// If a dependency is absent in the dictionary, it is present in all versions of the package
/// above or below the given version. As with regular version ranges, the lower bound is
/// inclusive and the upper bound is exclusive.
private func computeBounds(
for node: DependencyResolutionNode,
constraints: [PackageContainerConstraint],
startingWith firstVersion: Version
) throws -> (lowerBounds: [DependencyResolutionNode: Version], upperBounds: [DependencyResolutionNode: Version]) {
let preloadCount = 3

// nothing to do
if constraints.isEmpty {
return ([:], [:])
}

func preload(_ versions: [Version]) {
let sync = DispatchGroup()
for version in versions {
DispatchQueue.sharedConcurrent.async(group: sync) {
if self.underlying.isToolsVersionCompatible(at: version) {
_ = try? self.underlying.getDependencies(at: version, productFilter: node.productFilter)
}
}
}
sync.wait()
}

func compute(_ versions: [Version], upperBound: Bool) -> [DependencyResolutionNode: Version] {
var result: [DependencyResolutionNode: Version] = [:]
var previousVersion = firstVersion

for (index, version) in versions.enumerated() {
// optimization to preload few version in parallel.
// we cant preload all versions ahead of time since it will be more
// costly given the early termination condition
if index.isMultiple(of: preloadCount) {
preload(Array(versions[index ..< min(index + preloadCount, versions.count)]))
}

// Record this version as the bound if we're finding upper bounds since
// upper bound is exclusive and record the previous version if we're
// finding the lower bound since that is inclusive.
let bound = upperBound ? version : previousVersion

let isToolsVersionCompatible = self.underlying.isToolsVersionCompatible(at: version)
for constraint in constraints {
for constraintNode in constraint.nodes() where !result.keys.contains(constraintNode) {
// If we hit a version which doesn't have a compatible tools version then that's the boundary.
// Record the bound if the tools version isn't compatible at the current version.
if !isToolsVersionCompatible {
result[constraintNode] = bound
} else {
// Get the dependencies at this version.
if let currentDependencies = try? self.underlying.getDependencies(at: version, productFilter: constraintNode.productFilter) {
// Record this version as the bound for our list of dependencies, if appropriate.
if currentDependencies.first(where: { $0.package == constraint.package }) != constraint {
result[constraintNode] = bound
}
}
}
}
}

// We're done if we found bounds for all of our dependencies.
if result.count == constraints.count {
break
}

previousVersion = version
}

return result
}

let versions: [Version] = try self.underlying.versionsAscending()

guard let idx = versions.firstIndex(of: firstVersion) else {
throw InternalError("from version \(firstVersion) not found in \(node.package.identity)")
}

let sync = DispatchGroup()

var upperBounds = [DependencyResolutionNode: Version]()
DispatchQueue.sharedConcurrent.async(group: sync) {
upperBounds = compute(Array(versions.dropFirst(idx + 1)), upperBound: true)
}

var lowerBounds = [DependencyResolutionNode: Version]()
DispatchQueue.sharedConcurrent.async(group: sync) {
lowerBounds = compute(Array(versions.dropLast(versions.count - idx).reversed()), upperBound: false)
}

// timeout is a function of # of versions since we need to make several git operations per tag/version
let timeout = DispatchTimeInterval.seconds(60 + versions.count)
guard case .success = sync.wait(timeout: .now() + timeout) else {
throw StringError("timeout computing '\(node.package.identity)' bounds")
}

return (lowerBounds, upperBounds)
}
}

/// An utility class around PackageContainerProvider that allows "prefetching" the containers
Expand Down
4 changes: 4 additions & 0 deletions Sources/PackageLoading/ManifestLoader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
at: path,
packageIdentity: packageIdentity,
packageKind: packageKind,
version: version,
toolsVersion: toolsVersion,
identityResolver: identityResolver,
delegateQueue: queue,
Expand Down Expand Up @@ -528,6 +529,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
at path: AbsolutePath,
packageIdentity: PackageIdentity,
packageKind: PackageReference.Kind,
version: Version?,
toolsVersion: ToolsVersion,
identityResolver: IdentityResolver,
delegateQueue: DispatchQueue,
Expand Down Expand Up @@ -569,6 +571,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
do {
// try to get it from the cache
if let result = try cache?.get(key: key.sha256Checksum), let manifestJSON = result.manifestJSON, !manifestJSON.isEmpty {
observabilityScope.emit(debug: "loading manifest for '\(packageIdentity)' v. \(version?.description ?? "unknown") from cache")
return completion(.success(try self.parseManifest(
result,
packageIdentity: packageIdentity,
Expand All @@ -587,6 +590,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
let closeAfterWrite = closeAfterRead.delay()

// shells out and compiles the manifest, finally output a JSON
observabilityScope.emit(debug: "evaluating manifest for '\(packageIdentity)' v. \(version?.description ?? "unknown") ")
self.evaluateManifest(
packageIdentity: key.packageIdentity,
manifestPath: key.manifestPath,
Expand Down
9 changes: 7 additions & 2 deletions Sources/Workspace/Workspace.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,19 @@ private class WorkspaceRepositoryManagerDelegate: RepositoryManagerDelegate {
}

private struct WorkspaceDependencyResolverDelegate: DependencyResolverDelegate {
unowned let workspaceDelegate: WorkspaceDelegate
private unowned let workspaceDelegate: WorkspaceDelegate
private let resolving = ThreadSafeKeyValueStore<PackageIdentity, Bool>()

init(_ delegate: WorkspaceDelegate) {
self.workspaceDelegate = delegate
}

func willResolve(term: Term) {
self.workspaceDelegate.willComputeVersion(package: term.node.package.identity, location: term.node.package.locationString)
// this may be called multiple time by the resolver for various version ranges, but we only want to propagate once since we report at pacakge level
resolving.memoize(term.node.package.identity) {
self.workspaceDelegate.willComputeVersion(package: term.node.package.identity, location: term.node.package.locationString + " " + term.description)
return true
}
}

func didResolve(term: Term, version: Version, duration: DispatchTimeInterval) {
Expand Down
19 changes: 4 additions & 15 deletions Tests/PackageLoadingTests/PD_5_0_LoadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,22 +426,11 @@ class PackageDescription5_0LoadingTests: PackageDescriptionLoadingTests {
observabilityScope: observability.topScope
)

guard let diagnostic = observability.diagnostics.first else {
return XCTFail("Expected a diagnostic")
}
XCTAssertMatch(diagnostic.message, .contains("warning: initialization of immutable value"))
XCTAssertEqual(diagnostic.severity, .warning)
let contents = try diagnostic.metadata?.manifestLoadingDiagnosticFile.map { try localFileSystem.readFileContents($0) }
XCTAssertNotNil(contents)

// FIXME: (diagnostics) bring ^^ back when implemented again
/*guard let diagnostic = diagnostics.diagnostics.first else {
return XCTFail("Expected a diagnostic")
testDiagnostics(observability.diagnostics) { result in
let diagnostic = result.check(diagnostic: .contains("initialization of immutable value"), severity: .warning)
let contents = try diagnostic?.metadata?.manifestLoadingDiagnosticFile.map { try localFileSystem.readFileContents($0) }
XCTAssertNotNil(contents)
}
XCTAssertMatch(diagnostic.message.text, .contains("warning: initialization of immutable value"))
XCTAssertEqual(diagnostic.behavior, .warning)
let contents = try (diagnostic.data as? ManifestLoadingDiagnostic)?.diagnosticFile.map { try localFileSystem.readFileContents($0) }
XCTAssertNotNil(contents)*/
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/SPMBuildCoreTests/PluginInvocationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ class PluginInvocationTests: XCTestCase {

// Load the package graph.
let packageGraph = try workspace.loadPackageGraph(rootInput: rootInput, observabilityScope: observability.topScope)
XCTAssert(observability.diagnostics.isEmpty, "\(observability.diagnostics)")
XCTAssertNoDiagnostics(observability.diagnostics)
XCTAssert(packageGraph.packages.count == 1, "\(packageGraph.packages)")

// Find the build tool plugin.
Expand Down