perf(query-core): optimize find/findAll lookups in QueryCache and MutationCache#10174
perf(query-core): optimize find/findAll lookups in QueryCache and MutationCache#10174976520 wants to merge 5 commits intoTanStack:mainfrom
Conversation
🦋 Changeset detectedLatest commit: b018ef1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughIntroduces O(1) key-based lookup optimizations by adding index maps to MutationCache and QueryCache, updating add/remove/clear paths to maintain indexes, and changing find/findAll and some iteration paths to use indexed access when filters.exact and a key are provided. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit b018ef1
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/query-core/src/queryCache.ts (3)
215-223:findAllexact-path returns[]on filter mismatch — correct but asymmetric withfind.When the fast-path candidate exists but fails the remaining filters (e.g.,
type,stale,predicate),findAllreturns[]immediately rather than falling through to scan. This is correct (at most one query can match a given exact key), but the behavior is subtly different from thefindmethod where the candidate-not-found case falls through to the linear scan while candidate-found-but-mismatch does not. A brief comment would help future readers understand the early return is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/queryCache.ts` around lines 215 - 223, The findAll fast-path in QueryCache (inside findAll) early-returns [] when a candidate is found via hashKey(filters.queryKey) but fails matchQuery(filtersWithoutKey, candidate); add a brief comment above this block clarifying that this early return is intentional because at most one query can match an exact queryKey, so a mismatched candidate means no other query can match and we should not fall through to the linear scan (contrast with find which only falls through when the candidate is missing). Mention the involved symbols: findAll, find, `#queries`, hashKey, matchQuery, and filters.queryKey to help future readers locate the logic.
211-213: Minor:Object.keys(filters).length === 0allocates an array to check emptiness.For a perf-focused PR, you could avoid the temporary
Object.keys()array. However, this is only called once perfindAll({})invocation and the object is typically tiny, so the impact is negligible.Alternative without allocation
A zero-allocation check using a
for...inbail-out pattern, though arguably less readable:- if (Object.keys(filters).length === 0) { + let hasFilters = false + for (const _ in filters) { hasFilters = true; break } + if (!hasFilters) { return [...this.#queries.values()] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/queryCache.ts` around lines 211 - 213, The current emptiness check uses Object.keys(filters).length === 0 which allocates an array; update the check inside the findAll (or the method containing filters) to avoid allocation by using a zero-allocation pattern (e.g., bail out with a for...in that returns early if any enumerable property exists) so that when filters is empty you still return [...this.#queries.values()] without creating the temporary keys array; locate the check referencing filters and replace the Object.keys(...) length test with the non-allocating for...in emptiness test.
188-207: Fast path optimization silently degrades to O(n) when customqueryKeyHashFnis used.Queries are stored under
query.queryHash, which is computed asoptions.queryHash ?? hashQueryKeyByOptions(queryKey, options). The fast path on line 191 attempts a direct lookup usinghashKey(queryKey), which will fail when a custom hash function is configured in options, causing the code to fall through to the linear scan at line 201. While correctness is preserved (the linear scan usesmatchQuerywhich properly validates using the query's stored options), this means the O(1) optimization is silently lost for any queries created with customqueryKeyHashFn. Consider adding an inline comment documenting this trade-off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/queryCache.ts` around lines 188 - 207, The fast-path direct lookup using hashKey(defaultedFilters.queryKey) in the get/find logic (around defaultedFilters, hashKey, and matchQuery) can miss queries that were stored under a custom hash (query.queryHash computed with options.queryKeyHashFn), silently degrading to the O(n) linear scan; update the code in the fast-path block to include an inline comment explaining this trade-off (that direct lookup only works for the global/default hashKey and queries with a custom queryKeyHashFn are stored under query.queryHash so the code falls back to the linear scan using matchQuery), and mention that preserving O(1) for custom hashes would require computing/using the query's stored hash when possible.packages/query-core/src/mutationCache.ts (1)
237-254: Verify thatmatchMutationbehaves correctly whenmutationKeyis stripped butexactremains.The fast path removes
mutationKeyfrom the filters before callingmatchMutation, butexact: trueis still present. Looking atmatchMutation, this is safe becauseexactis only consulted inside theif (mutationKey)branch, which is skipped whenmutationKeyis absent. However, this coupling is subtle — ifmatchMutationever changes to inspectexactindependently, this optimization would silently break.Consider adding a brief inline comment explaining why stripping
mutationKeyalone is sufficient.Also applies to: 265-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/mutationCache.ts` around lines 237 - 254, Add a short inline comment in the fast-path blocks inside MutationCache where defaultedFilters.mutationKey is stripped before calling matchMutation (the blocks that iterate candidates from this.#byMutationKey and use filtersWithoutKey) explaining that removing mutationKey is safe even when exact: true because matchMutation only checks exact inside its mutationKey branch; reference the symbols defaultedFilters, mutationKey, exact, matchMutation and the candidates loop so future maintainers know this coupling and to update the comment if matchMutation's logic changes (apply the same comment to the second occurrence around the other fast-path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/query-core/src/mutationCache.ts`:
- Around line 162-174: The deletion from the private map `#byMutationKey` is
missing a defensive check and should mirror the `#scopes` logic: when keyed.length
=== 1, verify that keyed[0] === mutation before calling
this.#byMutationKey.delete(hash) to avoid deleting the bucket if the invariant
is violated; update the removal code that handles mutation.options.mutationKey /
hashKey(mutationKey) to perform this extra equality check (use the existing
local variables mutation, keyed and hash) and only delete the map entry when the
first item equals the mutation.
---
Nitpick comments:
In `@packages/query-core/src/mutationCache.ts`:
- Around line 237-254: Add a short inline comment in the fast-path blocks inside
MutationCache where defaultedFilters.mutationKey is stripped before calling
matchMutation (the blocks that iterate candidates from this.#byMutationKey and
use filtersWithoutKey) explaining that removing mutationKey is safe even when
exact: true because matchMutation only checks exact inside its mutationKey
branch; reference the symbols defaultedFilters, mutationKey, exact,
matchMutation and the candidates loop so future maintainers know this coupling
and to update the comment if matchMutation's logic changes (apply the same
comment to the second occurrence around the other fast-path).
In `@packages/query-core/src/queryCache.ts`:
- Around line 215-223: The findAll fast-path in QueryCache (inside findAll)
early-returns [] when a candidate is found via hashKey(filters.queryKey) but
fails matchQuery(filtersWithoutKey, candidate); add a brief comment above this
block clarifying that this early return is intentional because at most one query
can match an exact queryKey, so a mismatched candidate means no other query can
match and we should not fall through to the linear scan (contrast with find
which only falls through when the candidate is missing). Mention the involved
symbols: findAll, find, `#queries`, hashKey, matchQuery, and filters.queryKey to
help future readers locate the logic.
- Around line 211-213: The current emptiness check uses
Object.keys(filters).length === 0 which allocates an array; update the check
inside the findAll (or the method containing filters) to avoid allocation by
using a zero-allocation pattern (e.g., bail out with a for...in that returns
early if any enumerable property exists) so that when filters is empty you still
return [...this.#queries.values()] without creating the temporary keys array;
locate the check referencing filters and replace the Object.keys(...) length
test with the non-allocating for...in emptiness test.
- Around line 188-207: The fast-path direct lookup using
hashKey(defaultedFilters.queryKey) in the get/find logic (around
defaultedFilters, hashKey, and matchQuery) can miss queries that were stored
under a custom hash (query.queryHash computed with options.queryKeyHashFn),
silently degrading to the O(n) linear scan; update the code in the fast-path
block to include an inline comment explaining this trade-off (that direct lookup
only works for the global/default hashKey and queries with a custom
queryKeyHashFn are stored under query.queryHash so the code falls back to the
linear scan using matchQuery), and mention that preserving O(1) for custom
hashes would require computing/using the query's stored hash when possible.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/perf-cache-lookup-optimization.mdpackages/query-core/src/mutationCache.tspackages/query-core/src/queryCache.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/query-core/src/mutationCache.ts (1)
162-173: Past concern is resolved by theindexOf-based removal.The previous version of this block was flagged for omitting the
keyed[0] === mutationguard before deleting the bucket. The current implementation avoids that entire branch by usingindexOfuniformly: splicing only when the mutation is found, then deleting the bucket only after the splice leaves it empty. This is both correct and cleaner than the#scopespattern it mirrors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/mutationCache.ts` around lines 162 - 173, The removal logic in MutationCache that uses indexOf to find and splice the mutation (referenced by mutation.options.mutationKey, hashKey, and the `#byMutationKey` map) is correct and safer than the old keyed[0] guard; leave the current implementation as-is: find the index with keyed.indexOf(mutation), only splice when index !== -1, and delete the hash bucket only when keyed.length === 0 after splicing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/query-core/src/mutationCache.ts`:
- Around line 162-173: The removal logic in MutationCache that uses indexOf to
find and splice the mutation (referenced by mutation.options.mutationKey,
hashKey, and the `#byMutationKey` map) is correct and safer than the old keyed[0]
guard; leave the current implementation as-is: find the index with
keyed.indexOf(mutation), only splice when index !== -1, and delete the hash
bucket only when keyed.length === 0 after splicing.
1 participant
🎯 Changes
Remove unnecessary array allocations:
In
QueryCacheandMutationCachemethods such asfind,findAll,onFocus, andonOnline, we removed the intermediate arrays that were being created on every call viagetAll(), and instead iterated the internal collections directlyWhen using the filter combination
exact: true+queryKey/mutationKey, we now bypass full-cache scans and perform direct hash map lookupsQueryCache: since#queriesis already a hash → Query map, we directly access it viahashKey(queryKey)MutationCache: we maintain a separate index map#byMutationKeyand keep it synchronized onadd/remove/clear✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit