Rewrite subscription handling and polling calculations for better perf#5064
Merged
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. |
Collaborator
Author
|
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
EskiMojo14
reviewed
Sep 1, 2025
…eduxjs/redux-toolkit into feature/5052-subscription-perf
…eduxjs/redux-toolkit into feature/5052-subscription-perf
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.