-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[POC] Move Model mappings to Background #2168
Conversation
📏 Size AnalysisTotal install size 9.4 MB | This change: ⬆️ +26.6 kB (+0.283%)🗂 See size breakdown
🔎 See the full size analysis (b126fe1) merging into develop (8a01f51)
|
Generated by 🚫 Danger |
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.
LGTM overall 👍 Just added some minor comments
import CoreData | ||
import Foundation | ||
|
||
class ListDatabaseObserverWrapper<Item, DTO: NSManagedObject> { |
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.
Since it looks like both ListDatabaseObserver
's have the same API, instead of having the isBackground
spread all over this wrapper, can't we export the API to a protocol, and set the ListDatabaseObserver implementation depending on the isBackground
flag?
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 tried the protocol approach but because of the generics it was not possible to then maintain certain APIs
/// When called, release the notification observers | ||
private var releaseNotificationObservers: (() -> Void)? | ||
|
||
private let queue = DispatchQueue.global() |
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.
Better to create a queue from scratch instead of using the global one
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.
Yes, and might be good to specify qos
in this case.
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.
As we've discussed, we need to see what happens with the diffing solution, as well as the SwiftUI SDK (there are some glitches there).
Closed in benefit of #2237 with uses the new messages list |
🔗 Issue Links
https://stream-io.atlassian.net/browse/CIS-1854
🎯 Goal
The goal of this PR is to investigate the viability of moving the mapping of DTOs to Model to a background thread, reducing the pressure we put in the view/main thread.
📝 Summary
This PR introduces a new ListDatabaseObserver, matching the same API for simplicity purposes. This new object, named BackgroundListDatabaseObserver does exactly the same as its predecessor, but in a background and asynchronous way, and only notifies about results when those are already mapped.
🛠 Implementation
As of now:
_isLazyMappingEnabled
In order for background mapping to work, we cannot use the diffing mechanism on the messages list (for now)
There are several deficiencies that were found during this process and should be addressed before iterating this POC even further.
All this implementation is guarded now behind the following mechanisms:
_isBackgroundMappingEnabled
which enables the usage of BackgroundListDatabaseObserver_isLazyMappingEnabled
when enabling_isBackgroundMappingEnabled
When using DemoApp-StreamDevelopers, it uses Background Mapping by default
☑️ Contributor Checklist
🎁 Meme