Add ImagePrefetcher (and swift-collection) and integrate in Reader #23928
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| import UIKit | ||
| import Collections | ||
|
|
||
| @ImageDownloaderActor | ||
| public final class ImagePrefetcher { | ||
| private let downloader: ImageDownloader | ||
| private let maxConcurrentTasks: Int | ||
| private var queue = OrderedDictionary<PrefetchKey, PrefetchTask>() | ||
|
There was a problem hiding this comment. I feel like There was a problem hiding this comment.
This part we don't really need because the entire systems already runs in the background. If you need an async There was a problem hiding this comment. For me personally, I probably would choose There was a problem hiding this comment. We should find a good open-source implementation that's sound and covered with tests. If you have something in mind, please let me know. I also recently took a stab at it https://github.com/kean/Nuke/blob/task-queue/Sources/Nuke/Internal/WorkQueue.swift, but it's not finished yet, and I'm not sure if there is a way to make it general-purpose by allowing you to customize it with an actor. |
||
| private var numberOfActiveTasks = 0 | ||
|
|
||
| deinit { | ||
| let tasks = queue.values.compactMap(\.task) | ||
| for task in tasks { | ||
| task.cancel() | ||
| } | ||
| } | ||
|
|
||
| public nonisolated init(downloader: ImageDownloader, maxConcurrentTasks: Int = 2) { | ||
| self.downloader = downloader | ||
| self.maxConcurrentTasks = maxConcurrentTasks | ||
| } | ||
|
|
||
| public nonisolated func startPrefetching(for requests: [ImageRequest]) { | ||
| Task { @ImageDownloaderActor in | ||
| for request in requests { | ||
| startPrefetching(for: request) | ||
| } | ||
| performPendingTasks() | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. This function looks a bit weird to me. Although the function signature says it's non-isolated, semantically, the implementation is isolated. IMHO, it makes more sense to remove There was a problem hiding this comment. Yeah, this doesn't feel quite right, but in this case, you will have to write There was a problem hiding this comment. Would you need the There was a problem hiding this comment. The rules for isolation inheritance are a bit tricky. In this case, since that method that creates a nonisolated func startPrefetching(...) {
Task {
// The closure is **not** isolated because the containing method is
// ``nonisolated``. It will require an `await` (async invocation).
// await startPrefetching(for: request)
}
}
nonisolated func startPrefetching(...) {
Task { @ImageDownloaderActor in
// OK since the closure is explicitly isolated to `@ImageDownloaderActor`
startPrefetching(for: request)
}
}
func startPrefetching(...) {
Task {
// The closure is isolated to the containing isolation domain (of the containing type)
startPrefetching(for: request)
}
} |
||
|
|
||
| private func startPrefetching(for request: ImageRequest) { | ||
| let key = PrefetchKey(request: request) | ||
| guard queue[key] == nil else { | ||
| return | ||
| } | ||
| queue[key] = PrefetchTask() | ||
| } | ||
|
|
||
| private func performPendingTasks() { | ||
| var index = 0 | ||
| func nextPendingTask() -> (PrefetchKey, PrefetchTask)? { | ||
| while index < queue.count { | ||
| if queue.elements[index].value.task == nil { | ||
| return queue.elements[index] | ||
| } | ||
| index += 1 | ||
| } | ||
| return nil | ||
| } | ||
| while numberOfActiveTasks < maxConcurrentTasks, let (key, task) = nextPendingTask() { | ||
| task.task = Task { | ||
| await self.actuallyPrefetchImage(for: key.request) | ||
| } | ||
| numberOfActiveTasks += 1 | ||
| } | ||
| } | ||
|
|
||
| private func actuallyPrefetchImage(for request: ImageRequest) async { | ||
| _ = try? await downloader.image(for: request) | ||
|
|
||
| numberOfActiveTasks -= 1 | ||
| queue[PrefetchKey(request: request)] = nil | ||
| performPendingTasks() | ||
| } | ||
|
|
||
| public nonisolated func stopPrefetching(for requests: [ImageRequest]) { | ||
| Task { @ImageDownloaderActor in | ||
| for request in requests { | ||
| stopPrefetching(for: request) | ||
| } | ||
| performPendingTasks() | ||
| } | ||
| } | ||
|
|
||
| private func stopPrefetching(for request: ImageRequest) { | ||
| let key = PrefetchKey(request: request) | ||
| if let task = queue.removeValue(forKey: key) { | ||
| task.task?.cancel() | ||
| } | ||
| } | ||
|
|
||
| public nonisolated func stopAll() { | ||
| Task { @ImageDownloaderActor in | ||
| for (_, value) in queue { | ||
| value.task?.cancel() | ||
| } | ||
| queue.removeAll() | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Should There was a problem hiding this comment. It's probably worth writing a few unit tests for this class. There was a problem hiding this comment. Fixed. Yeah, it needs some tests 😩 |
||
|
|
||
| private struct PrefetchKey: Hashable, Sendable { | ||
| let request: ImageRequest | ||
|
|
||
| func hash(into hasher: inout Hasher) { | ||
| request.source.url?.hash(into: &hasher) | ||
| } | ||
|
|
||
| static func == (lhs: PrefetchKey, rhs: PrefetchKey) -> Bool { | ||
| let (lhs, rhs) = (lhs.request, rhs.request) | ||
| return (lhs.source.url, lhs.options) == (rhs.source.url, rhs.options) | ||
| } | ||
| } | ||
|
|
||
| private final class PrefetchTask: @unchecked Sendable { | ||
| var task: Task<Void, Error>? | ||
| } | ||
| } | ||
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.
Any particular reason of using a global actor, instead of making
ImagePrefetcheran actor?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.
It's much easier to reason about code when every part of the system – image downloader in that case – is synchronized on the same actor. The main reason is that you can avoid a lot of
asynccalls because when you are on an actor (@ImageDownloaderActor), you don't needasyncto invoke methods on other classes synchronized on the same actor.It's also beneficial for performance as you can avoid unnecessary context switches. For example, when
ImagePrefetchercalls_ = try? await downloader.image(for: request), there is no context switch as the code already runs on@ImageDownloaderActor.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.
I'm not sure if there are any situation where a new actor would be preferable to a global actor or another form of synchronization.
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.
Hmmm, I'm kind of on the opposite side of this ⬆️ . I would always define
actortypes, rather than define global actors. I see global actors as a way to synchronize multiple types, which I rarely need.Ah, I missed that
ImageDownloaderActoris used by other types. Now it makes sense to me whyImageDownloaderActorglobal actor is used here.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.
I agree, I think creating a new
actoris a good starting point if there is no synchronization needed across different types/methods.