Fix poor message search performance #680
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nuno-vieira
commented
Dec 5, 2024
| loadingSearchResults = true | ||
| messageSearchController?.search(query: query, completion: { [weak self] _ in | ||
| messageSearchController?.search(text: searchText) { [weak self] _ in |
There was a problem hiding this comment.
Replaced the search(query:) with search(text:) since it is the same thing but with some optimizations
nuno-vieira
commented
Dec 5, 2024
Comment on lines
-112
to
-126
| extension ChatMessage { | ||
|
|
||
| func makeChannelSelectionInfo(with chatClient: ChatClient) -> ChannelSelectionInfo? { | ||
| if let channelId = cid, | ||
| let channel = chatClient.channelController(for: channelId).channel { | ||
| let searchResult = ChannelSelectionInfo( | ||
| channel: channel, | ||
| message: self | ||
| ) | ||
| return searchResult | ||
| } else { | ||
| return nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Removed the extension because it could be used in other places , and since the performance it not good, better to not make it easy to use.
SDK Size
|
martinmitrevski
approved these changes
Dec 5, 2024
CHANGELOG.md
Outdated
| @@ -4,6 +4,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). | |||
| # Upcoming | |||
|
|
|||
| ### 🔄 Changed | |||
| - Fix poor message search performance [#680](https://github.com/GetStream/stream-chat-swiftui/pull/680) | |||
There was a problem hiding this comment.
"Improve message search performance" sounds better?
| queue.async { | ||
| let results: [ChannelSelectionInfo] = messageSearchController.messages.compactMap { message in | ||
| guard let channelId = message.cid else { return nil } | ||
| guard let channel = messageSearchController.client.channelController(for: channelId).channel else { |
There was a problem hiding this comment.
why do we have to go through the message search controller here? We have the client injected in the VM.
There was a problem hiding this comment.
ah yes you are correct 👍
|
laevandus
approved these changes
Dec 5, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🔗 Issue Links
Fixes IOS-574
🎯 Goal
Fixes the poor performance of Message Search.
🛠 Implementation
Problem
The root cause of the issue was in the
updateMessageSearchResults()function. We were accessing thechannelController.channelfor every search result. The problem with this is that we callstartObservers()for everychannelproperty access, which performs a Core Data fetch and, in this case, on the main thread.We can see this from the time profiler:
Besides this, we were calling
updateMessageSearchResults()twice, for every completion block performed in theloadNextMessages()and also in thedidUpdateMessages()delegate calls.First Solution
My first idea was to follow the UIKit approach. There, we use the
dataStoreto fetch the channel directly. However, this also does not work for SwiftUI. The reason is that for UIKit, thedataStoreis accessed when the cell is reused, whereas in SwiftUI, we map all the results at once, and so we access thedataStore.channelfor every result again, which means we convert the DTO to model on the main thread multiple times.Profile result:
So, to avoid converting the channel models every time, I implemented a cache that would reuse the existing channels instead of always doing the DTO -> Model conversion.
This worked and the performance was great, but it does require an additional cache, which can increase the memory and is dependent on the amount of different channels we have in the result. So the final solution (check next section) is probably the better one.
Final Solution
Eventually, I realised that I could simply use the original implementation but offload the search results mapping to a background thread. This worked flawlessly, and it has the less amount of changes and does not require any additional caching.
Result (No hangs, and Main thread not busy):
🧪 Testing
Perform a message search in the Channel List. For example, search for "Hey".
Known Issue: The channel name is not rendered for some search results. This is an existing issue and out of the scope of this PR.
🎨 Changes
ScreenRecording_12-05-2024.01-13-47_1.MP4
ScreenRecording_12-05-2024.00-49-49_1.MP4
☑️ Checklist