-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Paralellize retrieving resolved packages #8220
Open
fortmarek
wants to merge
5
commits into
swiftlang:main
Choose a base branch
from
tuist:workspace-state-actor
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e507854
Make WorkspaceState into actor to make it thread-safe
fortmarek f326ad8
Merge branch 'main' into workspace-state-actor
fortmarek 0cf2d7c
Parallelize retrieving resolved packages
fortmarek 3d99328
Make mutating ManagedDependencies thread-safe
fortmarek 7bdd76e
Change SwiftCommand conformance to AsyncParsableCommand
fortmarek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,7 +169,7 @@ extension Workspace { | |
} | ||
|
||
// Update the resolved file. | ||
try self.saveResolvedFile( | ||
try await self.saveResolvedFile( | ||
resolvedPackagesStore: resolvedPackagesStore, | ||
dependencyManifests: updatedDependencyManifests, | ||
originHash: resolvedFileOriginHash, | ||
|
@@ -219,7 +219,7 @@ extension Workspace { | |
case .update(let forceResolution): | ||
return try await resolveAndUpdateResolvedFile(forceResolution: forceResolution) | ||
case .bestEffort: | ||
guard !self.state.dependencies.hasEditedDependencies() else { | ||
guard await !self.state.dependencies.hasEditedDependencies() else { | ||
return try await resolveAndUpdateResolvedFile(forceResolution: false) | ||
} | ||
guard self.fileSystem.exists(self.location.resolvedVersionsFile) else { | ||
|
@@ -410,9 +410,10 @@ extension Workspace { | |
// | ||
// We require cloning if there is no checkout or if the checkout doesn't | ||
// match with the pin. | ||
let dependencies = await state.dependencies | ||
let requiredResolvedPackages = resolvedPackagesStore.resolvedPackages.values.filter { pin in | ||
// also compare the location in case it has changed | ||
guard let dependency = state.dependencies[comparingLocation: pin.packageRef] else { | ||
guard let dependency = dependencies[comparingLocation: pin.packageRef] else { | ||
return true | ||
} | ||
switch dependency.state { | ||
|
@@ -426,26 +427,31 @@ extension Workspace { | |
} | ||
|
||
// Retrieve the required resolved packages. | ||
for resolvedPackage in requiredResolvedPackages { | ||
await observabilityScope.makeChildScope( | ||
description: "retrieving resolved package versions for dependencies", | ||
metadata: resolvedPackage.packageRef.diagnosticsMetadata | ||
).trap { | ||
switch resolvedPackage.packageRef.kind { | ||
case .localSourceControl, .remoteSourceControl: | ||
_ = try await self.checkoutRepository( | ||
package: resolvedPackage.packageRef, | ||
at: resolvedPackage.state, | ||
observabilityScope: observabilityScope | ||
) | ||
case .registry: | ||
_ = try await self.downloadRegistryArchive( | ||
package: resolvedPackage.packageRef, | ||
at: resolvedPackage.state, | ||
observabilityScope: observabilityScope | ||
) | ||
default: | ||
throw InternalError("invalid resolved package type \(resolvedPackage.packageRef.kind)") | ||
await withThrowingTaskGroup(of: Void.self) { taskGroup in | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning back the parallelization of retrieving resolved packages |
||
for resolvedPackage in requiredResolvedPackages { | ||
let observabilityScope = observabilityScope.makeChildScope( | ||
description: "retrieving resolved package versions for dependencies", | ||
metadata: resolvedPackage.packageRef.diagnosticsMetadata | ||
) | ||
taskGroup.addTask { | ||
await observabilityScope.trap { | ||
switch resolvedPackage.packageRef.kind { | ||
case .localSourceControl, .remoteSourceControl: | ||
_ = try await self.checkoutRepository( | ||
package: resolvedPackage.packageRef, | ||
at: resolvedPackage.state, | ||
observabilityScope: observabilityScope | ||
) | ||
case .registry: | ||
_ = try await self.downloadRegistryArchive( | ||
package: resolvedPackage.packageRef, | ||
at: resolvedPackage.state, | ||
observabilityScope: observabilityScope | ||
) | ||
default: | ||
throw InternalError("invalid resolved package type \(resolvedPackage.packageRef.kind)") | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -523,7 +529,7 @@ extension Workspace { | |
} | ||
|
||
// load and update the `Package.resolved` store with any changes from loading the top level dependencies | ||
guard let resolvedPackagesStore = self.loadAndUpdateResolvedPackagesStore( | ||
guard let resolvedPackagesStore = await self.loadAndUpdateResolvedPackagesStore( | ||
dependencyManifests: currentManifests, | ||
rootManifestsMinimumToolsVersion: rootManifestsMinimumToolsVersion, | ||
observabilityScope: observabilityScope | ||
|
@@ -554,7 +560,7 @@ extension Workspace { | |
case .notRequired: | ||
// since nothing changed we can exit early, | ||
// but need update resolved file and download an missing binary artifact | ||
try self.saveResolvedFile( | ||
try await self.saveResolvedFile( | ||
resolvedPackagesStore: resolvedPackagesStore, | ||
dependencyManifests: currentManifests, | ||
originHash: resolvedFileOriginHash, | ||
|
@@ -625,7 +631,7 @@ extension Workspace { | |
} | ||
|
||
// Update the resolved file. | ||
try self.saveResolvedFile( | ||
try await self.saveResolvedFile( | ||
resolvedPackagesStore: resolvedPackagesStore, | ||
dependencyManifests: updatedDependencyManifests, | ||
originHash: resolvedFileOriginHash, | ||
|
@@ -679,15 +685,15 @@ extension Workspace { | |
|
||
// First remove the checkouts that are no longer required. | ||
for (packageRef, state) in packageStateChanges { | ||
observabilityScope.makeChildScope( | ||
await observabilityScope.makeChildScope( | ||
description: "removing unneeded checkouts", | ||
metadata: packageRef.diagnosticsMetadata | ||
).trap { | ||
switch state { | ||
case .added, .updated, .unchanged: | ||
break | ||
case .removed: | ||
try self.remove(package: packageRef) | ||
try await self.remove(package: packageRef) | ||
} | ||
} | ||
} | ||
|
@@ -772,8 +778,8 @@ extension Workspace { | |
state: .custom(version: version, path: path), | ||
subpath: RelativePath(validating: "") | ||
) | ||
self.state.dependencies.add(dependency) | ||
try self.state.save() | ||
await self.state.add(dependency: dependency) | ||
try await self.state.save() | ||
return path | ||
} else { | ||
throw InternalError("invalid container for \(package.identity) of type \(package.kind)") | ||
|
@@ -800,8 +806,8 @@ extension Workspace { | |
throw InternalError("invalid package type: \(package.kind)") | ||
} | ||
|
||
self.state.dependencies.add(dependency) | ||
try self.state.save() | ||
await self.state.add(dependency: dependency) | ||
try await self.state.save() | ||
return path | ||
} | ||
} | ||
|
@@ -889,7 +895,7 @@ extension Workspace { | |
dependencyManifests: DependencyManifests, | ||
rootManifestsMinimumToolsVersion: ToolsVersion, | ||
observabilityScope: ObservabilityScope | ||
) -> ResolvedPackagesStore? { | ||
) async -> ResolvedPackagesStore? { | ||
guard let resolvedPackagesStore = observabilityScope.trap({ try self.resolvedPackagesStore.load() }) else { | ||
return nil | ||
} | ||
|
@@ -899,7 +905,7 @@ extension Workspace { | |
else { | ||
return nil | ||
} | ||
for dependency in self.state.dependencies.filter(\.packageRef.kind.isResolvable) { | ||
for dependency in await self.state.dependencies.filter(\.packageRef.kind.isResolvable) { | ||
// a required dependency that is already loaded (managed) should be represented in the `Package.resolved` store. | ||
// also comparing location as it may have changed at this point | ||
if requiredDependencies.contains(where: { $0.equalsIncludingLocation(dependency.packageRef) }) { | ||
|
@@ -1011,13 +1017,13 @@ extension Workspace { | |
// Get the existing managed dependency for this package ref, if any. | ||
|
||
// first find by identity only since edit location may be different by design | ||
var currentDependency = self.state.dependencies[binding.package.identity] | ||
var currentDependency = await self.state.dependencies[binding.package.identity] | ||
// Check if this is an edited dependency. | ||
if case .edited(let basedOn, _) = currentDependency?.state, let originalReference = basedOn?.packageRef { | ||
packageStateChanges[originalReference.identity] = (originalReference, .unchanged) | ||
} else { | ||
// if not edited, also compare by location since it may have changed | ||
currentDependency = self.state.dependencies[comparingLocation: binding.package] | ||
currentDependency = await self.state.dependencies[comparingLocation: binding.package] | ||
} | ||
|
||
switch binding.boundVersion { | ||
|
@@ -1119,7 +1125,7 @@ extension Workspace { | |
} | ||
} | ||
// Set the state of any old package that might have been removed. | ||
for packageRef in self.state.dependencies.lazy.map(\.packageRef) | ||
for packageRef in await self.state.dependencies.lazy.map(\.packageRef) | ||
where packageStateChanges[packageRef.identity] == nil | ||
{ | ||
packageStateChanges[packageRef.identity] = (packageRef, .removed) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turning
ManagedDependencies
into astruct
. The alternative would be to make it anactor
but since it conforms toCollection
, it wouldn't be that easy.There are only two methods of
ManagedDependencies
that are mutable –add
andremove
. Instead of mutatingself
, we can return copy ofSelf
and do the mutation inWorkspaceState
.