Skip to content

perf: match the bridge query by identity instead of re-hashing per cache entry - #157

Open
mbret wants to merge 5 commits into
mainfrom
chore/perf-2026-07-27-query-bridge-cache-scans
Open

perf: match the bridge query by identity instead of re-hashing per cache entry#157
mbret wants to merge 5 commits into
mainfrom
chore/perf-2026-07-27-query-bridge-cache-scans

Conversation

@mbret

@mbret mbret commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Target

Subsystem: queries — the Observable → TanStack Query bridge (createObservableQueryFn, QueryClient$).

User-visible symptom: main-thread work that grows with the size of the query cache, paid on the hottest loop the library has — a live stream bridged through useQuery$ triggers one refetch cycle per emission.

The mechanism

refetchQueries/cancelQueries with { queryKey, exact: true } are not expensive because they iterate the cache. They are expensive because matchQuery re-derives the hash for the filter key against every query in the cache (hashQueryKeyByOptions → key-sorted JSON.stringify), since each query may carry its own queryKeyHashFn.

So the fix is to stop hashing, not to stop calling refetchQueries. context.queryKey is the very array the target query holds (query.js:226), so an identity predicate is an exact match that hashes nothing:

queryClient?.refetchQueries({
  predicate: (query) => query.queryKey === context.queryKey,
})

All refetch semantics — notifyManager batching, the disabled/static skip, cancelRefetch, error swallowing — stay inside the query client instead of being reproduced here. The same change is applied to the cancel in QueryClient$.deleteQuery.

Measured impact

Benchmarked against the installed @tanstack/query-core 5.100.14, object-shaped keys (["entity", { id, scope }]), locating one query:

cached queries { queryKey, exact: true } identity predicate direct cache.get(hash)
50 65 µs 0.6 µs 20 ns
200 275 µs 2.6 µs 15 ns
1000 1518 µs 12.8 µs 18 ns

The predicate captures ~99% of the available win. Going the last step to a direct queryCache.get(queryHash) saves a further ~13 µs but requires hand-reproducing refetchQueries' per-query behavior — an earlier revision of this PR did exactly that and it cost two behavioral bugs (see below), so it was reverted in favor of the predicate.

History of this PR (what was tried and dropped)

The first revision replaced the call with queryCache.get(queryHash) + query.fetch(...). Two defects came out of duplicating those internals:

  1. Custom queryKeyHashFn queries were never refetched. TanStack stores a query under its configured hash; the bridge computed the default hashKey, so the lookup returned undefined and live streams published their first value and then stopped. Caught in review by @chatgpt-codex-connector.
  2. Missing notifyManager.batch. refetchQueries batches the observer notifications raised during the fetch into one React update; the bare query.fetch() notified per event, changing how renders coalesce.

The identity predicate makes both structurally impossible — nothing hashes, and nothing is reproduced — which is why it is the version being proposed.

Verification

  • npm run check (biome) — clean
  • npm run build (tsc + vite) — succeeds
  • npm run test:ci140/140, six consecutive full-suite runs

Includes a regression test (re-renders when the query uses a custom queryKeyHashFn) added while fixing defect 1; it still passes and now guards the property for free.

Note on CI: an earlier run failed on useQuery$.test.tsx "should return consecutive results". That test asserts on every intermediate render of an interval(5) and is flaky under CPU contention independently of this PR — under identical 6-way load, 12 runs each: clean main 1/12 failures, this branch 1/12. Worth a separate issue to make it deterministic.

One semantic difference to weigh

Identity matching will not match a query that was removed and rebuilt with a fresh queryKey array while the stream is still alive, where hash matching would. In practice the teardown path deletes the bridge entry (and stops the refetch loop) before that window opens, and the clear/invalidation/cleanup/unmount suites all pass — but it is a real difference, not a no-op.

Backlog (found but not taken)

  • Custom-hash entries are never torn down on observerRemoved — pre-existing on main. QueryClient$.queryMap is keyed by the default hashKey, but the cache subscription calls deleteQuery(event.query.queryHash) with TanStack's configured hash, so the two never match and the observable keeps running after the last observer unmounts. Measured: unmounting only the consumer leaves queryMap.size at 1 with a custom hash vs 0 with the default, identically on main. A behavior fix, not a perf one.
  • createObservableQueryFn computes hashKey(context.queryKey) and QueryClient$.setQuery immediately recomputes it on the cache-miss path — a duplicate stringify per query creation (per new query, not per emission).
  • The bridge's delay(1) + take(1) refetch loop costs two timers per emission by design; coalescing bursts would change delivery timing.
  • useObserve keeps its eager ObservableStore subscription alongside the useSyncExternalStore one, so each emission reaches one extra no-op subscriber.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FqgGygsYCHiwqmWqFj3wYm

