refactor(mobile): IOS replace DispatchQueue + DispatchSemaphore with OperationQueue for image processing#27471
refactor(mobile): IOS replace DispatchQueue + DispatchSemaphore with OperationQueue for image processing#27471
Conversation
…ue for image processing
|
Just to mention it: I think all the .sync calls on other queues (eg. requests) also cause the thread to be blocked for a short time, if the sync has to wait because there is already someone doing something on the queue. |
|
Have you actually compared |
|
@mertalev Yes it behaves differently. If for example the maxConcurrentOperationCount is 4 and already 4 operations are running, and one operation blocks on eg. PHAsset.fetchAssets, the OperationQueue considers that operation as still running. In comparison our old code dumped everything into the concurrent queue and blocked threads with a semaphore, which forces GCD to spawn new threads (leading to thread explosion and more threads that wait). When I researched this, the Operation Queue came up as one of the mitigations to the thread explosion problem. For example here on stackoverflow Actually I wanted to ask, If you ever tried to rewrite this with swift concurrency? |
|
Removed the bugfix label as this probably doesn't fix the mentioned issue |
This isn't a documented behavior of
I've thought about this but |
|
previously didn't test it myself, but an assumption based on all the articles/discussions on thread explosion in swift. But in the meantime I did a quick test in the Xcode playground. And yes, there are really only maxConcurrentOperationCount of operations in maxConcurrentOperationCount threads active all the time, even if I block the threads with sleep(1). Just to verify I also tested the old approach (DispatchQueue + DispatchSemaphore) and there it used 64 concurrent thread. |
Description
The iOS image processing pipeline uses a concurrent DispatchQueue paired with a DispatchSemaphore to limit how many image operations run in parallel. This has small issue:
when a task blocks (e.g. on PHAsset.fetchAssets(WithLocalIdentifiers), which does a synchronous fetch) or for example is just blocked by the semaphore, GCD sees the thread as idle and spins up a new worker thread.
That new tasks are getting blocked by the semaphore, causing GCD to spawn yet another thread, and so on. In the crash logs from the linked issue, this is visible by the 50+ threads all stuck on semaphore_wait_trap on the thumbnail.processing queue.
Usually, the UI/Main thread should still be doing its work. But if the UI Thread now starts to wait on something, the whole UI freezes.
The log itself, was only created later, when @Magnus987 tried to close the app. In the logs we then see that iOS tried to move the app into the background but the app didn't respond so It got killed after 5 seconds.
I guess under normal circumstances this isn't really noticeable, because the threads usually finish fast enough, to not cause stalling.
The fix: Instead of using DispatchQueue + DispatchSemaphore, we use OperationQueue which limits the actual threads spawned to the amount we specify.
I am actually not sure if it fully fixes #27365 but now IOS doesn't have to kill the app anymore in such a case, and it could be that we missed some logs in the flutter because of that(?).
How Has This Been Tested?
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
To discuss iOS/swift. Not really my language.