Skip to content

perf(normalizr): store consumer-facing journey at write time#3928

Merged
ntucker merged 1 commit intofix/journey-mutation-on-result-cache-hit-scalarfrom
cursor/journey-at-write-5fd9
Apr 28, 2026
Merged

perf(normalizr): store consumer-facing journey at write time#3928
ntucker merged 1 commit intofix/journey-mutation-on-result-cache-hit-scalarfrom
cursor/journey-at-write-5fd9

Conversation

@ntucker
Copy link
Copy Markdown
Collaborator

@ntucker ntucker commented Apr 28, 2026

Stacks on top of #3925.

Summary

Move the per-hit paths.slice(1) (and the hasStringDeps filter loop) out of GlobalCache.getResults and into the cache write. 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.

@@ 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

paths is 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 the does not mutate cached journey across repeated result-cache hits test. The two consumers — entityExpiresAt(paths, …) and GCPolicy.createCountRef({key, paths}) — are both read-only for…of loops.

If a later set() overwrites the same Link, curLink.journey = journey assigns a new reference; old captures still see the old array.

Tests

yarn jest --selectProjects ReactDOM --testPathPatterns "packages/(normalizr|core|endpoint)"
Test Suites: 37 passed, 37 total
Tests:       837 passed, 837 total

yarn jest --selectProjects ReactDOM --testPathPatterns "packages/react/.*useSuspense|useCache|useQuery"
Test Suites: 5 passed, 5 total
Tests:       62 passed, 62 total

Performance — read path (cache-hit benchmarks, n=5 each, trimmed mean)

OPT vs PR is the actual delta from the optimization. OPT vs master shown for context (master here being the buggy paths.shift() baseline that was draining the journey, not real work).

Benchmark PR (this branch's parent) OPT (this branch) Δ% (OPT vs PR) Δ% (OPT vs master)
denormalizeLong withCache 11,483 13,291 +15.75% +6.86%
denormalizeLong withCache (Scalar churn) 11,495 13,277 +15.50% +6.67%
denormalizeLong All withCache 9,742 11,907 +22.23% +18.04%
denormalizeLong Query-sorted withCache 9,710 12,313 +26.81% +15.77%
denormalizeLong Scalar withCache (hasStringDeps) 12,315 13,965 +13.40% +0.03%
denormalizeLong Scalar update withCache (hasStringDeps) 6,978 7,549 +8.19% -0.67%
denormalizeLong Values withCache 8,718 9,064 +3.97% +2.10%
denormalizeLongAndShort withEntityCacheOnly 2,911 3,075 +5.63% +3.22%
query All withCache 9,269 12,277 +32.46% +16.38%
queryShort 500x withCache 5,337 5,504 +3.14% -1.73%
denormalizeShort 500x withCache 13,417 13,217 -1.49% +0.46%

Non-cached/donotcache benchmarks unchanged (within noise):

Benchmark PR OPT Δ%
denormalizeLong donotcache 1,851 1,856 +0.25%
denormalizeLong Scalar donotcache 1,838 1,790 -2.59%
denormalizeShort donotcache 500x 2,624 2,646 +0.83%
normalizeLong 782 780 -0.26%
buildQueryKey All 88,520 89,041 +0.59%

Performance — write path

The write path now skips dependencies.map(d => d.path) inside WeakDependencyMap.set; the caller (GlobalCache.getResults miss branch) was already computing paths = 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):

Benchmark PR OPT Δ% (OPT vs PR)
WRITE-only: ProjectSchema (1 miss) 712 700 -1.64%
WRITE-only: AllProjects (1 miss) 711 708 -0.33%
WRITE-only: StockSchema (1 miss, hasStringDeps) 656 678 +3.30%

Net write impact is within run-to-run noise (1–3%) on entity-only schemas and a small win on hasStringDeps (the second dependencies.map allocation is now skipped and the redundant _hasArgsKey filter pass at write time becomes the only walk).

End-to-end suite benchmarks confirm: ^normalize and ^denormalize.*donotcache benchmarks all within ±1%; ^set* core benchmarks (which don’t go through MemoCache.set) within thermal noise; cache-hit benchmarks show the full read-path win.

Drain-free fair microbench

(One fresh MemoCache per iter → one miss + one hit. Isolates the per-hit cost without letting any branch's bug accumulate state across iterations.)

Benchmark PR OPT Δ%
ProjectSchema fresh-prime + 1 hit 639 650 +1.75%
AllProjects fresh-prime + 1 hit 648 676 +4.27%
StockSchema fresh-prime + 1 hit (hasStringDeps) 608 635 +4.44%

Bundlesize

packages/normalizr/src/memo/globalCache.ts shrinks: the entire if (hasStringDeps) { … filter … } else { paths.slice(1) } block is removed (–14 LOC). WeakDependencyMap.ts adds 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 normalizr suite + 3 runs of full core suite 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.

Open in Web Open in Cursor 

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.
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 28, 2026

⚠️ No Changeset found

Latest commit: f049276

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 28, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs-site Ignored Ignored Apr 28, 2026 11:27am

Request Review

@ntucker ntucker marked this pull request as ready for review April 28, 2026 11:40
@ntucker ntucker merged commit 5a63f74 into fix/journey-mutation-on-result-cache-hit-scalar Apr 28, 2026
21 checks passed
@ntucker ntucker deleted the cursor/journey-at-write-5fd9 branch April 28, 2026 12:04
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().
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