perf(normalizr): store consumer-facing journey at write time#3928
Merged
ntucker merged 1 commit intofix/journey-mutation-on-result-cache-hit-scalarfrom Apr 28, 2026
Merged
Conversation
Move the per-hit paths.slice(1) (and the hasStringDeps filter loop) out of GlobalCache.getResults and into the cache write. GlobalCache.paths() already produces the placeholder-free, function-free shape every consumer needs. Hand it to WeakDependencyMap.set as the journey, and the cache-hit branch can return that array by reference - no per-hit allocation, no per-hit typeof === 'function' walk. Safety: paths is now a shared reference held by every subsequent hit. The contract that consumers must not mutate it was established by the journey-mutation fix (PR #3925) and is exercised by the existing globalCache.test.ts regression test.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
5a63f74
into
fix/journey-mutation-on-result-cache-hit-scalar
21 checks passed
4 tasks
ntucker
added a commit
that referenced
this pull request
Apr 28, 2026
* chore: rebase PR #3925 onto latest master * pkg: Bump peerdeps of @data-client/react to support 0.17 (#3927) * pkg: Bump peerdeps of @data-client/react to support 0.17 * pkg: Update yarn.lock for peerdep bump * perf(normalizr): store consumer-facing journey at write time (#3928) Move the per-hit paths.slice(1) (and the hasStringDeps filter loop) out of GlobalCache.getResults and into the cache write. GlobalCache.paths() already produces the placeholder-free, function-free shape every consumer needs. Hand it to WeakDependencyMap.set as the journey, and the cache-hit branch can return that array by reference - no per-hit allocation, no per-hit typeof === 'function' walk. Safety: paths is now a shared reference held by every subsequent hit. The contract that consumers must not mutate it was established by the journey-mutation fix (PR #3925) and is exercised by the existing globalCache.test.ts regression test. * fix: Scalar reversion * test(core): add getResponseMeta-paths integration test (non-GC angle) (#3929) * test(core): replace getResponseMeta-countRef with getResponseMeta-paths The previous integration test poked GCPolicy['entityCount'] directly to prove the journey-mutation bug. Replace it with a test that asserts the public-API consequence the bug creates outside of GC: every subscriber to the same endpoint must observe the same expiresAt from Controller.getResponseMeta(). This is the property the bug actually broke for non-GC users: with the buggy paths.shift(), entityExpiresAt(paths, …) iterates a progressively-shorter list, dropping the entity with the earliest expiry first. Subscriber 2 observes a too-late expiresAt; subscriber 3+ observe Infinity and never refetch. Fires under ImmortalGCPolicy too, since entityExpiresAt is unconditional whenever the endpoint has no top-level meta.expiresAt — typical for state populated via controller.set(Entity, …), SSR hydration, or useQuery. Verified the assertion fails on the buggy paths.shift() (m3.expiresAt returns FOO_2_EXPIRY instead of FOO_1_EXPIRY) and passes on the fix. * test(core): keep existing countRef integration test alongside paths test Restore the GC-side getResponseMeta-countRef.ts integration test that the prior commit replaced. The two tests cover the journey-mutation bug from complementary angles: - getResponseMeta-countRef.ts: GC consumer of paths (entityCount under-counting → premature reaping under default GCPolicy). - getResponseMeta-paths.ts: non-GC consumer of paths (entityExpiresAt → suppressed entity-expiry refetch; fires under ImmortalGCPolicy too). Both pass on the fix; both fail on the buggy paths.shift().
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.
Stacks on top of #3925.
Summary
Move the per-hit
paths.slice(1)(and thehasStringDepsfilter loop) out ofGlobalCache.getResultsand into the cache write.paths()already produces the placeholder-free, function-free shape every consumer needs; hand it toWeakDependencyMap.setas the journey, and the cache-hit branch can return that array by reference.@@ packages/normalizr/src/memo/WeakDependencyMap.ts set( dependencies: Dep<Path, K>[], value: V, args: readonly any[] = [], + /** Optional consumer-facing journey returned to `get()` callers verbatim. + * Defaults to dependencies.map(d => d.path). The array becomes a shared + * reference held by every subsequent cache hit; callers MUST NOT mutate. */ + journey?: Path[], ) { … - curLink.journey = dependencies.map(d => d.path) as Path[]; + curLink.journey = journey ?? (dependencies.map(d => d.path) as Path[]); }@@ packages/normalizr/src/memo/globalCache.ts (getResults) if (paths === undefined) { data = computeValue(); paths = this.paths(); this.dependencies[0] = { path: { key: '', pk: '' }, entity: input }; - this._resultCache.set(this.dependencies, data, this._args); - } else { - if (this._resultCache.hasStringDeps) { - const filtered: EntityPath[] = []; - for (let i = 1; i < paths.length; i++) { - const p = paths[i]; - if (typeof p !== 'function') filtered.push(p); - } - paths = filtered; - } else { - paths = paths.slice(1); - } + this._resultCache.set(this.dependencies, data, this._args, paths); } + // hit branch: `paths` aliases the stored Link.journey — return as-is. return { data, paths }; }Correctness
pathsis now a shared reference held by every subsequent cache hit. The contract that consumers must not mutate it was established by #3925 and is exercised by thedoes not mutate cached journey across repeated result-cache hitstest. The two consumers —entityExpiresAt(paths, …)andGCPolicy.createCountRef({key, paths})— are both read-onlyfor…ofloops.If a later
set()overwrites the same Link,curLink.journey = journeyassigns a new reference; old captures still see the old array.Tests
Performance — read path (cache-hit benchmarks, n=5 each, trimmed mean)
OPT vs PRis the actual delta from the optimization.OPT vs mastershown for context (master here being the buggypaths.shift()baseline that was draining the journey, not real work).denormalizeLong withCachedenormalizeLong withCache (Scalar churn)denormalizeLong All withCachedenormalizeLong Query-sorted withCachedenormalizeLong Scalar withCache(hasStringDeps)denormalizeLong Scalar update withCache(hasStringDeps)denormalizeLong Values withCachedenormalizeLongAndShort withEntityCacheOnlyquery All withCachequeryShort 500x withCachedenormalizeShort 500x withCacheNon-cached/
donotcachebenchmarks unchanged (within noise):denormalizeLong donotcachedenormalizeLong Scalar donotcachedenormalizeShort donotcache 500xnormalizeLongbuildQueryKey AllPerformance — write path
The write path now skips
dependencies.map(d => d.path)insideWeakDependencyMap.set; the caller (GlobalCache.getResultsmiss branch) was already computingpaths = this.paths(), so the net change to write-path work is one fewer N-element array allocation per cache miss.Pure write-only microbench (one fresh
MemoCache, one denormalize call per iteration, n=5, trimmed mean):WRITE-only: ProjectSchema (1 miss)WRITE-only: AllProjects (1 miss)WRITE-only: StockSchema (1 miss, hasStringDeps)Net write impact is within run-to-run noise (1–3%) on entity-only schemas and a small win on
hasStringDeps(the seconddependencies.mapallocation is now skipped and the redundant_hasArgsKeyfilter pass at write time becomes the only walk).End-to-end suite benchmarks confirm:
^normalizeand^denormalize.*donotcachebenchmarks all within ±1%;^set*core benchmarks (which don’t go throughMemoCache.set) within thermal noise; cache-hit benchmarks show the full read-path win.Drain-free fair microbench
(One fresh
MemoCacheper iter → one miss + one hit. Isolates the per-hit cost without letting any branch's bug accumulate state across iterations.)ProjectSchema fresh-prime + 1 hitAllProjects fresh-prime + 1 hitStockSchema fresh-prime + 1 hit (hasStringDeps)Bundlesize
packages/normalizr/src/memo/globalCache.tsshrinks: the entireif (hasStringDeps) { … filter … } else { paths.slice(1) }block is removed (–14 LOC).WeakDependencyMap.tsadds 1 optional parameter (+12 LOC of TS, mostly the JSDoc explaining the no-mutate contract). Net source change: –2 LOC, ~–80 bytes minified.Methodology
5 runs of full
normalizrsuite + 3 runs of fullcoresuite per branch + 5 runs each of two focused microbenches (write-only, drain-free fair). Trimmed mean (drop min/max for n=5). All runs back-to-back on the same VM, same Node 24.14.1.