use core background fetch, use .default qos #2071
Conversation
.background is probably not what we want - this qos may have long delays, apple is a bit opaque in its description, but stackoverflow is pretty concrete (https://stackoverflow.com/questions/53141829/performance-of-background-queue-in-swift) and we even have a comment in our code ("may be delayed by tens of minutes"). this also fixes the warning "Thread running at User-initiated quality-of-service class waiting on a lower QoS thread" as before the background was .default while the event handler was .backgound potentially, this change could explain several issues :)
deltachat-ios/AppDelegate.swift
Outdated
|
|
||
| // wait for DC_EVENT_ACCOUNTS_BACKGROUND_FETCH_DONE, | ||
| // marking the end of events needed to being processed. | ||
| // as IO was not started, random events caused by other reasons are not added. |
There was a problem hiding this comment.
maybe also mention that this event is also emitted in case of timeout, so it is clearer that we can not just do an early exit when the fetch timed-out (because the event is still emitted and we still might have a partial fetch that has some of the new message events)
There was a problem hiding this comment.
i made the comment more precise, putting it more in code context and not just repeating documentation
main purpose of the comment is anyway to know that you need to grep for DC_EVENT_ACCOUNTS_BACKGROUND_FETCH_DONE :)
| @@ -484,35 +484,33 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD | |||
| pushToDebugArray("1") | |||
|
|
|||
| // move work to non-main thread to not block UI (otherwise, in case we get suspended, the app is blocked totally) | |||
| // (we are using `qos: default` as `qos: .background` or `main.asyncAfter` may be delayed by tens of minutes) | |||
| // (we are using `qos: default` as `qos: .background` may be delayed by tens of minutes) | |||
There was a problem hiding this comment.
From what I know, .background-work only gets done when the app is in background and stuff gets done with a very low priority. So it's not a background thread (as in "not main") while the app is running in foreground. So it's not only a question of time.
When we switch to iOS 13+, we could start using Task and async/await for background-stuff.
There was a problem hiding this comment.
I would also like async/await because it would make working with jsonrpc nicer.
Also iOS 13 brings swiftUI but that seems to be not mature enough to be used from what I heard.
But on the other hand we are a messenger and people appreciate that we support older devices as well at least on android, for iPhone users it might be more common to upgrade to a new phone regularly - I suggest we look at the usage percentage take that into account.
| // backgroundFetch() pauses IO as needed | ||
| if !self.dcAccounts.backgroundFetch(timeout: 20) { | ||
| logger.error("backgroundFetch failed") | ||
| self.pushToDebugArray("ERR2") |
There was a problem hiding this comment.
What does ERR2 stand for?
There was a problem hiding this comment.
it js just a random short string added to "Settings/Advaned/View Log". it makes sense only if you look at the code. once things are settled, we can remove this DebugArray completely
|
thanks for review and thoughts! i am also using the PR already since a day on my phone, and it seems to work in general :) |
this PR aims to improve the existing background fetch further by:
use the new
dc_accounts_background_fetch()api which was explicitly made for this purposechanging qos ("quality of service") of background task to
.defaultinstead of.background- while the latter sounds as if it is correct, it was probably never the intention as.backgroundmay be delayed by a loooong time according to stackoverflow - but also by our own observations (in theory, there is also.utility, however, finer granulation is probably not needed, i assume anyways that there is some additional per-app-weight)the "one second sleep" is untouched for now, in fact, you'll see some core activity between "finishing fetch" and "fetch done", at least in the simulator, maybe in "real background" where re-start IO is not needed, it is really superfluous. also, in theory, swift could have started short-living threads from events need to finish. so, let's do not have too many moving targets and leave that as is for now