search

apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.29k stars 2.64k forks source link

useQuery teardown performance #10291

Open nathanmarks opened 1 year ago

nathanmarks commented 1 year ago

When we end up with a lot of useQuery hooks on the page, we've noticed significant performance issues trying to unmount the components on navigation. This becomes a huge issue on pages where we do infinite scroll pagination and end up with a lot of results, each of which has numerous useQuery instances within.

I tracked this down to here: https://github.com/apollographql/apollo-client/blob/v3.7.1/src/core/ObservableQuery.ts#L926

stopQuery calls broadcastQueries: https://github.com/apollographql/apollo-client/blob/v3.7.1/src/core/QueryManager.ts#L703

This means that for each of the ObservableQuery instances being torn down (which, is a LOT), we're iterating over every single query known by QueryManager to maybe synchronously flush/notify their listeners if they have already been marked as dirty. When QueryManager is aware of thousands of queries, this adds up and can lock up the thread for a long period of time -- especially on slower devices.

This seems to be somehat of a brute force flush, the only place queryInfo.dirty seems to even get set to true is in setDiff, which will kick notify off itself if it needs to: https://github.com/apollographql/apollo-client/blob/v3.7.1/src/core/QueryInfo.ts#L204-L215. Yes it's not synchronous there, but there doesn't seem to be a reason to incur this cost on every single useQuery teardown just to maybe flush synchronously. With some brief tests I couldn't find a single place in our app where any of those broadcastQuery notifies were synchronously flushing an update anyways. It's just a ton of work done for nothing.

Why do we need to do this broadcast to all queries here? it's crippling when scaling the use of the apollo hooks.

I did a before and after performance recording in some extreme (but real) circumstances. The "change" here is that i changed the teardown method to call stopQueryNoBroadcast instead.

Before change image
After change image
dkempner commented 1 year ago

+1.

Similar to some of the performance issues I noted in this community post.

alekangelov commented 1 year ago

+1 here's another issue that's regarding this

https://github.com/apollographql/apollo-client/issues/10322

@jerelmiller Is there any implication to doing stopQueryNoBroadcast? Could there be an options object in the constructor to let us change this behaviour?

Edit: At least until InMemoryCache's performance is fixed.