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

Rewrite providedTags handling for better perf #4910

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

markerikson
Copy link
Collaborator

@markerikson markerikson commented Mar 27, 2025

Per #4909 , our recent update to add the missing providesTags handling for upsertQueryEntries in #4872 has resulted in a significant perf regression for that use case.

The current api.provides state structure looks like:

427705022-b353576e-5515-4813-9cdb-b42747a6ee63

When we update tags for a cache key, we first need to go in and remove that cache key from all tag entries that referenced it. Unfortunately, the existing logic is doing that via a brute-force search over all existing tag > cache key arrays. Not only is this O(n), but it's doing so by going through an Immer draft proxy, which is slower:

      updateProvidedBy: {
        reducer(
          draft,
          action: PayloadAction<{
            queryCacheKey: QueryCacheKey
            providedTags: readonly FullTagDescription<string>[]
          }>,
        ) {
          const { queryCacheKey, providedTags } = action.payload

          for (const tagTypeSubscriptions of Object.values(draft)) {
            for (const idSubscriptions of Object.values(tagTypeSubscriptions)) {
              const foundAt = idSubscriptions.indexOf(queryCacheKey)
              if (foundAt !== -1) {
                idSubscriptions.splice(foundAt, 1)
              }
            }
          }

          for (const { type, id } of providedTags) {
            const subscribedQueries = ((draft[type] ??= {})[
              id || '__internal_without_id'
            ] ??= [])
            const alreadySubscribed = subscribedQueries.includes(queryCacheKey)
            if (!alreadySubscribed) {
              subscribedQueries.push(queryCacheKey)
            }
          }
        },

To make things worse, the upsertQueryEntries handling was doing this one cache key at a time. So, when we try to insert 5000 cache entries, we do the whole "painter's algorithm" anti-pattern and iterate through 1, 2, .... 4998, 4999 different tag > cache key arrays just to try to clear out any existing uses of each cache key.

This PR restructures the invalidation state to be:

export type InvalidationState<TagTypes extends string> = {
  tags: {
    [_ in TagTypes]: {
      [id: string]: Array<QueryCacheKey>
      [id: number]: Array<QueryCacheKey>
    }
  }
  keys: Record<QueryCacheKey, Array<FullTagDescription<any>>>
}

From there, we just save the per-cache-key array of tags in a second lookup table, which then means that deletion simplifies down to grabbing that array of tags and removing the cache key from the corresponding per-tag cache key arrays. So, instead of iterating over the entire list of all tags, we can just grab the few specific tags and target those directly. (To toss two CS-like bits of knowledge in here, we're "trading memory for speed".)

I also updated the upsertQueryEntries handling for tags to try to batch together all of the entries into one nested reducer call. Didn't appear to make much of a difference, but shouldn't hurt.

In local testing, this knocked the runtime for inserting 5000 entries from 23 seconds down to 90 milliseconds. (Yeah. It was that bad.)

The downside is that this does change the structure of state.api.provided. We've always considered that internal, so this is not a public breaking change and can thus be a patch release.

Unfortunately, the Redux DevTools RTKQ display does expect the existing state structure, and I've confirmed that trying to view a cache entry with this build causes the entire DevTools to crash when it accesses an undefined field due to the structure change.

So, we're going to need to update the DevTools to handle both cases and get that release out, then publish this fix.

The PR preview build here can be used as a workaround in the meantime.

Copy link

codesandbox bot commented Mar 27, 2025

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link

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 c16a3cd:

Sandbox Source
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
@examples-action-listener/counter Configuration
rtk-esm-cra Configuration

Copy link

size-limit report 📦

Path Size
1. entry point: @reduxjs/toolkit/query/react (modern.mjs) 14.53 KB (+0.34% 🔺)
1. entry point: @reduxjs/toolkit/query (cjs, production.min.cjs) 23.42 KB (+0.29% 🔺)
1. entry point: @reduxjs/toolkit/query/react (cjs, production.min.cjs) 25.86 KB (+0.29% 🔺)
2. entry point: @reduxjs/toolkit/query (without dependencies) (cjs, production.min.cjs) 10.15 KB (+0.43% 🔺)
3. createApi (.modern.mjs) 14.94 KB (+0.31% 🔺)
3. createApi (react) (.modern.mjs) 16.97 KB (+0.29% 🔺)

Copy link

netlify bot commented Mar 27, 2025

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit c16a3cd
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/67e5b1359437de000883cfee
😎 Deploy Preview https://deploy-preview-4910--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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

Successfully merging this pull request may close these issues.

1 participant