Skip to content

Commit

Permalink
[CIS-1854] [Internal] Move Model mappings to Background (#2237)
Browse files Browse the repository at this point in the history
* Add cell heights cache so that new contentSize calculation is more precise

* Remove `_messageListDiffingEnabled` flag and diffable data source implementation

* WIP DifferenceKit

* Move DiffKit logic to Message List only

* Improve performance by using ChatMessage directly instead of mapping always to DiffChatMessage

* Improve performance of the message list by skipping channel list updates

* Disable animations when loading new messages

Since the this was actually not causing the messages to jump

* Fix not loading previous messages when there are skipped messages

* Add `ListChange` helpers

* WIP Skipping Messages

* Move the skipping logic to the message list view

* Improve the performance of Message Content Equality

* Fix Giphy not updating the UI when moved from original position

* Add code docs to cell height cache + channel list skip rendering

* Add comments explaining CATransaction in DiffKit reload

* Update CHANGELOG.md

* Update documentation of our dependencies

* When skipping messages, only skip insertions, not all updates

* Improve the readability of skipping messages

* Always store the original messages from the data source to avoid data inconsistencies

* Provide comments for the new introduced methods

* Add Sentry dependency to DemoApp and remove it from StreamChat

* Properly add DifferenceKit dependency

* Ignore third party dependencies when formatting code with SwiftFormat

* Add required declarations changes to DiffKit dependency

Also adds a function to make the removePublicDeclarations.sh more readable

* Undo changes on SwiftyMarkdown done by our SwiftFormat

* Add missing required declaration changes to DiffKit Source files

* Move DiffKit logic to the correct files

* Add missing message id comparison in ChatMessage Content Equality

* Fix crash when message from same user but with difference device is inserted while skipping messages

This caused some data concurrency issues because a lot of data updates are triggered. In order to avoid this reloadData() is much more safe.

* Remove willUpdateMessages events since it is actually not needed

* Fix message cell layout options not being correctly calculated

It was using the data controller directly, instead of the data source messages

* Make sure to use the messages from the data source in ChatThreadVC

* Intial POC for background mappings

* Working POC for Background Mapping list & channel

* Remove leftovers

* Make messages list work

* Move to custom queue

* Bring back some non failing parsing

* Force enable flags

* Keep certain properties as lazy

* Unify flags under _isBackgroundMappingEnabled

* Add force flag to CoreDataLazy

* Add safety to ListDatabaseObserverWrapper

* Use environment in Channel List

* Address small bits

* Fix missing reset

* Remove warnings

* Use custom assertions

* Add tets for BackgroundListDatabaseObserver

* Add CoreDataLazy tests for _isBackgroundMappingEnabled

* Add tests for LazyCachedMapCollection

Co-authored-by: Nuno Vieira <nuno.fcvieira93@gmail.com>
  • Loading branch information
polqf and nuno-vieira committed Sep 15, 2022
1 parent 1dfac5b commit 88f325d
Show file tree
Hide file tree
Showing 18 changed files with 829 additions and 53 deletions.
9 changes: 5 additions & 4 deletions DemoApp/DemoAppConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@ enum DemoAppConfiguration {
// This function is called from `DemoAppCoordinator` before the Chat UI is created
static func setInternalConfiguration() {
StreamRuntimeCheck.assertionsEnabled = isStreamInternalConfiguration
StreamRuntimeCheck._isLazyMappingEnabled = !isStreamInternalConfiguration
StreamRuntimeCheck._isBackgroundMappingEnabled = isStreamInternalConfiguration

configureAtlantisIfNeeded()
trackPerformanceIfNeeded()
}

// HTTP and WebSocket Proxy with Proxyman.app
Expand All @@ -44,8 +43,10 @@ enum DemoAppConfiguration {
}

// Performance tracker
private static func trackPerformanceIfNeeded() {
if isStreamInternalConfiguration {
static func showPerformanceTracker() {
guard isStreamInternalConfiguration else { return }
// PerformanceMonitor seems to have a bug where it cannot find the hierarchy when trying to place its view
DispatchQueue.main.asyncAfter(deadline: .now() + 3) {
PerformanceMonitor.shared().performanceViewConfigurator.options = [.performance]
PerformanceMonitor.shared().start()
}
Expand Down
1 change: 1 addition & 0 deletions DemoApp/DemoAppCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ private extension DemoAppCoordinator {
}

set(rootViewController: chatVC, animated: animated)
DemoAppConfiguration.showPerformanceTracker()
}

func showLogin(animated: Bool) {
Expand Down
4 changes: 2 additions & 2 deletions Sources/StreamChat/Config/StreamRuntimeCheck.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@ public enum StreamRuntimeCheck {

/// For *internal use* only
///
/// Enables lazy mapping of DB models
public static var _isLazyMappingEnabled = true
/// Enables background mapping of DB models
public static var _isBackgroundMappingEnabled = false
}
297 changes: 297 additions & 0 deletions Sources/StreamChat/Controllers/BackgroundListDatabaseObserver.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,297 @@
//
// Copyright © 2022 Stream.io Inc. All rights reserved.
//

import CoreData
import Foundation

class ListDatabaseObserverWrapper<Item, DTO: NSManagedObject> {
private var foreground: ListDatabaseObserver<Item, DTO>?
private var background: BackgroundListDatabaseObserver<Item, DTO>?
let isBackground: Bool

var items: LazyCachedMapCollection<Item> {
if isBackground, let background = background {
return background.items
} else if let foreground = foreground {
return foreground.items
} else {
log.assertionFailure("Should have foreground or background observer")
return []
}
}

/// Called with the aggregated changes after the internal `NSFetchResultsController` calls `controllerWillChangeContent`
/// on its delegate.
var onWillChange: (() -> Void)? {
didSet {
if isBackground {
background?.onWillChange = onWillChange
} else {
foreground?.onWillChange = onWillChange
}
}
}

/// Called with the aggregated changes after the internal `NSFetchResultsController` calls `controllerDidChangeContent`
/// on its delegate.
var onDidChange: (([ListChange<Item>]) -> Void)? {
didSet {
if isBackground {
background?.onDidChange = onDidChange
} else {
foreground?.onChange = onDidChange
}
}
}

init(
isBackground: Bool,
database: DatabaseContainer,
fetchRequest: NSFetchRequest<DTO>,
itemCreator: @escaping (DTO) throws -> Item,
fetchedResultsControllerType: NSFetchedResultsController<DTO>.Type = NSFetchedResultsController<DTO>.self
) {
self.isBackground = isBackground
if isBackground {
background = BackgroundListDatabaseObserver(
context: database.backgroundReadOnlyContext,
fetchRequest: fetchRequest,
itemCreator: itemCreator,
fetchedResultsControllerType: fetchedResultsControllerType
)
} else {
foreground = ListDatabaseObserver(
context: database.viewContext,
fetchRequest: fetchRequest,
itemCreator: itemCreator,
fetchedResultsControllerType: fetchedResultsControllerType
)
}
}

func startObserving() throws {
if isBackground, let background = background {
try background.startObserving()
} else if let foreground = foreground {
try foreground.startObserving()
} else {
log.assertionFailure("Should have foreground or background observer")
}
}
}

class BackgroundListDatabaseObserver<Item, DTO: NSManagedObject> {
/// Called with the aggregated changes after the internal `NSFetchResultsController` calls `controllerWillChangeContent`
/// on its delegate.
var onWillChange: (() -> Void)?

/// Called with the aggregated changes after the internal `NSFetchResultsController` calls `controllerDidChangeContent`
/// on its delegate.
var onDidChange: (([ListChange<Item>]) -> Void)?

/// Used to convert the `DTO`s to the resulting `Item`s.
private let itemCreator: (DTO) throws -> Item

/// Used for observing the changes in the DB.
let frc: NSFetchedResultsController<DTO>

/// Acts like the `NSFetchedResultsController`'s delegate and aggregates the reported changes into easily consumable form.
let changeAggregator: ListChangeAggregator<DTO, Item>

/// When called, release the notification observers
private(set) var releaseNotificationObservers: (() -> Void)?

private let queue = DispatchQueue(label: "io.getstream.list-database-observer", qos: .userInitiated)

private var _items: [Item] = []
var items: LazyCachedMapCollection<Item> {
queue.sync { LazyCachedMapCollection(source: _items, map: { $0 }) }
}

private var _isInitialized: Bool = false
private var isInitialized: Bool {
get {
queue.sync { _isInitialized }
}
set {
queue.async(flags: .barrier) {
self._isInitialized = newValue
}
}
}

deinit {
releaseNotificationObservers?()
}

/// Creates a new `ListObserver`.
///
/// Please note that no updates are reported until you call `startUpdating`.
///
/// - Important: ⚠️ Because the observer uses `NSFetchedResultsController` to observe the entity in the DB, it's required
/// that the provided `fetchRequest` has at lease one `NSSortDescriptor` specified.
///
/// - Parameters:
/// - fetchRequest: The `NSFetchRequest` that specifies the elements of the list.
/// - context: The `NSManagedObjectContext` the observer observes.
/// - fetchedResultsControllerType: The `NSFetchedResultsController` subclass the observe uses to create its FRC. You can
/// inject your custom subclass if needed, i.e. when testing.
init(
context: NSManagedObjectContext,
fetchRequest: NSFetchRequest<DTO>,
itemCreator: @escaping (DTO) throws -> Item,
fetchedResultsControllerType: NSFetchedResultsController<DTO>.Type = NSFetchedResultsController<DTO>.self
) {
self.itemCreator = itemCreator
changeAggregator = ListChangeAggregator<DTO, Item>(itemCreator: itemCreator)
frc = fetchedResultsControllerType.init(
fetchRequest: fetchRequest,
managedObjectContext: context,
sectionNameKeyPath: nil,
cacheName: nil
)

changeAggregator.onWillChange = { [weak self] in
self?.notifyWillChange()
}

changeAggregator.onDidChange = { [weak self] changes in
self?.processItems(changes)
}

listenForRemoveAllDataNotifications()
}

/// Starts observing the changes in the database. The current items in the list are synchronously available in the
/// `item` variable, after this function returns.
///
/// - Throws: An error if the provided fetch request fails.
func startObserving() throws {
guard !isInitialized else { return }
isInitialized = true

onWillChange?()

do {
try frc.performFetch()
} catch {
log.error("Failed to start observing database: \(error). This is an internal error.")
throw error
}

frc.delegate = changeAggregator

processItems()
}

private func notifyWillChange() {
DispatchQueue.main.async { [weak self] in
self?.onWillChange?()
}
}

private func notifyDidChange(changes: [ListChange<Item>]) {
DispatchQueue.main.async { [weak self] in
self?.onDidChange?(changes)
}
}

private func mapItems(completion: @escaping ([Item]) -> Void) {
let objects = frc.fetchedObjects ?? []
let context = frc.managedObjectContext

var items = [Item?](repeating: nil, count: objects.count)

let group = DispatchGroup()
for (i, dto) in objects.enumerated() {
group.enter()
context.perform { [weak self] in
items[i] = try? self?.itemCreator(dto)
group.leave()
}
}

group.notify(queue: queue) {
completion(items.compactMap { $0 })
}
}

private func processItems(_ changes: [ListChange<Item>] = []) {
mapItems { [weak self] items in
self?._items = items
self?.notifyDidChange(changes: changes)
}
}

/// Listens for `Will/DidRemoveAllData` notifications from the context and simulates the callback when the notifications
/// are received.
private func listenForRemoveAllDataNotifications() {
let notificationCenter = NotificationCenter.default
let context = frc.managedObjectContext

// When `WillRemoveAllDataNotification` is received, we need to simulate the callback from change observer, like all
// existing entities are being removed. At this point, these entities still existing in the context, and it's safe to
// access and serialize them.
let willRemoveAllDataNotificationObserver = notificationCenter.addObserver(
forName: DatabaseContainer.WillRemoveAllDataNotification,
object: context,
queue: .main
) { [weak self] _ in
// Technically, this should rather be `unowned`, however, `deinit` is not always called on the main thread which can
// cause a race condition when the notification observers are not removed at the right time.
guard let self = self else { return }
guard let fetchResultsController = self.frc as? NSFetchedResultsController<NSFetchRequestResult> else { return }

// Simulate ChangeObserver callbacks like all data are being removed
self.changeAggregator.controllerWillChangeContent(fetchResultsController)

self.frc.fetchedObjects?.enumerated().forEach { index, item in
self.changeAggregator.controller(
fetchResultsController,
didChange: item,
at: IndexPath(item: index, section: 0),
for: .delete,
newIndexPath: nil
)
}

// Remove the cached items since they're now deleted, technically. It is important for it to be reset before
// calling `controllerDidChangeContent` so it properly reflects the state
self.queue.sync {
self._items = []
}

// Publish the changes
self.changeAggregator.controllerDidChangeContent(fetchResultsController)

// Remove delegate so it doesn't get further removal updates
self.frc.delegate = nil
}

// When `DidRemoveAllDataNotification` is received, we need to reset the FRC. At this point, the entities are removed but
// the FRC doesn't know about it yet. Resetting the FRC removes the content of `FRC.fetchedObjects`.
let didRemoveAllDataNotificationObserver = notificationCenter.addObserver(
forName: DatabaseContainer.DidRemoveAllDataNotification,
object: context,
queue: .main
) { [weak self] _ in
// Technically, this should rather be `unowned`, however, `deinit` is not always called on the main thread which can
// cause a race condition when the notification observers are not removed at the right time.
guard let self = self else { return }

// Reset FRC which causes the current `frc.fetchedObjects` to be reloaded
do {
self.isInitialized = false
try self.startObserving()
} catch {
log.error("Error when starting observing: \(error)")
}
}

releaseNotificationObservers = { [weak notificationCenter] in
notificationCenter?.removeObserver(willRemoveAllDataNotificationObserver)
notificationCenter?.removeObserver(didRemoveAllDataNotificationObserver)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ public class ChatChannelController: DataController, DelegateCallable, DataStoreP
/// Database observers.
/// Will be `nil` when observing channel with backend generated `id` is not yet created.
@Cached private var channelObserver: EntityDatabaseObserver<ChatChannel, ChannelDTO>?
@Cached private var messagesObserver: ListDatabaseObserver<ChatMessage, MessageDTO>?
private var messagesObserver: ListDatabaseObserverWrapper<ChatMessage, MessageDTO>?

private var eventObservers: [EventObserver] = []
private let environment: Environment
Expand Down Expand Up @@ -353,7 +353,7 @@ public class ChatChannelController: DataController, DelegateCallable, DataStoreP
}

private func setMessagesObserver() {
_messagesObserver.computeValue = { [weak self] in
messagesObserver = { [weak self] in
guard let self = self else {
log.warning("Callback called while self is nil")
return nil
Expand All @@ -371,8 +371,9 @@ public class ChatChannelController: DataController, DelegateCallable, DataStoreP
shouldShowShadowedMessages = self.client.databaseContainer.viewContext.shouldShowShadowedMessages
}

let observer = ListDatabaseObserver(
context: self.client.databaseContainer.viewContext,
let observer = ListDatabaseObserverWrapper(
isBackground: StreamRuntimeCheck._isBackgroundMappingEnabled,
database: client.databaseContainer,
fetchRequest: MessageDTO.messagesFetchRequest(
for: cid,
sortAscending: sortAscending,
Expand All @@ -381,7 +382,7 @@ public class ChatChannelController: DataController, DelegateCallable, DataStoreP
),
itemCreator: { try $0.asModel() as ChatMessage }
)
observer.onChange = { [weak self] changes in
observer.onDidChange = { [weak self] changes in
self?.delegateCallback {
guard let self = self else { return }
log.debug("didUpdateMessages: \(changes.map(\.debugDescription))")
Expand All @@ -390,7 +391,7 @@ public class ChatChannelController: DataController, DelegateCallable, DataStoreP
}

return observer
}
}()
}

override public func synchronize(_ completion: ((_ error: Error?) -> Void)? = nil) {
Expand Down Expand Up @@ -482,8 +483,8 @@ public class ChatChannelController: DataController, DelegateCallable, DataStoreP
}

private func startMessagesObserver() -> Error? {
_messagesObserver.reset()
setMessagesObserver()

do {
try messagesObserver?.startObserving()
return nil
Expand Down
Loading

0 comments on commit 88f325d

Please sign in to comment.