Skip to content

Commit 106c681

Browse files
tomerdtomerd
authored
optimize repository updates (#6685)
motivation: improve performance changes: * instead of binary update flag, use a strategy that takes into account the revision needed * implement the update strategy for source control based dependency * update all call sites and tests * add tests
1 parent 4d26a9a commit 106c681

14 files changed

+335
-86
lines changed

‎Sources/PackageGraph/PackageContainer.swift

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,9 +166,18 @@ public protocol PackageContainerProvider {
166166
/// Get the container for a particular identifier asynchronously.
167167
func getContainer(
168168
for package: PackageReference,
169-
skipUpdate: Bool,
169+
updateStrategy: ContainerUpdateStrategy,
170170
observabilityScope: ObservabilityScope,
171171
on queue: DispatchQueue,
172-
completion: @escaping (Result<PackageContainer, Swift.Error>) -> Void
172+
completion: @escaping (Result<PackageContainer, Error>) -> Void
173173
)
174174
}
175+
176+
/// Only used for source control containers and as such a mirror of RepositoryUpdateStrategy
177+
/// This duplication is unfortunate - ideally this is not a concern of the ContainerProvider at all
178+
/// but it is required give how PackageContainerProvider currently integrated into the resolver
179+
public enum ContainerUpdateStrategy {
180+
case never
181+
case always
182+
case ifNeeded(revision: String)
183+
}

‎Sources/PackageGraph/PubGrub/ContainerProvider.swift

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,10 @@ final class ContainerProvider {
5757
}
5858

5959
/// Get the container for the given identifier, loading it if necessary.
60-
func getContainer(for package: PackageReference, completion: @escaping (Result<PubGrubPackageContainer, Error>) -> Void) {
60+
func getContainer(
61+
for package: PackageReference,
62+
completion: @escaping (Result<PubGrubPackageContainer, Error>) -> Void
63+
) {
6164
// Return the cached container, if available.
6265
if let container = self.containersCache[package], package.equalsIncludingLocation(container.package) {
6366
return completion(.success(container))
@@ -79,7 +82,7 @@ final class ContainerProvider {
7982
// Otherwise, fetch the container from the provider
8083
self.underlying.getContainer(
8184
for: package,
82-
skipUpdate: self.skipUpdate,
85+
updateStrategy: self.skipUpdate ? .never : .always, // TODO: make this more elaborate
8386
observabilityScope: self.observabilityScope.makeChildScope(description: "getting package container", metadata: package.diagnosticsMetadata),
8487
on: .sharedConcurrent
8588
) { result in
@@ -108,7 +111,7 @@ final class ContainerProvider {
108111
if needsFetching {
109112
self.underlying.getContainer(
110113
for: identifier,
111-
skipUpdate: self.skipUpdate,
114+
updateStrategy: self.skipUpdate ? .never : .always, // TODO: make this more elaborate
112115
observabilityScope: self.observabilityScope.makeChildScope(description: "prefetching package container", metadata: identifier.diagnosticsMetadata),
113116
on: .sharedConcurrent
114117
) { result in

‎Sources/SPMTestSupport/MockPackageContainer.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ public struct MockPackageContainerProvider: PackageContainerProvider {
165165

166166
public func getContainer(
167167
for package: PackageReference,
168-
skipUpdate: Bool,
168+
updateStrategy: ContainerUpdateStrategy,
169169
observabilityScope: ObservabilityScope,
170170
on queue: DispatchQueue,
171171
completion: @escaping (Result<PackageContainer, Swift.Error>

‎Sources/SPMTestSupport/MockWorkspace.swift

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,9 +139,10 @@ public final class MockWorkspace {
139139
let container = try temp_await {
140140
containerProvider.getContainer(
141141
for: packageRef,
142-
skipUpdate: true,
142+
updateStrategy: .never,
143143
observabilityScope: observability.topScope,
144-
on: .sharedConcurrent, completion: $0
144+
on: .sharedConcurrent,
145+
completion: $0
145146
)
146147
}
147148
guard let customContainer = container as? CustomPackageContainer else {

‎Sources/SourceControl/GitRepository.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -703,7 +703,8 @@ public final class GitRepository: Repository, WorkingCheckout {
703703
/// Returns true if a revision exists.
704704
public func exists(revision: Revision) -> Bool {
705705
self.lock.withLock {
706-
(try? callGit("rev-parse", "--verify", revision.identifier)) != nil
706+
let output = try? callGit("rev-parse", "--verify", "\(revision.identifier)^{commit}")
707+
return output != nil
707708
}
708709
}
709710

‎Sources/SourceControl/RepositoryManager.swift

Lines changed: 43 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -102,15 +102,15 @@ public class RepositoryManager: Cancellable {
102102
/// - Parameters:
103103
/// - package: The package identity of the repository to fetch,
104104
/// - repository: The repository to look up.
105-
/// - skipUpdate: If a repository is available, skip updating it.
105+
/// - updateStrategy: strategy to update the repository.
106106
/// - observabilityScope: The observability scope
107107
/// - delegateQueue: Dispatch queue for delegate events
108108
/// - callbackQueue: Dispatch queue for callbacks
109109
/// - completion: The completion block that should be called after lookup finishes.
110110
public func lookup(
111111
package: PackageIdentity,
112112
repository: RepositorySpecifier,
113-
skipUpdate: Bool,
113+
updateStrategy: RepositoryUpdateStrategy,
114114
observabilityScope: ObservabilityScope,
115115
delegateQueue: DispatchQueue,
116116
callbackQueue: DispatchQueue,
@@ -154,7 +154,7 @@ public class RepositoryManager: Cancellable {
154154
try self.lookup(
155155
package: package,
156156
repository: repository,
157-
skipUpdate: skipUpdate,
157+
updateStrategy: updateStrategy,
158158
observabilityScope: observabilityScope,
159159
delegateQueue: delegateQueue
160160
)
@@ -173,7 +173,7 @@ public class RepositoryManager: Cancellable {
173173
try self.lookup(
174174
package: package,
175175
repository: repository,
176-
skipUpdate: skipUpdate,
176+
updateStrategy: updateStrategy,
177177
observabilityScope: observabilityScope,
178178
delegateQueue: delegateQueue
179179
)
@@ -189,7 +189,7 @@ public class RepositoryManager: Cancellable {
189189
private func lookup(
190190
package: PackageIdentity,
191191
repository: RepositorySpecifier,
192-
skipUpdate: Bool,
192+
updateStrategy: RepositoryUpdateStrategy,
193193
observabilityScope: ObservabilityScope,
194194
delegateQueue: DispatchQueue
195195
) throws -> RepositoryHandle {
@@ -201,21 +201,20 @@ public class RepositoryManager: Cancellable {
201201
// errors when trying to check if a repository already exists are legitimate
202202
// and recoverable, and as such can be ignored
203203
if (try? self.provider.repositoryExists(at: repositoryPath)) ?? false {
204-
// update if necessary and return early
205-
// skip update if not needed
206-
if skipUpdate {
207-
return handle
208-
}
209-
// Update the repository when it is being looked up.
210-
delegateQueue.async {
211-
self.delegate?.willUpdate(package: package, repository: handle.repository)
212-
}
213-
let start = DispatchTime.now()
214204
let repository = try handle.open()
215-
try repository.fetch()
216-
let duration = start.distance(to: .now())
217-
delegateQueue.async {
218-
self.delegate?.didUpdate(package: package, repository: handle.repository, duration: duration)
205+
// Update the repository if needed
206+
if self.fetchRequired(repository: repository, updateStrategy: updateStrategy) {
207+
let start = DispatchTime.now()
208+
209+
delegateQueue.async {
210+
self.delegate?.willUpdate(package: package, repository: handle.repository)
211+
}
212+
213+
try repository.fetch()
214+
let duration = start.distance(to: .now())
215+
delegateQueue.async {
216+
self.delegate?.didUpdate(package: package, repository: handle.repository, duration: duration)
217+
}
219218
}
220219
return handle
221220
}
@@ -238,6 +237,7 @@ public class RepositoryManager: Cancellable {
238237
package: package,
239238
handle: handle,
240239
repositoryPath: repositoryPath,
240+
updateStrategy: updateStrategy,
241241
observabilityScope: observabilityScope,
242242
delegateQueue: delegateQueue
243243
)
@@ -279,6 +279,7 @@ public class RepositoryManager: Cancellable {
279279
package: PackageIdentity,
280280
handle: RepositoryHandle,
281281
repositoryPath: AbsolutePath,
282+
updateStrategy: RepositoryUpdateStrategy,
282283
observabilityScope: ObservabilityScope,
283284
delegateQueue: DispatchQueue
284285
) throws -> FetchDetails {
@@ -312,7 +313,9 @@ public class RepositoryManager: Cancellable {
312313
// Fetch the repository into the cache.
313314
if (self.fileSystem.exists(cachedRepositoryPath)) {
314315
let repo = try self.provider.open(repository: handle.repository, at: cachedRepositoryPath)
315-
try repo.fetch(progress: updateFetchProgress(progress:))
316+
if self.fetchRequired(repository: repo, updateStrategy: updateStrategy) {
317+
try repo.fetch(progress: updateFetchProgress(progress:))
318+
}
316319
cacheUsed = true
317320
} else {
318321
try self.provider.fetch(repository: handle.repository, to: cachedRepositoryPath, progressHandler: updateFetchProgress(progress:))
@@ -362,6 +365,20 @@ public class RepositoryManager: Cancellable {
362365
return FetchDetails(fromCache: cacheUsed, updatedCache: cacheUpdated)
363366
}
364367

368+
private func fetchRequired(
369+
repository: Repository,
370+
updateStrategy: RepositoryUpdateStrategy
371+
) -> Bool {
372+
switch updateStrategy {
373+
case .never:
374+
return false
375+
case .always:
376+
return true
377+
case .ifNeeded(let revision):
378+
return !repository.exists(revision: revision)
379+
}
380+
}
381+
365382
public func openWorkingCopy(at path: AbsolutePath) throws -> WorkingCheckout {
366383
try self.provider.openWorkingCopy(at: path)
367384
}
@@ -510,6 +527,12 @@ extension RepositoryManager {
510527
}
511528
}
512529

530+
public enum RepositoryUpdateStrategy {
531+
case never
532+
case always
533+
case ifNeeded(revision: Revision)
534+
}
535+
513536
/// Delegate to notify clients about actions being performed by RepositoryManager.
514537
public protocol RepositoryManagerDelegate {
515538
/// Called when a repository is about to be fetched.

‎Sources/Workspace/ResolverPrecomputationProvider.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ struct ResolverPrecomputationProvider: PackageContainerProvider {
5656

5757
func getContainer(
5858
for package: PackageReference,
59-
skipUpdate: Bool,
59+
updateStrategy: ContainerUpdateStrategy,
6060
observabilityScope: ObservabilityScope,
6161
on queue: DispatchQueue,
6262
completion: @escaping (Result<PackageContainer, Error>) -> Void

‎Sources/Workspace/Workspace.swift

Lines changed: 68 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1456,7 +1456,7 @@ extension Workspace {
14561456
repositoryManager.lookup(
14571457
package: dependency.packageRef.identity,
14581458
repository: repository,
1459-
skipUpdate: true,
1459+
updateStrategy: .never,
14601460
observabilityScope: observabilityScope,
14611461
delegateQueue: .sharedConcurrent,
14621462
callbackQueue: .sharedConcurrent,
@@ -2210,7 +2210,15 @@ extension Workspace {
22102210
observabilityScope.emit(.registryDependencyMissing(packageName: dependency.packageRef.identity.description))
22112211

22122212
case .custom(let version, let path):
2213-
let container = try temp_await { packageContainerProvider.getContainer(for: dependency.packageRef, skipUpdate: true, observabilityScope: observabilityScope, on: .sharedConcurrent, completion: $0) }
2213+
let container = try temp_await {
2214+
self.packageContainerProvider.getContainer(
2215+
for: dependency.packageRef,
2216+
updateStrategy: .never,
2217+
observabilityScope: observabilityScope,
2218+
on: .sharedConcurrent,
2219+
completion: $0
2220+
)
2221+
}
22142222
if let customContainer = container as? CustomPackageContainer {
22152223
let newPath = try customContainer.retrieve(at: version, observabilityScope: observabilityScope)
22162224
observabilityScope.emit(.customDependencyMissing(packageName: dependency.packageRef.identity.description))
@@ -2419,9 +2427,31 @@ extension Workspace {
24192427
for pin in pinsStore.pins.values {
24202428
group.enter()
24212429
let observabilityScope = observabilityScope.makeChildScope(description: "requesting package containers", metadata: pin.packageRef.diagnosticsMetadata)
2422-
packageContainerProvider.getContainer(for: pin.packageRef, skipUpdate: self.configuration.skipDependenciesUpdates, observabilityScope: observabilityScope, on: .sharedConcurrent, completion: { _ in
2423-
group.leave()
2424-
})
2430+
2431+
let updateStrategy: ContainerUpdateStrategy = {
2432+
if self.configuration.skipDependenciesUpdates {
2433+
return .never
2434+
} else {
2435+
switch pin.state {
2436+
case .branch:
2437+
return .always
2438+
case .revision(let revision):
2439+
return .ifNeeded(revision: revision)
2440+
case .version(_, let .some(revision)):
2441+
return .ifNeeded(revision: revision)
2442+
case .version(let version, .none):
2443+
return .always
2444+
}
2445+
}
2446+
}()
2447+
2448+
self.packageContainerProvider.getContainer(
2449+
for: pin.packageRef,
2450+
updateStrategy: updateStrategy,
2451+
observabilityScope: observabilityScope,
2452+
on: .sharedConcurrent,
2453+
completion: { _ in group.leave() }
2454+
)
24252455
}
24262456
group.wait()
24272457

@@ -2691,7 +2721,7 @@ extension Workspace {
26912721
let container = try temp_await {
26922722
packageContainerProvider.getContainer(
26932723
for: package,
2694-
skipUpdate: true,
2724+
updateStrategy: .never,
26952725
observabilityScope: observabilityScope,
26962726
on: .sharedConcurrent,
26972727
completion: $0
@@ -2959,7 +2989,13 @@ extension Workspace {
29592989
// Get the latest revision from the container.
29602990
// TODO: replace with async/await when available
29612991
guard let container = (try temp_await {
2962-
packageContainerProvider.getContainer(for: packageRef, skipUpdate: true, observabilityScope: observabilityScope, on: .sharedConcurrent, completion: $0)
2992+
packageContainerProvider.getContainer(
2993+
for: packageRef,
2994+
updateStrategy: .never,
2995+
observabilityScope: observabilityScope,
2996+
on: .sharedConcurrent,
2997+
completion: $0
2998+
)
29632999
}) as? SourceControlPackageContainer else {
29643000
throw InternalError("invalid container for \(packageRef) expected a SourceControlPackageContainer")
29653001
}
@@ -3095,7 +3131,7 @@ public final class LoadableResult<Value> {
30953131
extension Workspace: PackageContainerProvider {
30963132
public func getContainer(
30973133
for package: PackageReference,
3098-
skipUpdate: Bool,
3134+
updateStrategy: ContainerUpdateStrategy,
30993135
observabilityScope: ObservabilityScope,
31003136
on queue: DispatchQueue,
31013137
completion: @escaping (Result<PackageContainer, Swift.Error>) -> Void
@@ -3121,7 +3157,7 @@ extension Workspace: PackageContainerProvider {
31213157
self.repositoryManager.lookup(
31223158
package: package.identity,
31233159
repository: repositorySpecifier,
3124-
skipUpdate: skipUpdate,
3160+
updateStrategy: updateStrategy.repositoryUpdateStrategy,
31253161
observabilityScope: observabilityScope,
31263162
delegateQueue: queue,
31273163
callbackQueue: queue
@@ -3374,7 +3410,7 @@ extension Workspace {
33743410
self.repositoryManager.lookup(
33753411
package: package.identity,
33763412
repository: repository,
3377-
skipUpdate: true,
3413+
updateStrategy: .never,
33783414
observabilityScope: observabilityScope,
33793415
delegateQueue: .sharedConcurrent,
33803416
callbackQueue: .sharedConcurrent,
@@ -3765,7 +3801,15 @@ extension Workspace {
37653801
case .fileSystem:
37663802
return nil
37673803
case .custom:
3768-
let container = try temp_await { packageContainerProvider.getContainer(for: package, skipUpdate: true, observabilityScope: observabilityScope, on: .sharedConcurrent, completion: $0) }
3804+
let container = try temp_await {
3805+
self.packageContainerProvider.getContainer(
3806+
for: package,
3807+
updateStrategy: .never,
3808+
observabilityScope: observabilityScope,
3809+
on: .sharedConcurrent,
3810+
completion: $0
3811+
)
3812+
}
37693813
guard let customContainer = container as? CustomPackageContainer else {
37703814
observabilityScope.emit(error: "invalid custom dependency container: \(container)")
37713815
return nil
@@ -4206,3 +4250,16 @@ fileprivate func warnToStderr(_ message: String) {
42064250

42074251
// used for manifest validation
42084252
extension RepositoryManager: ManifestSourceControlValidator {}
4253+
4254+
extension ContainerUpdateStrategy {
4255+
var repositoryUpdateStrategy: RepositoryUpdateStrategy {
4256+
switch self {
4257+
case .always:
4258+
return .always
4259+
case .never:
4260+
return .never
4261+
case .ifNeeded(let revision):
4262+
return .ifNeeded(revision: .init(identifier: revision))
4263+
}
4264+
}
4265+
}

0 commit comments

Comments
 (0)