-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Rewrite subscription handling and polling calculations for better perf #5064
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
Conversation
# Conflicts: # packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts # packages/toolkit/src/query/core/buildMiddleware/index.ts # packages/toolkit/src/query/core/buildMiddleware/types.ts
# Conflicts: # packages/toolkit/src/query/core/buildMiddleware/index.ts # packages/toolkit/src/query/core/buildMiddleware/types.ts
# Conflicts: # packages/toolkit/src/query/core/buildMiddleware/polling.ts # packages/toolkit/src/query/core/buildMiddleware/types.ts # packages/toolkit/src/query/tests/polling.test.tsx
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 86b9072:
|
size-limit report 📦
|
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Tests are passing, we're just seeing PR failures due to |
# Conflicts: # packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts # packages/toolkit/src/query/core/buildMiddleware/index.ts # packages/toolkit/src/query/core/buildMiddleware/types.ts
# Conflicts: # packages/toolkit/src/query/core/buildMiddleware/index.ts # packages/toolkit/src/query/core/buildMiddleware/types.ts
# Conflicts: # packages/toolkit/src/query/core/buildMiddleware/polling.ts # packages/toolkit/src/query/core/buildMiddleware/types.ts # packages/toolkit/src/query/tests/polling.test.tsx
6ff3680 to
7513bfa
Compare
packages/toolkit/src/query/core/buildMiddleware/batchActions.ts
Outdated
Show resolved
Hide resolved
packages/toolkit/src/query/core/buildMiddleware/batchActions.ts
Outdated
Show resolved
Hide resolved
packages/toolkit/src/query/core/buildMiddleware/invalidationByTags.ts
Outdated
Show resolved
Hide resolved
…eduxjs/redux-toolkit into feature/5052-subscription-perf
…eduxjs/redux-toolkit into feature/5052-subscription-perf
This PR significantly rewrites our existing internal RTKQ middleware subscription tracking and polling trigger systems to greatly improve perf, especially in the many-subscriptions-one-cache-entry case.
Fixes #5052
Background
This rewrite was the result of working on #5061 to abort pending requests when cache entries are removed, and #5052 that demonstrated slow perf with thousands of subscriptions to the same cache entry.
Polling
In profiling the many-subscriptions case, I saw that
updatePollingIntervalwas running after every single additional subscription (which we signify internally as a/rejectedaction withcondition: true. That's because any new subscription could have included subscription options for a new polling value, and we need to find the lowest polling interval.However, there's no reason to be doing that recalculation synchronously every time. We don't even kick off the actual polling request immediately - we set a timer. So, instead it's better to debounce this and do a single
updatePollingIntervalcalculation on the next tick.Additionally, we were using plain JS
Records as the lookup table for polling items, and iterating keys in those objects to get the polling value entries. Switching those to beMaps isn't worse and probably helps.The only way I could figure out to test this was to hack in a test-only polling update counter and exfiltrate that via the middleware. Stupidly ugly but worked.
Subscriptions
Originally, our internal subscription system used plain object
Records as the key/value lookups for subscriptions by request ID, as a holdover from some of the earlier implementations. However, that meant that any time we needed to check for size or "is there at least 1 subscription", we had to do afor...incheck to iterate keys and count them. Based on perf profiling, this turned out to be relatively expensive.This was made trickier because we added a pseudo-subscription for
${requestId}_runningin #3709 when we added cache entries for non-subscribed initiated queries, and needed to prevent cache collection of those.When I reworked the logic in
cacheCollection.tsin #3709 to add abort handling, I had to special-case the_runningsituation as we iterated keys.This can all be avoided if we just move the
currentQueriesmap from thebuildInitiate()closure into the middlewareInternalStateobject and make it available to all the middleware, then use that to check if there's a running query for this cache key.Additionally, changing all of the subscription lookups from a
Recordto aMaplets us checksubscriptions.sizewithout any key iterating. This did require touching all the places we accessed the subscriptions accordingly (including making sure we switched fromObject.keys(subscriptions)tosubscriptions.keys()).Changes
subscriptionsUpdatedserialization behaviorInternalStateobject to be initialized inmodule.tsso it can be passed to bothbuildInitiate()andbuildMiddleware()runningQueries/runningMutationsmaps to be inInternalStatesubscriptionsto be nestedMaps instead ofRecords, including all accesses to the subscription datasubscriptionsUpdatedserialization to stringify the maps accordingly so we still get the right plain JS objects for use in the reducer${requestId}_runninghandling and replaced it with a direct check againstrunningQueriesfor that request IDMapinstead of aRecordsetTimeout(0)so we only do one set of updates per tickResults
Using the thousands-of-subscribed-components example from #5052 , and just unchecking and re-checking the "Render children" box to unmount and remount the components:
Perf before, dev:
Perf after, dev:
You can see we've eliminated the major perf bottlenecks in
polling.tsandcacheCollection.ts, and we're left with just the React dev effect overhead.Perf before, prod:
Perf after, prod
Similarly, total execution time dropped significantly and RTKQ is no longer at the top of the list.