refetchQueries/cancelQueries with { queryKey, exact: true } iterate every
query in the TanStack cache and re-hash the key against each entry. The
observable bridge runs the refetch once per emission of a live stream, so
the lookup is replaced with a direct queryCache.get(queryHash), and the
per-emission map lookup in the cache-entry subscription is replaced with
the closed-over entry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FqgGygsYCHiwqmWqFj3wYm

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f25d26b099

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib/queries/createObservableQueryFn.ts Outdated
claude added 3 commits July 27, 2026 12:28
TanStack stores a query under the hash produced by its configured
queryKeyHashFn, while the bridge computes the default hashKey, so the
direct queryCache.get lookup missed those queries entirely: live streams
published their first value and were never refetched again, and the
teardown cancel was skipped.

Keep the O(1) lookup for the default hashing case and fall back to
refetchQueries/cancelQueries when it misses, rather than re-deriving the
configured hash here.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FqgGygsYCHiwqmWqFj3wYm
refetchQueries and cancelQueries wrap their work in notifyManager.batch,
which queues the observer notifications raised during the call and
flushes them in a single React batched update. Calling query.fetch and
query.cancel bare notified per event instead, changing how renders
coalesce for live streams.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FqgGygsYCHiwqmWqFj3wYm

mbret commented Jul 27, 2026

Copy link
Copy Markdown
Owner Author

Update after the CI failure on a320846 — merged main in, plus one more real fix found while investigating.

The CI failure is a pre-existing flaky test, not this PR

CI failed on useQuery$.test.tsx:118 ("should return consecutive results"), expecting data: 2 and receiving data: 3. That test drives an interval(5) and asserts on every intermediate render in order — its own comment says "interval big enough so react does not skip some render" — so under CPU contention a value gets coalesced away.

It does not reproduce unloaded (0/10 isolated runs, 0/3 full-suite runs on this branch). Under identical 6-way CPU load, 12 runs each:

variant failures
clean main (60a083b, no perf change at all) 1/12
this branch, without the batching fix below 2/12
this branch, with the batching fix 1/12

main flakes at the same rate with none of my code, so the failure isn't attributable to this PR. Worth a separate issue to make that test deterministic (assert the final value and monotonicity rather than exact per-render snapshots).

But the investigation did surface a real parity bug (cfc1900)

Chasing whether I could have caused it, I found a genuine semantic difference I'd missed: refetchQueries and cancelQueries wrap their work in notifyManager.batch(...), and my direct query.fetch() / query.cancel() did not. That is not cosmetic — batch() increments transactions, so notifications raised during the call are queued and flushed together through React's batched-update function, whereas the bare call schedules each notification individually. For live streams that changes how renders coalesce.

Both call sites are now wrapped, matching upstream exactly. My earlier claim that the change was "invisible except in speed" was wrong before this commit — thanks to the CI red for prompting the check, even though it turned out to be flaky for unrelated reasons.

State

main merged in (bec7317). Gates green locally: biome clean, build succeeds, 140/140 tests.


Generated by Claude Code

The cost of refetchQueries/cancelQueries with { queryKey, exact: true }
is not the iteration but the re-hashing: matchQuery JSON.stringifies the
key again for every query in the cache, since each one may carry its own
queryKeyHashFn. An identity predicate skips hashing entirely, as
context.queryKey is the very array the target query holds.

This replaces the direct queryCache.get + query.fetch path. Batching,
the disabled/static skip, cancelRefetch and error swallowing all go back
to living inside the query client rather than being duplicated here, so
the custom queryKeyHashFn fallback and the notifyManager.batch
replication are no longer needed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FqgGygsYCHiwqmWqFj3wYm
@mbret mbret changed the title perf: locate bridge queries by hash instead of scanning the query cache perf: match the bridge query by identity instead of re-hashing per cache entry Jul 27, 2026
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.

2 participants