Add ImagePrefetcher (and swift-collection) and integrate in Reader

[kean]

Fixes #4288 by adding prefetching to the Reader stream. This PR also adds swift-collection as a dependency.

To test:

Regression Notes

1. Potential unintended areas of impact

2. What I did to test those areas of impact (or what existing automated tests I relied on)

3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

• WordPress.com sites and self-hosted Jetpack sites.
• Portrait and landscape orientations.
• Light and dark modes.
• Fonts: Larger, smaller and bold text.
• High contrast.
• VoiceOver.
• Languages with large words or with letters/accents not frequently used in English.
• Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
• iPhone and iPad.
• Multi-tasking: Split view and Slide over. (iPad)

[dangermattic]

	1 Warning
⚠️	Modules/Package.swift was changed without updating its corresponding Package.resolved. Please resolve the Swift packages as appropriate to your project setup (e.g. in Xcode or by running swift package resolve).

Generated by 🚫 Danger

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr23928-65f91b0
	Version	25.6
	Bundle ID	org.wordpress.alpha
	Commit	65f91b0
	App Center Build	WPiOS - One-Offs #11234

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr23928-65f91b0
	Version	25.6
	Bundle ID	com.jetpack.alpha
	Commit	65f91b0
	App Center Build	jetpack-installable-builds #10272

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[crazytonyli]

Any particular reason of using a global actor, instead of making ImagePrefetcher an actor?

[kean]

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 async calls because when you are on an actor (@ImageDownloaderActor), you don't need async to 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 ImagePrefetcher calls _ = try? await downloader.image(for: request), there is no context switch as the code already runs on @ImageDownloaderActor.

[kean]

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.

[crazytonyli]

Hmmm, I'm kind of on the opposite side of this ⬆️ . I would always define actor types, rather than define global actors. I see global actors as a way to synchronize multiple types, which I rarely need.

It's also beneficial for performance as you can avoid unnecessary context switches. For example, when ImagePrefetcher calls _ = try? await downloader.image(for: request), there is no context switch as the code already runs on @ImageDownloaderActor.

Ah, I missed that ImageDownloaderActor is used by other types. Now it makes sense to me why ImageDownloaderActor global actor is used here.

[kean]

I agree, I think creating a new actor is a good starting point if there is no synchronization needed across different types/methods.

[crazytonyli]

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 nonisolated and make callers create Task { await prefetcher.startPrefectching(...) } instead.

[kean]

Yeah, this doesn't feel quite right, but in this case, you will have to write Task { @ImageDownloaderActor in in 100% of the places where you use it, so I opted to make it nonisolated and callable from any isolation domain, including the main actor. It's easier and cleaner from the caller point of view.

[crazytonyli]

Would you need the @ImageDownloaderActor part? I thought the func in Task { await actor.func() } would just run in the actor, regardless actor is an actor type or bound to an global actor.

[kean]

The rules for isolation inheritance are a bit tricky. In this case, since that method that creates a Task is nonisolated, so is the task.

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) 
    }
}

[crazytonyli]

I feel like OperationQueue might be a better suit here. It manages two key things here: concurrency and queue.

[kean]

concurrency

This part we don't really need because the entire systems already runs in the background.

If you need an async Operation, I think these have outlived their usefulness in the Swift Concurrency world. There needs to be a new primitive in the standard library. But it's relatively easy to limit a maximum number of tasks even without one.

[crazytonyli]

For me personally, I probably would choose OperationQueue over implementing task orchestration (which is the core implementation here) myself, even though it does not feel like a "modern tech". 😆

[kean]

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.

[crazytonyli]

Should maxConcurrentTasks value be updated in stopPrefetching and stopAll?

[crazytonyli]

It's probably worth writing a few unit tests for this class.

[kean]

Fixed. Yeah, it needs some tests 😩