Fix performance issues with search in Reader Subscriptions #24841
Conversation
kean
force-pushed
the
fix/subscription-search-performance
branch
from
f1dda0f to
3354a73
Compare
Generated by 🚫 Danger |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 29144 | |
| Version | PR #24841 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 4d52aa1 | |
| Installation URL | 036kog98dnnd8 |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 29144 | |
| Version | PR #24841 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 4d52aa1 | |
| Installation URL | 25joq5gd5ueso |
| // If there's a pending search, start it now | ||
| if let pendingSearchText { | ||
| performBackgroundSearch(searchText: pendingSearchText) | ||
| } |
There was a problem hiding this comment.
I find the use of pendingSearchText is a bit confusing. Every change to the search input would trigger a performBackgroundSearch call, which would perform a new search. That makes me think if this call here is redundant?
There was a problem hiding this comment.
It was late and I failed to fully check the implementation. It does seems like it included parts of two separate strategies in one. I rewrote it manually.
| let searchableData = subscriptions.map(SearchableSubscription.init) | ||
|
|
||
| // Perform the search on a background queue with parallel processing | ||
| let resultObjectIDs = await StringRankedSearch(searchTerm: currentSearchText) |
There was a problem hiding this comment.
Maybe check cancellation before the await call, just in case performBackgroundSearch gets called too frequently?
| let items = Array(items) | ||
|
|
||
| // Calculate optimal chunk size based on CPU cores (leave one core free) | ||
| let maxConcurrency = max(1, ProcessInfo.processInfo.activeProcessorCount - 1) |
There was a problem hiding this comment.
This is not a request to change, but I'm curious if controlling the number of Task instances in the TaskGroup is necessary.
Let's say you create one Task for each element, that does not necessarily mean TaskGroup will run them simultaneously on the same number of threads. I'd be surprised if Swift actually does that. I'd expect TaskGroup, or Swift concurrency in general, to schedule Task appropriately according to the number of CPU cores, or whatever metric they see appropriate.
There was a problem hiding this comment.
Swift concurrency will limit the number of tasks to the number of available cores. The app still needs to create chunks as submitting one at a time would likely be wasteful – more time will be spent on dispatching work than performing it. I think a better option would probably be to create more chunk – not too large not too small.
| .parallelSearch(in: searchableData) { $0.searchableText } | ||
| .map(\.objectID) | ||
|
|
||
| searchResults = subscriptions.filter { resultObjectIDs.contains($0.objectID) } |
There was a problem hiding this comment.
Convert resultObjectIDs to a Set before filter?
There was a problem hiding this comment.
Oh right forgot to do it – fixed.
kean
force-pushed
the
fix/subscription-search-performance
branch
from
cbd106a to
5ffd81e
Compare
|
4 participants
Fixes https://linear.app/a8c/issue/CMM-93/ipad-search-function-freezing-in-reader. Search is now performed in the background and in parallel. It would be nice to optimize search itself, but I didn't get to it. It's OK for most use-cases.