Skip to content
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

Open
GabbeV opened this issue Feb 4, 2025 · 2 comments
Open

Comments

@GabbeV
Copy link

GabbeV commented Feb 4, 2025

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:

const fooLoader = new DataLoader(ids => fetchFoos(ids));
const queries = useQueries({
  queries: ids.map(id => ({
    queryKey: ["foo", id],
    queryFn: () => fooLoader.load(id),
  })),
});

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

  • OS: Windows 11 24H2 26100.2894
  • Browser: Firefox 135.0b9, Chrome 132.0.6834.160

Tanstack Query adapter

react-query

TanStack Query version

5.64.2

TypeScript version

No response

Additional context

No response

@joseph0926
Copy link

joseph0926 commented Feb 5, 2025

Sorry for the awkward English I used a translator

I haven't actually tested with large queries, but from looking at the code, I suspect there may be performance issues due to the following reason.

  • In the original implementation, findMatchingObservers was called only once inside getOptimisticResult, and its result matches was reused for tracking.

  • However, in the modified code, we now already call findMatchingObservers to get matches and then call this.#trackResult(result, queries). Inside #trackResult, we perform another call to findMatchingObservers

    return this.#trackResult(result, queries)
    #trackResult(result, queries) {
      const matches = this.#findMatchingObservers(queries)
      ...
    }
  • This means findMatchingObservers can be run multiple times during a single rendering or notification cycle.

The problem is the findMatchingObservers function is expensive


If you have N queries managed by useQueries, and multiple of them are updated nearly simultaneously, Each update may trigger onUpdatenotifytrackResultfindMatchingObservers.
There are also O(N^2) possibilities


I could be wrong, of course, but I think reusing matches would fix it again

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]
+          // ...
+        })
    },
  ]
}

@GabbeV
Copy link
Author

GabbeV commented Feb 5, 2025

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 onUpdate → notify → trackResult → findMatchingObservers. onUpdate will be called N times in my scenario and then findMatchingObservers will do N operations leading to O(N*N) performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants