-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
useQueries have quadratic performance in relation to the number of queries #8604
Comments
The problem is the If you have N queries managed by I could be wrong, of course, but I think reusing function getOptimisticResult(queries, combine) {
const matches = this.#findMatchingObservers(queries)
const result = matches.map(match => ...)
return [
result,
(r) => {
return this.#combineResult(r ?? result, combine)
},
() => {
- // Avoid calling #trackResult which calls findMatchingObservers again
- // Instead, directly reuse the 'matches'
+ return matches.map((match, index) => {
+ const observerResult = result[index]
+ // ...
+ })
},
]
}
|
I don't think there is any issue with getOptimisticResult. It'll only be called once in my scenario. It then doing N operations is fine as it only leads to O(N) performance. Maybe it could be even better with better bookkeeping but that is not what my issue is about. The issue is with |
I’ll happily accept a PR that changes how this all works without breaking anything 😂 |
Looking at the code myself and at your PR @joseph0926 I realized that this will be quite hard to solve entirely. Your PR does remove some quadratic work but onUpdate still contain the mentioned indexOf, trackResult still contain a map over all observers, any user defined combine method will most likely also do N amount of work and if not combineResult will in replaceEqualDeep. What fundamentally makes multiple useQuery faster than useQueries is reacts auto batching. We notify react once for each updated useQuery but react only rerender once. I thought that maybe we could do the same for useQueries by notifying useSyncExternalStore on every update and doing combineResult/trackResult lazily when react request the value in getSnapshot. However it seems like react will call getSnapshot every time it is notified defeating this idea. |
I think you're right. |
@GabbeV @joseph0926 I can merge the PR as all current tests pass. Or do you want to continue tinkering on a better solution? |
@TkDodo |
Describe the bug
This PR #8295 introduced a significant performance regression for us. A commenter on that PR seem to have had a similar issue #8295 (comment).
We use the data loader pattern together with useQueries to give individual results from a batch fetch individual cache entries in react-query. Something like this:
Looking at a performance profile i noticed this will result in one call to #findMatchingObservers in QueriesObserver for every query when the fetch resolve. #findMatchingObservers will then loop over all observers for for all queries internally making the performance quadratic O(n^2).
In our case we were loading way too many ids at once for this to be reasonable so the performance was already bad before the change but became browser freezing bad after the change.
Seems like the problem however should be avoidable. #onUpdate know what observer was updated so should not have to find it. This might however require some reorganization of the code and how observers are stored in QueriesObserver.
The indexOf in #onUpdate is also a source of quadratic performance that should be avoidable however indexOf is a way faster operation than what happens in #findMatchingObservers so it might not be an issue until even more queries are fetched at the same time.
Your minimal, reproducible example
https://codesandbox.io/p/sandbox/boring-https-dxlhnv?file=%2Fsrc%2FApp.js%3A17%2C62
Steps to reproduce
Click on the numeric buttons to try loading different amount of queries and click on the "useQueries" and "multiple useQuery" buttons to switch between using a single useQueries or multiple useQuery and observer the performance difference.
Expected behavior
I expect useQueries to have similar or faster performance than multiple useQuery.
How often does this bug happen?
Every time
Screenshots or Videos
No response
Platform
Tanstack Query adapter
react-query
TanStack Query version
5.64.2
TypeScript version
No response
Additional context
No response
The text was updated successfully, but these errors were encountered: