Add ImagePrefetcher (and swift-collection) and integrate in Reader by kean · Pull Request #23928 · wordpress-mobile/WordPress-iOS

kean · 2024-12-30T19:01:18Z

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

To test:

Regression Notes

Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
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:

dangermattic · 2024-12-30T19:02:21Z

	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 · 2024-12-30T19:12:55Z

📲 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 · 2024-12-30T19:16:03Z

📲 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 · 2025-01-13T01:54:26Z

Modules/Sources/WordPressMedia/ImagePrefetcher.swift

+import Collections
+
+@ImageDownloaderActor
+public final class ImagePrefetcher {


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

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.

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.

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.

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

crazytonyli · 2025-01-13T01:58:03Z

Modules/Sources/WordPressMedia/ImagePrefetcher.swift

+            }
+            performPendingTasks()
+        }
+    }


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.

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.

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.

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 · 2025-01-13T02:07:48Z

Modules/Sources/WordPressMedia/ImagePrefetcher.swift

+public final class ImagePrefetcher {
+    private let downloader: ImageDownloader
+    private let maxConcurrentTasks: Int
+    private var queue = OrderedDictionary<PrefetchKey, PrefetchTask>()


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

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.

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". 😆

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 · 2025-01-14T00:54:00Z

Modules/Sources/WordPressMedia/ImagePrefetcher.swift

+            }
+            queue.removeAll()
+        }
+    }


Should maxConcurrentTasks value be updated in stopPrefetching and stopAll?

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

Fixed. Yeah, it needs some tests 😩

Add ImagePrefetcher

a4a876a

kean added the Reader label Dec 30, 2024

kean added this to the 25.7 milestone Dec 30, 2024

Update releaes notes

65f91b0

kean merged commit 21a133e into christmas-feature-branch Dec 30, 2024

kean deleted the task/image-prefetcher branch December 30, 2024 19:11

kean mentioned this pull request Dec 30, 2024

Feature Branch 25.7 #23923

Merged

14 tasks

kean changed the title ~~Add ImagePrefetcher~~ Add ImagePrefetcher (and swift-collection)and integrate in Reader Dec 30, 2024

kean changed the title ~~Add ImagePrefetcher (and swift-collection)and integrate in Reader~~ Add ImagePrefetcher (and swift-collection) and integrate in Reader Dec 30, 2024

crazytonyli reviewed Jan 13, 2025

View reviewed changes

crazytonyli reviewed Jan 14, 2025

View reviewed changes

Conversation

kean commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Regression Notes

Uh oh!

dangermattic commented Dec 30, 2024

Uh oh!

wpmobilebot commented Dec 30, 2024

Uh oh!

wpmobilebot commented Dec 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kean Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kean commented Dec 30, 2024 •

edited

Loading

kean Jan 13, 2025 •

edited

Loading