Skip to content

Commit d30183c

Browse files
tomerdtomerd
authored
performance improvements (#3098)
motivation: improve performance of dependency resolution changes: * pubgrub manages a concurrent queue that is passed to the container providers * start prefetching dependencies as soon as we discover them * refractor pubgrub to do work in parallel where possible (e.g. version counts and bound calculation) * serialize writing to output stream in SwifTool since now delegate callbacks can be concurrent * improve computeBounds to pre-compute some bounds in parallel * fix RepositoryManagerTests to address delayed callbacks * make relevant tests thread-safe
1 parent dafe0e6 commit d30183c

22 files changed

+967
-587
lines changed

‎Sources/Basics/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ add_library(Basics
1010
ConcurrencyHelpers.swift
1111
Dictionary+Extensions.swift
1212
DispatchTimeInterval+Extensions.swift
13+
Errors.swift
1314
FileSystem+Extensions.swift
1415
HTPClient+URLSession.swift
1516
HTTPClient.swift

‎Sources/Basics/ConcurrencyHelpers.swift

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import TSCBasic
1010

1111
/// Thread-safe dictionary like structure
12-
public struct ThreadSafeKeyValueStore<Key, Value> where Key: Hashable {
12+
public final class ThreadSafeKeyValueStore<Key, Value> where Key: Hashable {
1313
private var underlying: [Key: Value]
1414
private let lock = Lock()
1515

@@ -30,26 +30,40 @@ public struct ThreadSafeKeyValueStore<Key, Value> where Key: Hashable {
3030
}
3131

3232
@discardableResult
33-
public mutating func memoize(_ key: Key, body: () throws -> Value) rethrows -> Value {
34-
try self.underlying.memoize(key: key, lock: self.lock, body: body)
33+
public func memoize(_ key: Key, body: () throws -> Value) rethrows -> Value {
34+
try self.lock.withLock {
35+
try self.underlying.memoize(key: key, body: body)
36+
}
3537
}
3638

37-
public mutating func clear() {
39+
public func clear() {
3840
self.lock.withLock {
3941
self.underlying.removeAll()
4042
}
4143
}
44+
45+
public var isEmpty: Bool {
46+
self.lock.withLock {
47+
self.underlying.isEmpty
48+
}
49+
}
50+
51+
public func contains(_ key: Key) -> Bool {
52+
self.lock.withLock {
53+
self.underlying.keys.contains(key)
54+
}
55+
}
4256
}
4357

4458
/// Thread-safe value boxing structure
45-
public struct ThreadSafeBox<Value> {
59+
public final class ThreadSafeBox<Value> {
4660
private var underlying: Value?
4761
private let lock = Lock()
4862

4963
public init() {}
5064

5165
@discardableResult
52-
public mutating func memoize(body: () throws -> Value) rethrows -> Value {
66+
public func memoize(body: () throws -> Value) rethrows -> Value {
5367
if let value = self.get() {
5468
return value
5569
}
@@ -60,7 +74,7 @@ public struct ThreadSafeBox<Value> {
6074
return value
6175
}
6276

63-
public mutating func clear() {
77+
public func clear() {
6478
self.lock.withLock {
6579
self.underlying = nil
6680
}
@@ -72,3 +86,10 @@ public struct ThreadSafeBox<Value> {
7286
}
7387
}
7488
}
89+
90+
// FIXME: mark as deprecated once async/await is available
91+
//@available(*, deprecated, message: "replace with async/await when available")
92+
@inlinable
93+
public func temp_await<T, ErrorType>(_ body: (@escaping (Result<T, ErrorType>) -> Void) -> Void) throws -> T {
94+
return try tsc_await(body)
95+
}

‎Sources/Basics/Dictionary+Extensions.swift

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,12 @@ import TSCBasic
1111
extension Dictionary {
1212
@inlinable
1313
@discardableResult
14-
public mutating func memoize(key: Key, lock: Lock, body: () throws -> Value) rethrows -> Value {
15-
if let value = (lock.withLock { self[key] }) {
14+
public mutating func memoize(key: Key, body: () throws -> Value) rethrows -> Value {
15+
if let value = self[key] {
1616
return value
1717
}
1818
let value = try body()
19-
lock.withLock {
20-
self[key] = value
21-
}
19+
self[key] = value
2220
return value
2321
}
2422
}

‎Sources/Basics/Errors.swift

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
/*
2+
This source file is part of the Swift.org open source project
3+
4+
Copyright (c) 2020 Apple Inc. and the Swift project authors
5+
Licensed under Apache License v2.0 with Runtime Library Exception
6+
7+
See http://swift.org/LICENSE.txt for license information
8+
See http://swift.org/CONTRIBUTORS.txt for Swift project authors
9+
*/
10+
11+
public struct InternalError: Error {
12+
private let description: String
13+
public init(_ description: String) {
14+
self.description = description
15+
}
16+
}

‎Sources/Commands/SwiftTool.swift

Lines changed: 84 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -61,112 +61,128 @@ private class ToolWorkspaceDelegate: WorkspaceDelegate {
6161
}
6262

6363
func fetchingWillBegin(repository: String, fetchDetails: RepositoryManager.FetchDetails?) {
64-
stdoutStream <<< "Fetching \(repository)"
65-
if let fetchDetails = fetchDetails {
66-
if fetchDetails.fromCache {
67-
stdoutStream <<< " from cache"
64+
queue.async {
65+
self.stdoutStream <<< "Fetching \(repository)"
66+
if let fetchDetails = fetchDetails {
67+
if fetchDetails.fromCache {
68+
self.stdoutStream <<< " from cache"
69+
}
6870
}
71+
self.stdoutStream <<< "\n"
72+
self.stdoutStream.flush()
6973
}
70-
stdoutStream <<< "\n"
71-
stdoutStream.flush()
7274
}
7375

7476
func fetchingDidFinish(repository: String, fetchDetails: RepositoryManager.FetchDetails?, diagnostic: Diagnostic?) {
7577
}
7678

7779
func repositoryWillUpdate(_ repository: String) {
78-
stdoutStream <<< "Updating \(repository)"
79-
stdoutStream <<< "\n"
80-
stdoutStream.flush()
80+
queue.async {
81+
self.stdoutStream <<< "Updating \(repository)"
82+
self.stdoutStream <<< "\n"
83+
self.stdoutStream.flush()
84+
}
8185
}
8286

8387
func repositoryDidUpdate(_ repository: String) {
8488
}
8589

8690
func dependenciesUpToDate() {
87-
stdoutStream <<< "Everything is already up-to-date"
88-
stdoutStream <<< "\n"
89-
stdoutStream.flush()
91+
queue.async {
92+
self.stdoutStream <<< "Everything is already up-to-date"
93+
self.stdoutStream <<< "\n"
94+
self.stdoutStream.flush()
95+
}
9096
}
9197

9298
func cloning(repository: String) {
93-
stdoutStream <<< "Cloning \(repository)"
94-
stdoutStream <<< "\n"
95-
stdoutStream.flush()
99+
queue.async {
100+
self.stdoutStream <<< "Cloning \(repository)"
101+
self.stdoutStream <<< "\n"
102+
self.stdoutStream.flush()
103+
}
96104
}
97105

98106
func checkingOut(repository: String, atReference reference: String, to path: AbsolutePath) {
99-
stdoutStream <<< "Resolving \(repository) at \(reference)"
100-
stdoutStream <<< "\n"
101-
stdoutStream.flush()
107+
queue.async {
108+
self.stdoutStream <<< "Resolving \(repository) at \(reference)"
109+
self.stdoutStream <<< "\n"
110+
self.stdoutStream.flush()
111+
}
102112
}
103113

104114
func removing(repository: String) {
105-
stdoutStream <<< "Removing \(repository)"
106-
stdoutStream <<< "\n"
107-
stdoutStream.flush()
115+
queue.async {
116+
self.stdoutStream <<< "Removing \(repository)"
117+
self.stdoutStream <<< "\n"
118+
self.stdoutStream.flush()
119+
}
108120
}
109121

110122
func warning(message: String) {
111123
// FIXME: We should emit warnings through the diagnostic engine.
112-
stdoutStream <<< "warning: " <<< message
113-
stdoutStream <<< "\n"
114-
stdoutStream.flush()
124+
queue.async {
125+
self.stdoutStream <<< "warning: " <<< message
126+
self.stdoutStream <<< "\n"
127+
self.stdoutStream.flush()
128+
}
115129
}
116130

117131
func willResolveDependencies(reason: WorkspaceResolveReason) {
118132
guard isVerbose else { return }
119133

120-
stdoutStream <<< "Running resolver because "
121-
122-
switch reason {
123-
case .forced:
124-
stdoutStream <<< "it was forced"
125-
case .newPackages(let packages):
126-
let dependencies = packages.lazy.map({ "'\($0.path)'" }).joined(separator: ", ")
127-
stdoutStream <<< "the following dependencies were added: \(dependencies)"
128-
case .packageRequirementChange(let package, let state, let requirement):
129-
stdoutStream <<< "dependency '\(package.name)' was "
130-
131-
switch state {
132-
case .checkout(let checkoutState)?:
133-
switch checkoutState.requirement {
134-
case .versionSet(.exact(let version)):
135-
stdoutStream <<< "resolved to '\(version)'"
136-
case .versionSet(_):
137-
// Impossible
138-
break
139-
case .revision(let revision):
140-
stdoutStream <<< "resolved to '\(revision)'"
141-
case .unversioned:
142-
stdoutStream <<< "unversioned"
134+
queue.sync {
135+
self.stdoutStream <<< "Running resolver because "
136+
137+
switch reason {
138+
case .forced:
139+
self.stdoutStream <<< "it was forced"
140+
case .newPackages(let packages):
141+
let dependencies = packages.lazy.map({ "'\($0.path)'" }).joined(separator: ", ")
142+
self.stdoutStream <<< "the following dependencies were added: \(dependencies)"
143+
case .packageRequirementChange(let package, let state, let requirement):
144+
self.stdoutStream <<< "dependency '\(package.name)' was "
145+
146+
switch state {
147+
case .checkout(let checkoutState)?:
148+
switch checkoutState.requirement {
149+
case .versionSet(.exact(let version)):
150+
self.stdoutStream <<< "resolved to '\(version)'"
151+
case .versionSet(_):
152+
// Impossible
153+
break
154+
case .revision(let revision):
155+
self.stdoutStream <<< "resolved to '\(revision)'"
156+
case .unversioned:
157+
self.stdoutStream <<< "unversioned"
158+
}
159+
case .edited?:
160+
self.stdoutStream <<< "edited"
161+
case .local?:
162+
self.stdoutStream <<< "versioned"
163+
case nil:
164+
self.stdoutStream <<< "root"
143165
}
144-
case .edited?:
145-
stdoutStream <<< "edited"
146-
case .local?:
147-
stdoutStream <<< "versioned"
148-
case nil:
149-
stdoutStream <<< "root"
150-
}
151166

152-
stdoutStream <<< " but now has a "
167+
self.stdoutStream <<< " but now has a "
153168

154-
switch requirement {
155-
case .versionSet:
156-
stdoutStream <<< "different version-based"
157-
case .revision:
158-
stdoutStream <<< "different revision-based"
159-
case .unversioned:
160-
stdoutStream <<< "unversioned"
169+
switch requirement {
170+
case .versionSet:
171+
self.stdoutStream <<< "different version-based"
172+
case .revision:
173+
self.stdoutStream <<< "different revision-based"
174+
case .unversioned:
175+
self.stdoutStream <<< "unversioned"
176+
}
177+
178+
self.stdoutStream <<< " requirement."
179+
default:
180+
self.stdoutStream <<< " requirements have changed."
161181
}
162182

163-
stdoutStream <<< " requirement."
164-
default:
165-
stdoutStream <<< " requirements have changed."
183+
self.stdoutStream <<< "\n"
184+
stdoutStream.flush()
166185
}
167-
168-
stdoutStream <<< "\n"
169-
stdoutStream.flush()
170186
}
171187

172188
func downloadingBinaryArtifact(from url: String, bytesDownloaded: Int64, totalBytesToDownload: Int64?) {

‎Sources/PackageGraph/PackageContainer.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
See http://swift.org/CONTRIBUTORS.txt for Swift project authors
99
*/
1010

11+
import Dispatch
1112
import PackageLoading
1213
import PackageModel
1314
import SourceControl
@@ -132,6 +133,7 @@ public protocol PackageContainerProvider {
132133
func getContainer(
133134
for identifier: PackageReference,
134135
skipUpdate: Bool,
136+
on queue: DispatchQueue,
135137
completion: @escaping (Result<PackageContainer, Swift.Error>) -> Void
136138
)
137139
}

0 commit comments

Comments
 (0)