From ae94df89d0a210ba1672e2121f7c17e6bea38a04 Mon Sep 17 00:00:00 2001 From: Sheraff Date: Wed, 10 Jun 2026 23:00:14 +0200 Subject: [PATCH 1/5] perf(react-router): add match selector compares --- packages/react-router/src/Match.tsx | 44 +++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/packages/react-router/src/Match.tsx b/packages/react-router/src/Match.tsx index fefd256bf6..f5cffd5b4d 100644 --- a/packages/react-router/src/Match.tsx +++ b/packages/react-router/src/Match.tsx @@ -20,7 +20,24 @@ import { renderRouteNotFound } from './renderRouteNotFound' import { ScrollRestoration } from './scroll-restoration' import { ClientOnly } from './ClientOnly' import { useLayoutEffect } from './utils' -import type { AnyRoute, RootRouteOptions } from '@tanstack/router-core' +import type { + AnyRoute, + AnyRouteMatch, + RootRouteOptions, +} from '@tanstack/router-core' + +type OutletMatchSelection = [ + routeId: string | undefined, + parentGlobalNotFound: boolean, +] + +const matchViewFieldsEqual = (a: AnyRouteMatch, b: AnyRouteMatch) => + a.routeId === b.routeId && a._displayPending === b._displayPending + +const outletMatchSelectionEqual = ( + a: OutletMatchSelection, + b: OutletMatchSelection, +) => a[0] === b[0] && a[1] === b[1] export const Match = React.memo(function MatchImpl({ matchId, @@ -77,7 +94,7 @@ export const Match = React.memo(function MatchImpl({ // eslint-disable-next-line react-hooks/rules-of-hooks const resetKey = useStore(router.stores.loadedAt, (loadedAt) => loadedAt) // eslint-disable-next-line react-hooks/rules-of-hooks - const match = useStore(matchStore, (value) => value) + const match = useStore(matchStore, (value) => value, matchViewFieldsEqual) // eslint-disable-next-line react-hooks/rules-of-hooks const matchState = React.useMemo(() => { const routeId = match.routeId as string @@ -208,7 +225,7 @@ function MatchView({ {matchState.parentRouteId === rootRouteId ? ( <> - + {router.options.scrollRestoration && (isServer ?? router.isServer) ? ( ) : null} @@ -222,7 +239,7 @@ function MatchView({ // the route subtree has committed below the root layout. Keeping it here lets // us fire onRendered even after a hydration mismatch above the root layout // (like bad head/link tags, which is common). -function OnRendered({ resetKey }: { resetKey: number }) { +function OnRendered() { const router = useRouter() if (isServer ?? router.isServer) { @@ -231,6 +248,11 @@ function OnRendered({ resetKey }: { resetKey: number }) { // eslint-disable-next-line react-hooks/rules-of-hooks const prevHrefRef = React.useRef(undefined) + // eslint-disable-next-line react-hooks/rules-of-hooks + const renderedLocationKey = useStore( + router.stores.resolvedLocation, + (resolvedLocation) => resolvedLocation?.state.__TSR_key, + ) // eslint-disable-next-line react-hooks/rules-of-hooks useLayoutEffect(() => { @@ -249,7 +271,7 @@ function OnRendered({ resetKey }: { resetKey: number }) { }) prevHrefRef.current = currentHref } - }, [router.latestLocation.state.__TSR_key, resetKey, router]) + }, [renderedLocationKey, router]) return null } @@ -521,10 +543,14 @@ export const Outlet = React.memo(function OutletImpl() { : undefined // eslint-disable-next-line react-hooks/rules-of-hooks - ;[routeId, parentGlobalNotFound] = useStore(parentMatchStore, (match) => [ - match?.routeId as string | undefined, - match?.globalNotFound ?? false, - ]) + ;[routeId, parentGlobalNotFound] = useStore( + parentMatchStore, + (match): OutletMatchSelection => [ + match?.routeId as string | undefined, + match?.globalNotFound ?? false, + ], + outletMatchSelectionEqual, + ) // eslint-disable-next-line react-hooks/rules-of-hooks childMatchId = useStore(router.stores.matchesId, (ids) => { From 620e9674fa8735aa3e261312b60aa80cdd6035fa Mon Sep 17 00:00:00 2001 From: Sheraff Date: Thu, 11 Jun 2026 00:04:06 +0200 Subject: [PATCH 2/5] fix scroll restoration --- packages/react-router/src/Match.tsx | 23 +++- ...earch-params-json-parse-exception-storm.md | 71 ++++++++++++ .../002-match-routes-lightweight-memo.md | 67 +++++++++++ .../003-build-middleware-chain-cache.md | 56 +++++++++ .../004-load-matches-sync-fastpaths.md | 49 ++++++++ .../005-loaderless-runloader-fast-path.md | 50 ++++++++ .../006-update-match-write-coalescing.md | 48 ++++++++ .../007-transitioner-transition-refcount.md | 52 +++++++++ .../008-lazy-controlled-promise.md | 44 +++++++ ...009-parse-location-reuse-built-location.md | 58 ++++++++++ .../010-hook-selector-slice-memoization.md | 49 ++++++++ .../011-use-match-invariant-throw.md | 59 ++++++++++ .../012-outlet-match-selector-compares.md | 47 ++++++++ .../013-link-options-memo-stability.md | 41 +++++++ .../014-link-isactive-client-prechecks.md | 44 +++++++ .../015-link-flushsync-click.md | 56 +++++++++ .../016-link-render-granularity.md | 65 +++++++++++ .../017-interpolate-path-template-cache.md | 38 ++++++ .../018-match-routes-internal-overheads.md | 48 ++++++++ .../019-match-identity-reuse.md | 41 +++++++ .../020-build-match-context-incremental.md | 40 +++++++ .../021-commit-cached-pool-single-pass.md | 47 ++++++++ .../022-commit-path-micro-allocations.md | 43 +++++++ .../023-ssr-router-update-three-times.md | 45 ++++++++ .../024-ssr-handler-hoisting.md | 42 +++++++ .../025-ssr-html-boundary-scan.md | 39 +++++++ .../026-ssr-trie-allocations.md | 38 ++++++ perf-investigations/README.md | 109 ++++++++++++++++++ 28 files changed, 1403 insertions(+), 6 deletions(-) create mode 100644 perf-investigations/001-search-params-json-parse-exception-storm.md create mode 100644 perf-investigations/002-match-routes-lightweight-memo.md create mode 100644 perf-investigations/003-build-middleware-chain-cache.md create mode 100644 perf-investigations/004-load-matches-sync-fastpaths.md create mode 100644 perf-investigations/005-loaderless-runloader-fast-path.md create mode 100644 perf-investigations/006-update-match-write-coalescing.md create mode 100644 perf-investigations/007-transitioner-transition-refcount.md create mode 100644 perf-investigations/008-lazy-controlled-promise.md create mode 100644 perf-investigations/009-parse-location-reuse-built-location.md create mode 100644 perf-investigations/010-hook-selector-slice-memoization.md create mode 100644 perf-investigations/011-use-match-invariant-throw.md create mode 100644 perf-investigations/012-outlet-match-selector-compares.md create mode 100644 perf-investigations/013-link-options-memo-stability.md create mode 100644 perf-investigations/014-link-isactive-client-prechecks.md create mode 100644 perf-investigations/015-link-flushsync-click.md create mode 100644 perf-investigations/016-link-render-granularity.md create mode 100644 perf-investigations/017-interpolate-path-template-cache.md create mode 100644 perf-investigations/018-match-routes-internal-overheads.md create mode 100644 perf-investigations/019-match-identity-reuse.md create mode 100644 perf-investigations/020-build-match-context-incremental.md create mode 100644 perf-investigations/021-commit-cached-pool-single-pass.md create mode 100644 perf-investigations/022-commit-path-micro-allocations.md create mode 100644 perf-investigations/023-ssr-router-update-three-times.md create mode 100644 perf-investigations/024-ssr-handler-hoisting.md create mode 100644 perf-investigations/025-ssr-html-boundary-scan.md create mode 100644 perf-investigations/026-ssr-trie-allocations.md create mode 100644 perf-investigations/README.md diff --git a/packages/react-router/src/Match.tsx b/packages/react-router/src/Match.tsx index f5cffd5b4d..acecde9043 100644 --- a/packages/react-router/src/Match.tsx +++ b/packages/react-router/src/Match.tsx @@ -23,6 +23,7 @@ import { useLayoutEffect } from './utils' import type { AnyRoute, AnyRouteMatch, + ParsedLocation, RootRouteOptions, } from '@tanstack/router-core' @@ -247,7 +248,15 @@ function OnRendered() { } // eslint-disable-next-line react-hooks/rules-of-hooks - const prevHrefRef = React.useRef(undefined) + // @ts-expect-error -- init to `undefined` but don't write `undefined` to shave bytes + // Track the resolvedLocation as of the last render so that onRendered can + // report the correct fromLocation. By the time this effect fires, + // resolvedLocation has already been updated to the new location by + // Transitioner, so we cannot use router.stores.resolvedLocation.get() + // directly as the fromLocation. + const prevResolvedLocationRef = React.useRef< + ParsedLocation | undefined + >() // eslint-disable-next-line react-hooks/rules-of-hooks const renderedLocationKey = useStore( router.stores.resolvedLocation, @@ -256,21 +265,23 @@ function OnRendered() { // eslint-disable-next-line react-hooks/rules-of-hooks useLayoutEffect(() => { - const currentHref = router.latestLocation.href + const currentResolvedLocation = router.stores.resolvedLocation.get() + const previousResolvedLocation = prevResolvedLocationRef.current if ( - prevHrefRef.current === undefined || - prevHrefRef.current !== currentHref + currentResolvedLocation && + (!previousResolvedLocation || + previousResolvedLocation.href !== currentResolvedLocation.href) ) { router.emit({ type: 'onRendered', ...getLocationChangeInfo( router.stores.location.get(), - router.stores.resolvedLocation.get(), + previousResolvedLocation ?? currentResolvedLocation, ), }) - prevHrefRef.current = currentHref } + prevResolvedLocationRef.current = currentResolvedLocation }, [renderedLocationKey, router]) return null diff --git a/perf-investigations/001-search-params-json-parse-exception-storm.md b/perf-investigations/001-search-params-json-parse-exception-storm.md new file mode 100644 index 0000000000..778ecce11c --- /dev/null +++ b/perf-investigations/001-search-params-json-parse-exception-storm.md @@ -0,0 +1,71 @@ +# 001 — search-params JSON.parse exception storm + +| | | +| --------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/searchParams.ts` (shared client+server) | +| Benchmarks | ssr (react) **and** client-nav (react) | +| Expected impact | **Large** — validated −42% SSR request time; `stringifyValue` is 5.7% self-time of the client-nav profile and the largest single router-core frame | +| Confidence | High (CPU-profiled on both benchmarks; SSR fix validated experimentally with byte-identical HTML output) | +| Risk | Low | +| Prior art | `origin/flo/search-params-json-parse-guard` (**most recent**, includes review feedback + tests), `origin/refactor-router-core-stringify-parse-search-json-perf` (older alternative) — **inspect/rebase these before writing new code** | + +## Problem + +The default search serializers use "try to JSON.parse it" as a type probe for every string value: + +`searchParams.ts:63-81` (`stringifyValue` inside `stringifySearchWith`): + +```ts +} else if (hasParser && typeof val === 'string') { + try { + // Check if it's a valid parseable string. + // If it is, then stringify it again. + parser(val) + return stringify(val) + } catch (_err) { + // silent + } +} +``` + +`parseSearchWith` (`searchParams.ts:~31-40`) has the symmetric `try { query[key] = parser(value) } catch {}` per string value. + +`parser` is `JSON.parse`. For any ordinary string (`'all'`, `'group-0'`, `'updater-2'`, `'q-abc123'`), **a real SyntaxError is constructed, thrown, and swallowed**. V8 error construction + stack capture ≈ 2.5 µs vs ~43 ns for a successful parse vs ~18 ns for a regex pre-check (micro-benchmarked). + +## Why it's hot + +- `stringifySearch` runs once per `buildLocation` (`router.ts:2009`). Every mounted `` calls `buildLocation` when the location changes (`react-router/src/link.tsx:410-413`), so the client-nav benchmark (≈22 links) does ~23 stringify passes per navigation, each throwing for every plain-string param. `parseSearch` adds a parse+stringify round trip per `parseLocation` (`router.ts:1309-1310`). +- On the server, each SSR request renders all Links → measured **35.0% self time** for `stringifyValue` in the SSR benchmark profile (1140 ms of 3.26 s over 3000 requests). +- Thrown `Error` objects also feed GC (8.6% of client profile). + +## Validated result + +An ESM-loader patch applying the pre-check to the built SSR bench: **0.957 → 0.557 ms/request (−42%)**, HTML byte-identical. Client micro-bench of `encode()` on the benchmark's search shape: 714 ms → 20.6 ms per 100k calls (~35×). + +## Proposed approach + +Add a cheap first-character pre-check before attempting `JSON.parse`. A string can only be valid JSON if its first non-whitespace char is one of `" { [ - 0-9 t f n` (true/false/null): + +1. In `stringifyValue`: skip the `parser(val)` probe (and return `val` directly) when the pre-check fails. +2. In `parseSearchWith`'s value loop: keep the string as-is when the pre-check fails. +3. Strings that _pass_ the pre-check but are still invalid JSON (`'1abc'`, `'true123'`) keep the existing try/catch — behavior is byte-identical. +4. **Scope the fast path to the default JSON implementations** (build it into `defaultParseSearch`/`defaultStringifySearch`, or gate on `parser === JSON.parse`) so custom serializers (JSURL, zipson…) keep exact semantics. + +Note: leading whitespace must count as "could be JSON" (`JSON.parse(' 1')` succeeds), so test the first _non-whitespace_ char or include `\s` in the class: `/^[\s"{[\-\dtfn]/`. + +## Risks & constraints + +- Pre-check must be a strict **superset** of valid JSON starts — never reject a parseable string. +- ~40-60 bytes added to shared client code; CI gzip check applies. This is the one finding where a tiny bundle increase is clearly worth it (it also cuts client CPU), but run the bundle benchmark and report the delta. +- An optional second step explored during the audit — replacing `URLSearchParams` in `qss.ts encode` with manual `encodeURIComponent` concatenation — changes space encoding (`+` vs `%20`) and should be treated as a separate, lower-priority investigation. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit # searchParams.test.ts has coverage; the prior-art branch adds ~75 lines of tests +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Expect a large, unambiguous SSR win and a visible client-nav win. The SSR bench must run with `NODE_ENV=production`. diff --git a/perf-investigations/002-match-routes-lightweight-memo.md b/perf-investigations/002-match-routes-lightweight-memo.md new file mode 100644 index 0000000000..4a6706dd33 --- /dev/null +++ b/perf-investigations/002-match-routes-lightweight-memo.md @@ -0,0 +1,67 @@ +# 002 — memoize `matchRoutesLightweight` per location + +| | | +| --------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/router.ts` (shared client+server) | +| Benchmarks | client-nav (react) **and** ssr (react) | +| Expected impact | **Large** (client: removes ~22/23 of per-navigation match/validate/param work; SSR: validated additional −7% on top of 001) | +| Confidence | High (profiled; SSR variant validated experimentally with byte-identical output) | +| Risk | Medium-low | +| Prior art | `origin/flo/current-location-lightweight-cache` (commits `b745f7d39f` + fix `b845204fa7`) — same idea, stale vs current main; **rebase it rather than re-implementing**. Local branch `flo/router-current-location-cache-guard` may contain related guard work. | + +## Problem + +Every `buildLocation` call starts by re-deriving the _current_ location's matches: + +- `router.ts:1829-1841` — `build()` does `const lightweightResult = this.matchRoutesLightweight(currentLocation)`. +- `router.ts:1722-1789` — `matchRoutesLightweight` runs: + - `this.getMatchedRoutes(location.pathname)` (LRU-cached match, but still `trimPathRight` + `Object.create(null)` + `Object.assign(routeParams, match.rawParams)` per call, `router.ts:3092-3100`), + - `const accumulatedSearch = { ...location.search }` plus the **user `validateSearch` for every matched route** (`router.ts:1743-1753`), + - a params-reuse check against `stores.matchesId`/match stores, or a fresh `extractStrictParams` chain re-running user `params.parse` (`router.ts:1756-1781`). + +Every mounted `` subscribes to `router.stores.location` with `prev.href === next.href` equality (`react-router/src/link.tsx:403-407`) and recomputes `router.buildLocation({ _fromLocation: currentLocation, ..._options })` when it changes (`link.tsx:410-413`). All links pass the **same** location object (the store value), and `router.navigate` itself uses `this.latestLocation`. + +## Why it's hot + +- client-nav: ~22 links + 1 navigate = **~23 byte-identical `matchRoutesLightweight` computations per navigation** (~230 per benchmark iteration). The user's `validateSearch` runs ~23× per navigation for the current location alone — in real apps this is often a zod schema, so the real-world cost is much higher than the benchmark shows. +- Profile share (client): `build` 61.8 ms + `matchRoutesLightweight` 40.1 ms + `getMatchedRoutes` 27.2 ms in a 5.4 s profile, plus a large share of the 8.6% GC. +- SSR: 10 Links per request → 10 identical computations; `getMatchedRoutes` alone was 4.7% self post-001. Validated patch: **0.557 → 0.516 ms/request**. + +## Proposed approach + +Single-entry memo on the router instance keyed by reference equality: + +```ts +private _lightweightCache?: { + location: ParsedLocation + lastMatchId: string | undefined // last(this.stores.matchesId.get()) + result: LightweightResult +} +``` + +- Hit when `location` is reference-equal **and** `last(this.stores.matchesId.get())` is unchanged (the result's `canReuseParams` branch at `router.ts:1756-1781` reads match-store state, so the key must cover it). +- Location objects are immutable and fresh per commit (`parseLocation` creates a new object), so reference identity is a sound key. +- Sharing the result object across callers is safe today: `build()` defensively copies params before mutating (`router.ts:1876-1879` `Object.assign(Object.create(null), lightweightResult.params)`) and treats `fromSearch` as read-only (`nullReplaceEqualDeep` input at `router.ts:2006`). Re-verify this when rebasing. +- Invalidate (or simply rely on key mismatch) when the route tree is swapped (`update()`/HMR). + +The prior-art branch additionally replaced the returned `search` with `currentLocation.search` — verify that's still correct after the retained-search fixes that landed later on main (`df1076c03a`, `7a83e67e65`); the follow-up commit `b845204fa7` on that branch ("preserve validated search in buildLocation") addresses exactly this, so read it. + +## Risks & constraints + +- Impure user `validateSearch` would now run once per navigation instead of N times — arguably a fix, but observable. +- Stale-cache bugs surface as wrong relative-link targets; cover with the existing link/buildLocation test suites. +- Bundle delta ≈ +10-15 lines in router-core; partially offset by deleting the dead commented-out block at `router.ts:1733-1740`. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +## Related + +- Composes with [003](003-build-middleware-chain-cache.md) (the other repeated block inside `buildLocation`) and [001](001-search-params-json-parse-exception-storm.md). +- [016](016-link-render-granularity.md) reduces how often links recompute at all. diff --git a/perf-investigations/003-build-middleware-chain-cache.md b/perf-investigations/003-build-middleware-chain-cache.md new file mode 100644 index 0000000000..7dc5c05391 --- /dev/null +++ b/perf-investigations/003-build-middleware-chain-cache.md @@ -0,0 +1,56 @@ +# 003 — cache `buildMiddlewareChain` per route branch + +| | | +| --------------- | -------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/router.ts` (shared client+server) | +| Benchmarks | client-nav (react), ssr (react) | +| Expected impact | Medium (allocation/GC churn removal on a ×23-per-navigation path; multiplies with 002) | +| Confidence | High — the code carries an explicit TODO asking for exactly this | +| Risk | Low-medium (re-entrancy detail) | +| Prior art | none found | + +## Problem + +Every `buildLocation` call rebuilds the entire search-middleware closure chain: + +- `router.ts:3111-3124` — `applySearchMiddleware` calls `buildMiddlewareChain(destRoutes)` unconditionally. +- `router.ts:3126-3230` — `buildMiddlewareChain` loops over all destRoutes, reading `route.options` (`'search' in routeOptions`, `preSearchFilters`, `validateSearch`) and allocating a `middlewares` array plus fresh `validate`/`legacyMiddleware`/`applyNext`/`next` closures per call — even for links with zero middleware. + +The code already anticipates the fix (`router.ts:3107-3110`): + +``` +TODO: once caches are persisted across requests on the server, +we can cache the built middleware chain using `last(destRoutes)` as the key +``` + +That precondition is now met: router caches **are** persisted cross-request via `globalThis.__TSR_CACHE__` (`router.ts:1121-1149`). + +The returned chain is already written to be reusable — it is parameterized per invocation via `middleware(search, dest, includeValidateSearch)` with mutable captured vars assigned at `router.ts:3221-3229`. + +## Why it's hot + +Runs inside every `buildLocation`: ~23×/navigation on client-nav (22 links + navigate), once per Link per SSR request. Pure repeated allocation; user code isn't the cost, the closure construction is. SSR profile: ~0.4% self + GC share. + +## Proposed approach + +1. Cache the built chain in a `WeakMap` keyed by **branch array identity** (`WeakMap, chain>`). `destRoutes` identity is stable in the hot paths: `getRouteBranch` memoizes branches (`router.ts:1386-1393`) and `findRouteMatch` caches `result.branch` (`new-process-route-tree.ts:755`). The notFound case spreads a fresh array (`router.ts:1923`) and will simply rebuild — fine. + - Alternative per the TODO: key on `last(destRoutes)` — but the appended `notFoundRoute` leaf can be shared across branches; array-identity keying avoids that hazard. +2. While here, consider removing the closure-captured mutable `let dest / includeValidateSearch` (`router.ts:3127-3128`) and threading them as arguments through `applyNext`, making the cached chain re-entrant (protects against a user middleware that synchronously calls `buildLocation` again). + +## Risks & constraints + +- **Re-entrancy**: today each call gets fresh closures; a cached chain with captured mutable state must not be invoked recursively (middleware → `buildLocation` → same chain). Step 2 eliminates this class of bug; if skipped, document/guard. +- Route objects are recreated on HMR/`update()`, so a WeakMap keyed on routes/branches self-invalidates. +- Users mutating `route.options.search.middlewares` at runtime would see stale chains — not a supported pattern. +- Bundle delta ≈ +5 lines. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +The win is mostly GC/allocation; expect a small direct delta on client-nav, clearer when stacked on [002](002-match-routes-lightweight-memo.md). diff --git a/perf-investigations/004-load-matches-sync-fastpaths.md b/perf-investigations/004-load-matches-sync-fastpaths.md new file mode 100644 index 0000000000..c0db3b1d43 --- /dev/null +++ b/perf-investigations/004-load-matches-sync-fastpaths.md @@ -0,0 +1,49 @@ +# 004 — sync fast paths in the loader phase of `loadMatches` + +| | | +| --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/load-matches.ts`, `router.ts` (shared client+server) | +| Benchmarks | client-nav (react), ssr (react) | +| Expected impact | Medium-large — shortens the microtask chain in front of every render commit | +| Confidence | High | +| Risk | Medium (error/redirect propagation paths) | +| Prior art | `origin/flo/load-matches-sync-fastpaths` (commits `14ecec2e52`, `4fc676c334`, `9be80233eb` — incl. a fix preserving sync beforeLoad abortController and store-update-count test adjustments). **Rebase/extend this branch rather than starting fresh.** Follows up on merged #7582 ("avoid creating promises when not necessary"), which only removed 4 trivial `Promise.resolve()`s. | + +## Problem + +The beforeLoad phase already uses a sync-continuation pattern (`handleBeforeLoad`, `load-matches.ts:533-558`, only awaits when `isPromise(beforeLoad)`), but the loader phase is unconditionally async: + +- `load-matches.ts:784` — `loadRouteMatch` is `async`. +- `load-matches.ts:789-854` — nested `async function handleLoader` is **re-created per match per navigation** (closure over `matchId`, `loaderShouldRunAsync`, …). +- `load-matches.ts:641` — `runLoader` is `async`. +- `load-matches.ts:972, 1025-1030` — every match pushes a promise into `matchPromises`, then `await Promise.all(matchPromises)`. +- `router.ts:2486` — `onReady: async () => {...}` has a fully synchronous body; the `async` keyword forces `triggerOnReady` (`load-matches.ts:1171-1174`) to await a promise. + +When all loaders are sync (the benchmark; also the common cache-hit case in real apps), this is 3 async-function frames per match — each a promise allocation + 1-2 microtask hops — and the React commit the benchmark waits for is queued behind all of them. Depth-2/3 match trees × 10 navigations per iteration. + +## Proposed approach + +1. Convert `loadRouteMatch` to return `AnyRouteMatch | Promise` using the same `isPromise` continuation style as `handleBeforeLoad`. Only push actual promises into `matchPromises`; skip `Promise.all` entirely when none are. +2. Hoist `handleLoader` to module scope with explicit args (also removes one closure per match per navigation). The `serverSsr`/`execute`/`queueExecution` closures in `handleBeforeLoad` (`load-matches.ts:540-555`) can be similarly flattened on the client, where `isServer` is statically `false` and dead-code-eliminated. +3. Make `onReady` (`router.ts:2486`) a plain sync function — `triggerOnReady` already handles a `void` return. + +## Risks & constraints + +- `getLoaderContext` (`load-matches.ts:614`) passes `matchPromises[index - 1]` as `parentMatchPromise` to loaders. When a loader actually runs it must still receive a promise — lazily wrap with `Promise.resolve(match)`. +- Error propagation: `handleRedirectAndNotFound` throws; the sync path needs equivalent try/catch placement so redirects/notFound behave identically (the existing branch has test updates around this). +- Solid/vue store-update-count tests assert exact store write counts; the prior-art branch already adjusted them (`9be80233eb`) — those frameworks are out of scope for changes but their tests still run. +- Code size roughly neutral (restructure, not addition) but verify with the bundle benchmark. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +## Related + +- [005](005-loaderless-runloader-fast-path.md), [006](006-update-match-write-coalescing.md), [008](008-lazy-controlled-promise.md) touch the same call sites — coordinate. +- [022](022-commit-path-micro-allocations.md) covers the analogous de-async work in `navigate`/`commitLocation`. diff --git a/perf-investigations/005-loaderless-runloader-fast-path.md b/perf-investigations/005-loaderless-runloader-fast-path.md new file mode 100644 index 0000000000..ff9e22d797 --- /dev/null +++ b/perf-investigations/005-loaderless-runloader-fast-path.md @@ -0,0 +1,50 @@ +# 005 — skip `runLoader` for routes that have no loader + +| | | +| --------------- | --------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/load-matches.ts` (shared client+server) | +| Benchmarks | client-nav (react) | +| Expected impact | Medium-large — removes one full match-store write + context rebuild + detached async chain per entered match per navigation | +| Confidence | High (branch conditions verified against the benchmark's route options) | +| Risk | Medium (must enumerate everything `runLoader` does besides calling the loader) | +| Prior art | none found; adjacent to `origin/flo/load-matches-sync-fastpaths` ([004](004-load-matches-sync-fastpaths.md)) | + +## Problem + +The stale-while-revalidate path fires `runLoader` even when the route has **no loader at all**: + +- `load-matches.ts:802` — `staleAge` defaults to 0, so for a `status === 'success'` match, `staleMatchShouldReload` (`load-matches.ts:818-823`) is true whenever `cause === 'enter'` or the route at that position changed — i.e. every `/items/1 → /items/2` style navigation. +- `load-matches.ts:835-848` — `loaderShouldRunAsync` triggers a fire-and-forget IIFE running `runLoader` **without checking `route.options.loader` exists**. +- `runLoader` (`load-matches.ts:641-782`) then: calls `loadRouteChunk(route)` (`:660`), finds no loader, and still finishes with a full `updateMatch` that re-spreads the ~35-field match object, rebuilds `context` via `buildMatchContext`, and stamps `updatedAt: Date.now()` (`:717-724`) — plus the IIFE's own promise chain resolving `loaderPromise`/`loadPromise` in detached microtasks (`:838-842`). + +In the client-nav benchmark, the `/items/$id`, `/items/$id/details`, and `/ctx/$id` routes are all loaderless, so this path runs ~1-2× per navigation. Real apps with layout/param routes without loaders pay the same on every param change. + +## Proposed approach + +In `handleLoader`, add a fast path when **all** of: + +- `!route.options.loader` +- no lazy route module pending (`route._lazyPromise`/`_lazyLoaded` state — a `lazyFn` could still add a loader) +- components already loaded (`route._componentsPromise`/`_componentsLoaded`) +- no `match._nonReactive.minPendingPromise` + +→ perform the status/`updatedAt`/context update synchronously, merged with the `syncMatchContext` write, instead of invoking `runLoader`. + +Also handle the **blocking** branch (`load-matches.ts:849-850`): loaderless routes whose status is `'pending'` (e.g. a route with only `beforeLoad`, like the benchmark's `/ctx/$id`) go through `await runLoader(...)` and need the same treatment. + +## Risks & constraints + +- `runLoader` is responsible for more than the loader: `route._lazyPromise`, `_componentsPromise` preloading, `errorComponent` head preloading, `updatedAt` freshness, status transitions. The fast path must be provably equivalent for the loaderless case — enumerate each side effect and reproduce or consciously skip it. +- `updatedAt` semantics feed staleness decisions on the next navigation; keep stamping it. +- Coordinate with [004](004-load-matches-sync-fastpaths.md) and [006](006-update-match-write-coalescing.md) — overlapping call sites; implementing together avoids double refactors. +- Bundle: restructure, roughly neutral; verify. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Watch specifically: lazy-route tests, preload tests, pending-component tests. diff --git a/perf-investigations/006-update-match-write-coalescing.md b/perf-investigations/006-update-match-write-coalescing.md new file mode 100644 index 0000000000..ee802d41dc --- /dev/null +++ b/perf-investigations/006-update-match-write-coalescing.md @@ -0,0 +1,48 @@ +# 006 — coalesce per-match store writes (`pending`/`resolve`/`syncMatchContext`) and reuse AbortControllers + +| | | +| --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/load-matches.ts`, `router.ts` (shared client+server) | +| Benchmarks | client-nav (react) | +| Expected impact | **Large** in combination — fewer 35-field object copies, fewer store flushes, an order of magnitude fewer React transition state updates per navigation (with [007](007-transitioner-transition-refcount.md)) | +| Confidence | High | +| Risk | Medium (observable `isFetching`/`fetchCount` semantics) | +| Prior art | none found directly; `origin/refactor-router-core-ready-transition-performance` touches the adjacent onReady/transition area | + +## Problem + +On the fully-sync / cache-hit path, each match still receives 3-5 separate store writes per navigation, each a full spread of the ~35-field match object, and each routed through `router.startTransition`: + +- `load-matches.ts:418-447` — `executeBeforeLoad` calls `pending()` then `resolve()` **even when `route.options.beforeLoad` is absent**: `pending()` spreads the match with `fetchCount + 1`, a `new AbortController()`, `isFetching: 'beforeLoad'`; `resolve()` spreads again with `isFetching: false`. +- `load-matches.ts:191-204` — `syncMatchContext` always writes a brand-new `context` object (identity churn for `useRouteContext` subscribers). +- Additional writes at `load-matches.ts:683-687`, `:700-704`, `:717-724` (loader phase), `:927-931`, `:949-955`. +- `router.ts:2698-2727` — every `updateMatch` wraps its write in `this.startTransition`, which on React is two `setState` calls on the Transitioner (see [007](007-transitioner-transition-refcount.md)). ~8-12 `updateMatch` calls per navigation → ~20 React state updates, in separate microtask continuations so they don't batch. +- `load-matches.ts:415` — a fresh `AbortController` per match per navigation even though the previous one was never aborted (the fast path completes synchronously; `cancelMatches`, `router.ts:1801-1820`, only aborts pending/loading matches). New matches even get two (`router.ts:1624` in `matchRoutesInternal` + `executeBeforeLoad`). In jsdom (the benchmark environment) AbortController/AbortSignal are pure-JS event-target objects, notably more expensive than native. + +Writes during loading go to pending-pool atoms with no component subscribers, but still dirty the computed graph (`stores.ts:146-158` `pendingMatches`/`hasPending`) and flush effects. + +## Proposed approach + +1. **No-beforeLoad collapse**: when `!route.options.beforeLoad`, replace `pending()+resolve()` with a single write (`{...prev, fetchCount: prev.fetchCount + 1, abortController}` — the `isFetching` round-trip nets out within the batch). Keep the two-phase writes when `beforeLoad` is genuinely async (its `isFetching: 'beforeLoad'` state is observable by `useMatch` subscribers mid-load). +2. **`syncMatchContext` bail-out**: shallow-compare the rebuilt context against `prev.context` and skip the write when equal — also stabilizes context identity for `useRouteContext` selectors (enables [019](019-match-identity-reuse.md)). +3. **AbortController reuse**: keep `match.abortController` when `!signal.aborted`; allocate a new one only after an actual abort. Drop the duplicate eager creation for brand-new matches (`router.ts:1624`) or create lazily when beforeLoad/loader exists. +4. **Transition dedup** belongs to [007](007-transitioner-transition-refcount.md) (react side); alternatively add a core-side "already in transition" flag set by `load()` (`router.ts:2454`) so nested `updateMatch` calls skip the wrapper. + +## Risks & constraints + +- `fetchCount` increments on every navigation today, even without a fetch — merging writes preserves that; _skipping_ writes would change it. +- Loaders/beforeLoad receive `abortController` in context; a reused signal spans multiple fetches of the same match. That matches the documented intent (abort on navigate-away), but tests may assert fresh controllers — check `load-matches`-related tests. The error path (`load-matches.ts:242`) deliberately resets the controller; keep that. +- Solid/vue adapters install their own `startTransition`; any core-side flag must be adapter-agnostic. Their store-update-count tests will need count adjustments (precedent: `9be80233eb` on the 004 branch). + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +## Related + +- Implement together with [004](004-load-matches-sync-fastpaths.md)/[005](005-loaderless-runloader-fast-path.md) (same call sites). +- [019](019-match-identity-reuse.md) only pays off after this lands. diff --git a/perf-investigations/007-transitioner-transition-refcount.md b/perf-investigations/007-transitioner-transition-refcount.md new file mode 100644 index 0000000000..cac8bd3feb --- /dev/null +++ b/perf-investigations/007-transitioner-transition-refcount.md @@ -0,0 +1,52 @@ +# 007 — ref-count Transitioner `setIsTransitioning` instead of toggling per `updateMatch` + +| | | +| --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| Area | `packages/react-router/src/Transitioner.tsx`, `packages/router-core/src/router.ts` | +| Benchmarks | client-nav (react) | +| Expected impact | Medium — React update processing (`updateReducerImpl`) is the single largest frame in the client profile at 15.8%; Transitioner churn is a significant driver beyond legitimate content re-renders | +| Confidence | Medium-high (mechanism verified; exact attribution within the 15.8% not isolated) | +| Risk | Medium (`onResolved` emission ordering) | +| Prior art | solid got an analogous cleanup in #7584 (`41e7a24f69`); React's side untouched. `origin/refactor-router-core-ready-transition-performance` (`cd6db1f5dc`) reworks the adjacent onReady/transition code in router.ts — read it first. | + +## Problem + +`Transitioner.tsx:26-32` installs: + +```ts +router.startTransition = (fn) => { + setIsTransitioning(true) + React.startTransition(() => { + fn() + setIsTransitioning(false) + }) +} +``` + +and `router.ts:2698-2727` routes **every** `updateMatch` through `this.startTransition`. With 3-8 `updateMatch` calls per navigation (`load-matches.ts:421,435,505,683,700,717` …) plus `load()` itself (`router.ts:2454`), each navigation produces ~8-14 `startTransition` invocations = ~16-28 React `setState` calls on the Transitioner. They happen in separate async continuations of `loadMatches`, so they don't batch; each toggle re-renders the Transitioner and re-evaluates its 3 `useLayoutEffect`s + `usePrevious` hooks (`Transitioner.tsx:86-128`), and oscillates `isAnyPending`. + +## Proposed approach + +1. **Ref-count edges**: keep a depth counter in a ref inside the Transitioner-installed `startTransition`; call `setIsTransitioning(true)` only on the 0→1 edge and `setIsTransitioning(false)` only when the count returns to 0 (checked inside the transition callback). The boolean is only consumed as "any transition pending", so edge-triggering preserves observable semantics exactly. +2. Alternative (riskier): stop routing `updateMatch` through the bookkeeping wrapper entirely and use bare `React.startTransition` for match writes — but `isTransitioning` gates `onResolved` emission, so semantics need careful verification. + +## Risks & constraints + +- `isTransitioning` feeds `isAnyPending` → `onResolved`/`onBeforeRouteMount` emission timing (`Transitioner.tsx:86-128`). The client-nav benchmark itself awaits `onRendered`/`onResolved` — the final 1→0 edge must fire at the same point as today. The ref-count variant is the one that preserves this. +- Tests exist around `onResolved` ordering; run react-router unit tests. +- Bundle delta: a few bytes (one ref + two comparisons). + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Use the flame benchmark (`test:flame:react`) before/after: the win should show as a reduction in `updateReducerImpl`/commit frames. + +## Related + +- [006](006-update-match-write-coalescing.md) reduces how many `updateMatch` calls happen at all; this file makes each remaining one cheaper on the React side. Independent but complementary. +- [015](015-link-flushsync-click.md) covers the Link-side `flushSync(setIsTransitioning)` per click. diff --git a/perf-investigations/008-lazy-controlled-promise.md b/perf-investigations/008-lazy-controlled-promise.md new file mode 100644 index 0000000000..e2bd703893 --- /dev/null +++ b/perf-investigations/008-lazy-controlled-promise.md @@ -0,0 +1,44 @@ +# 008 — create `ControlledPromise`s lazily on the sync path + +| | | +| --------------- | ---------------------------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/router.ts`, `load-matches.ts`, `utils.ts` (shared client+server) | +| Benchmarks | client-nav (react) | +| Expected impact | Medium (60-90 controlled-promise allocations per benchmark iteration eliminated) | +| Confidence | Medium-high (beforeLoadPromise part is high; loadPromise part needs a consumer audit) | +| Risk | Medium (suspense + concurrent-load coordination) | +| Prior art | partially adjacent to `origin/flo/load-matches-sync-fastpaths` ([004](004-load-matches-sync-fastpaths.md)) | + +## Problem + +On the fully-sync fast path, each match allocates 2-3 `ControlledPromise`s per navigation (3 for brand-new matches), all resolved and nulled before `loadMatches` returns, with nothing ever awaiting them: + +- `router.ts:1620` — every new match in `matchRoutesInternal` gets `_nonReactive: { loadPromise: createControlledPromise() }`. +- `load-matches.ts:397-401` — `executeBeforeLoad` unconditionally replaces `loadPromise` with a fresh controlled promise chaining the previous one (so the constructor-time one is immediately superseded). +- `load-matches.ts:451` — `beforeLoadPromise` is created **before** knowing whether `beforeLoad` is sync. +- `load-matches.ts:925` — `loaderPromise` created on every non-skip `loadRouteMatch`. +- `utils.ts:462-486` — each `createControlledPromise` = native promise + executor closure + resolve/reject closures + status fields. + +The only consumers are React suspense (`react-router/src/Match.tsx:316/338/434/461` via `getMatchPromise`, only when status is `'pending'`/`_displayPending`) and overlapping-load coordination (`load-matches.ts:364-368, 893-905`). + +## Proposed approach + +1. **`beforeLoadPromise`**: create it only inside the `isPromise(beforeLoadContext)` branch (`load-matches.ts:516`) — move creation after the call; sync callers never observe it. (Highest confidence, smallest diff.) +2. **`loaderPromise`**: create only when `handleLoader` decides async work will actually happen; it's needed by concurrent `loadRouteMatch` calls only once a load is genuinely in flight. +3. **Constructor-time `loadPromise`** (`router.ts:1620`): audit consumers — it's superseded at `:397-401` on every load; either drop the eager creation or create lazily via a `getMatchPromise`-side fallback. Must guarantee existence whenever a match can render as `'pending'` (suspense reads it). + +## Risks & constraints + +- Overlap detection: a second `loadMatches` (preload or rapid double navigation) checks `prevMatch._nonReactive.loaderPromise` (`load-matches.ts:893`) to detect in-flight loads. Creating it later shrinks but doesn't eliminate the window (everything before the first await is synchronous) — verify the preload/navigate race tests. +- Suspense: `Match.tsx` reads `loadPromise` while status is `'pending'`; the lazy path must cover `_displayPending`/`pendingComponent` scenarios. +- Bundle delta neutral-ish; verify. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Focus tests: suspense/pending-component, preload, concurrent navigation. diff --git a/perf-investigations/009-parse-location-reuse-built-location.md b/perf-investigations/009-parse-location-reuse-built-location.md new file mode 100644 index 0000000000..754031a138 --- /dev/null +++ b/perf-investigations/009-parse-location-reuse-built-location.md @@ -0,0 +1,58 @@ +# 009 — `parseLocation` reuses the location `navigate` just built + +| | | +| --------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/router.ts` (shared client+server) | +| Benchmarks | client-nav (react); small SSR benefit | +| Expected impact | Small-medium (once per navigation, but it's the densest string-processing block in the commit path; combos with 001) | +| Confidence | Medium (a previous broader attempt was reverted — see history) | +| Risk | Medium-high — **this exact area has a revert in its history**; keep the change strictly guarded | +| Prior art | #6398 "don't reparse upon navigation" was **reverted in #6468**. Read both PRs before starting. This proposal is deliberately a much narrower, byte-equality-guarded variant. | + +## Problem + +A navigation builds a complete `ParsedLocation`, commits it to history, and then immediately re-derives the same thing from the href: + +1. `buildLocation` produces `{href, publicHref, pathname, search, searchStr, hash, state}`; `buildAndCommitLocation` stores it as `this.pendingBuiltLocation` (`router.ts:2282`). +2. `commitLocation` pushes only `publicHref` + state into history (`router.ts:2226-2230`). +3. The history subscriber (`react-router/src/Transitioner.tsx:37`) calls `router.load()` → `beforeLoad()` → `updateLatestLocation()` (`router.ts:1220-1225`) → `parseLocation(this.history.location)` (`router.ts:1294-1374`), which re-runs `parseSearch` (URLSearchParams decode + JSON.parse attempts incl. thrown exceptions, see [001](001-search-params-json-parse-exception-storm.md)), re-runs `stringifySearch` for normalization (`router.ts:1309-1310`), `decodePath` ×2, `nullReplaceEqualDeep` on search and `replaceEqualDeep` on state — reconstructing what `pendingBuiltLocation` already holds. + +Timing is favorable: history `notify` fires synchronously inside `commitLocation`, before the `queueMicrotask` at `router.ts:2297-2301` clears `pendingBuiltLocation`, and `beforeLoad` runs synchronously inside `load()`'s `startTransition`. + +It also breaks structural identity: the re-parsed `search` is a fresh object, re-shared via `nullReplaceEqualDeep` against the _previous_ location rather than reusing the built object. + +## Proposed approach + +A guarded fast path in `updateLatestLocation` (or a dedicated branch in `parseLocation`): + +``` +if (this.pendingBuiltLocation + && this.pendingBuiltLocation.href === this.history.location.href + && !this.history.location.state.__tempLocation + && !this.rewrite) { + // reuse pathname/search/searchStr/hash/href/publicHref from the built location; + // take only state via replaceEqualDeep(previous?.state, history.location.state) + // (history adds __TSR_key/__TSR_index) +} +``` + +Fall back to the full parse for: masked locations (masked commits store `__tempLocation` state, `router.ts:2190-2217`), rewrites/external URLs, any href mismatch, and back/forward events (no pending built location). + +Alternative lighter form: an LRU of `rawSearchString → {search, searchStr}` inside the router, which also dedupes repeated parses of equal strings across history events — less reuse but fewer preconditions. + +## Risks & constraints + +- **Why the previous attempt was reverted**: read #6468's description/tests and make those cases explicit fall-backs. Keep the byte-equality guard (`href` exact match) so non-canonical hrefs still go through re-canonicalization. +- Custom `createHref`/browser history mutating the committed href: the post-roundtrip `history.location.href` comparison handles this (memory history's `createHref` is identity, `history/index.ts:628`). +- `parseSearch` impurity: search would skip the parse round-trip; the built object was produced by validators/middleware — which is what `matchRoutes` consumes anyway — but only behind exact href equality. +- Async history adapters: verify `pendingBuiltLocation` lifetime holds (it's cleared on a microtask). +- Bundle delta ≈ +15 guarded lines in router-core. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit +``` + +Focus tests: location masking, rewrites/basepath, back/forward, custom parseSearch/stringifySearch, hash handling. Find and re-run whatever test motivated the #6468 revert. diff --git a/perf-investigations/010-hook-selector-slice-memoization.md b/perf-investigations/010-hook-selector-slice-memoization.md new file mode 100644 index 0000000000..36fe32e262 --- /dev/null +++ b/perf-investigations/010-hook-selector-slice-memoization.md @@ -0,0 +1,49 @@ +# 010 — skip user `select` when the extracted match slice is referentially unchanged + +| | | +| --------------- | ---------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/react-router/src/` hooks (`useParams`, `useSearch`, `useLoaderData`, `useLoaderDeps`, `useRouteContext`, `useMatch`) | +| Benchmarks | client-nav (react) | +| Expected impact | Medium (the benchmark's 52 hook subscribers run deliberately-expensive selectors; real apps run arbitrary user code per store set) | +| Confidence | High on mechanism; medium on benchmark magnitude | +| Risk | Medium (stale-select hazards; must key on select identity) | +| Prior art | continues the consolidation from merged #7577 (`689d88e04c`, shared hook structuralSharing logic) | + +## Problem + +Every derived hook funnels to `useMatch` with a _composed_ selector: + +- `useParams.tsx:101-105`, `useSearch.tsx:101-103`, `useLoaderData.tsx:87-89`, `useLoaderDeps.tsx:65-67`, `useRouteContext.ts:27-28` — each wraps as `(match) => { const slice = match.X; return opts.select ? opts.select(slice) : slice }`. +- On **every** match-store `set`, `useSyncExternalStoreWithSelector` re-runs this selector for every subscriber (shared path `useMatch.tsx:53-66` `useStructuralSharing`, `useMatch.tsx:182-198`). + +But router-core deliberately stabilizes the slices: `match.params`/`match.search`/`loaderDeps` go through `replaceEqualDeep`/`nullReplaceEqualDeep` against the previous match (`router.ts:1586-1588`, `1627-1629`, `1667-1669`). So when a match object is replaced (new `cause`/`updatedAt`/identity) but the slice reference is unchanged, all subscribers still re-run their user `select` for nothing. In the benchmark: 20 root-level subscribers (strict:false `useParams`/`useSearch`, each running a 40-iteration computation) × every root-match set (≥1/navigation, more with post-commit context/loaderData updates), plus 6-18 route-level subscribers. + +## Proposed approach + +Refactor the hooks to pass `extract` (slice getter) and `select` separately to `useMatch`, then extend `useStructuralSharing` (or a sibling helper) to memoize: + +``` +slice = extract(match) +if (slice === prevSlice && select === prevSelect) return prevResult +prevResult = structuralSharing(select ? select(slice) : slice) +``` + +stored in the existing `previousResult` ref. Likely bundle-neutral or negative: it removes five near-identical wrapper closures. + +## Risks & constraints + +- A user `select` closing over changing component values must not get a stale cached result. Keying on `select` **identity** handles this: inline selects get a new identity each render, so the cache only short-circuits _store notifications between renders_ — exactly the safe hot path (same trick React Query uses). Users passing memoized selectors that close over other changing values were already broken under `useSyncExternalStoreWithSelector`'s own memoization assumptions. +- `shouldThrow`/undefined-match path must bypass the cache (see [011](011-use-match-invariant-throw.md)). +- Navigations that genuinely change the slice (e.g. `params.id`) must still re-run select — guaranteed by the reference check. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +## Related + +- [019](019-match-identity-reuse.md) (match identity reuse) attacks the same waste one level lower — if the match object itself keeps identity, stores don't notify at all. Both are worthwhile; this one also covers post-commit sets that legitimately replace the match. diff --git a/perf-investigations/011-use-match-invariant-throw.md b/perf-investigations/011-use-match-invariant-throw.md new file mode 100644 index 0000000000..2131966e00 --- /dev/null +++ b/perf-investigations/011-use-match-invariant-throw.md @@ -0,0 +1,59 @@ +DONE IN https://github.com/TanStack/router/pull/7595 + +# 011 — `useMatch` selector throws (and React swallows) an Error per unmounting subscriber per navigation + +| | | +| --------------- | -------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/react-router/src/useMatch.tsx` | +| Benchmarks | client-nav (react) | +| Expected impact | Small-medium (~1% of client profile + GC), very low risk | +| Confidence | High (profiled: `invariant` 50.8 ms self in a 5.4 s profile; mechanism verified in React's `checkIfSnapshotChanged`) | +| Risk | Low | +| Prior art | none found | + +## Problem + +`useMatch.tsx:182-198` — the selector passed to `useStore` throws when the match is missing: + +```ts +return useStore(matchStore ?? dummyStore, (match) => { + if (!match) { + if (opts.shouldThrow ?? true) { + ... + invariant() // throws Error('Invariant failed') + } + return undefined + } + return selector(match as any) +}) +``` + +When navigating away from a route (e.g. `/search` → `/items`), the subscribed match store resolves to `undefined` during the commit. The store notification re-runs each hook's selector via React's `checkIfSnapshotChanged`; the selector throws, **React's try/catch swallows the error and forces a re-render**, and the component unmounts before any render observes the throw. The Error (with stack capture) is pure overhead. + +In the benchmark: ~30 route-level subscribers on `/search` (10×3 hooks) and ~12 on `/ctx` each construct+throw+catch an Error on the navigation that removes them — every iteration. + +## Proposed approach + +Return a sentinel (or `undefined`) from the selector when the match is missing, and move the `shouldThrow` invariant into the hook body, applied to the value returned by `useStore` (i.e. on the render path): + +- Renders with a genuinely missing match still throw, with identical public behavior and message. +- Transient store notifications on about-to-unmount subscribers stop allocating/throwing. + +Needs a sentinel distinct from a legitimate `undefined` selector result (a module-level `const MISSING = Symbol()` or object) so `shouldThrow: false` keeps returning `undefined` to the caller while the hook can distinguish "missing match" from "select returned undefined". + +## Risks & constraints + +- Preserve `shouldThrow: false` → `undefined` exactly. +- The dev-mode descriptive error message (`useMatch.tsx:185-189`) must still be thrown from the same user-visible point (component render). +- Solid/vue have analogous code; out of scope here but note for parity. +- Bundle delta ≈ neutral (moves a branch). + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Focus tests: `useMatch`/hooks with `shouldThrow` both values, hooks used outside a matching route, unmount-during-navigation. diff --git a/perf-investigations/012-outlet-match-selector-compares.md b/perf-investigations/012-outlet-match-selector-compares.md new file mode 100644 index 0000000000..269d994ac3 --- /dev/null +++ b/perf-investigations/012-outlet-match-selector-compares.md @@ -0,0 +1,47 @@ +# 012 — `Outlet`/`Match`/`MatchInner` re-render on every match-store set (fresh-tuple selectors, `===` compare) + +| | | +| --------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/react-router/src/Match.tsx` | +| Benchmarks | client-nav (react) | +| Expected impact | Small-medium (one Outlet+Match+MatchInner per active level × every active-store set; `MatchView` rebuilds ~6 boundary elements + closures per re-render) | +| Confidence | High | +| Risk | Low-medium (under-selection of fields read in throw paths) | +| Prior art | none found | + +## Problem + +- `Match.tsx:524-527` (Outlet): + ```ts + const [routeId, parentGlobalNotFound] = useStore( + parentMatchStore, + (match) => [match?.routeId, match?.globalNotFound ?? false], + ) + ``` + A new array per selector run with the default `===` compare (`@tanstack/react-store` `defaultCompare`) → **every** parent match-store `set` forces an Outlet re-render even when neither field changed. +- `Match.tsx:78-80` — `Match` subscribes with an identity selector (`(value) => value`), re-rendering on every set; it only consumes `routeId`/`ssr`/`_displayPending` (memoized into `matchState` at `:82-93`), but the component render + `MatchView` element-tree rebuild (`:163-218`, ~6 boundary elements, `getResetKey`/`onCatch`/not-found `fallback` closures) still happens. +- `Match.tsx:371` — `MatchInner` likewise subscribes with identity and re-renders fully on every set (its `out` element is memoized, the render isn't). + +Each navigation performs several match-store sets per level (see [006](006-update-match-write-coalescing.md)), so with 3 active levels this is a steady stream of avoidable React renders. + +## Proposed approach + +1. **Outlet**: split into two scalar `useStore` selections, or pass a compare fn (`(a,b) => a[0]===b[0] && a[1]===b[1]`). +2. **Match**: select only the consumed fields (`routeId`, `ssr`, `_displayPending`) with a field-wise compare. +3. **MatchInner**: add a compare checking the identity of the fields actually read during render (`status`, `error`, `_displayPending`, `_forcePending`, `loaderDeps`, `_strictParams`, `_strictSearch`, …) — enumerate by reading the component body before finalizing the list. + +## Risks & constraints + +- Under-selecting a field read in a throw/suspense path is the failure mode — audit the full render body (note `match._nonReactive` is read fresh through `router.getMatch`, not via the subscription, so it's unaffected). +- Compare functions run per set per component — keep them field-identity checks only. +- Bundle delta: ± a few bytes. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Effect is amplified while [006](006-update-match-write-coalescing.md) hasn't landed (more sets today); still worthwhile after. diff --git a/perf-investigations/013-link-options-memo-stability.md b/perf-investigations/013-link-options-memo-stability.md new file mode 100644 index 0000000000..32bfd15ad1 --- /dev/null +++ b/perf-investigations/013-link-options-memo-stability.md @@ -0,0 +1,41 @@ +# 013 — Link `_options` memo defeated by inline `search`/`params` props + +| | | +| --------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/react-router/src/link.tsx` | +| Benchmarks | client-nav (react) | +| Expected impact | Small-medium (+2 avoidable `buildLocation` calls per click today; medium in apps whose link parents re-render often) | +| Confidence | High | +| Risk | Low | +| Prior art | `origin/perf/react-link-location-deps` explored this area (incl. removing search validation from buildLocation); its author judged the approach unsatisfactory ("i dont think this is good, just technically fun") — read it for pitfalls, don't resurrect it wholesale | + +## Problem + +- `link.tsx:385-400` — the `_options` memo's dependency array includes `options.search`, `options.params`, `options.state`, `options.mask` **by identity**. +- Typical usage (and the benchmark) passes inline literals and updater arrows: `search={{ page: 1, ... }}`, `params={{ id: itemsId }}`, `search={(prev) => ...}` — so `_options` is new on **every render**, invalidating the `next` memo (`link.tsx:410-413`) → full `buildLocation`, and recreating `doPreload`/`preloadViewportIoCallback` (`link.tsx:565-582`). +- Concretely: the clicked link re-renders twice per click from `setIsTransitioning(true/false)` (`link.tsx:620-627`) and pays two extra `buildLocation`s each time. Any parent re-render makes it O(`buildLocation` × links). + +## Proposed approach + +Stabilize `_options` with a previous-value ref doing a cheap equivalence check: + +- Keep prior dep values; compare `search`/`params`/`state`/`mask` objects via `deepEqual`/shallow compare (small POJOs — cheap; `deepEqual` is already imported in link.tsx), functions by identity only (same as today, no regression). +- Return the previous `_options` object when equivalent, so `next` truly recomputes only when `currentLocation` or real option values change. + +## Risks & constraints + +- `deepEqual` on every render costs something — these are tiny objects; ensure the compare short-circuits on reference equality first. +- Function props (updaters) can't be deep-compared; identity-only means inline updaters still invalidate — that matches current behavior, so no regression, but the win for updater-links comes from [002](002-match-routes-lightweight-memo.md)/[003](003-build-middleware-chain-cache.md) instead. +- Bundle delta small; reuse existing imports. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +## Related + +- [016](016-link-render-granularity.md) is the structural version of this fix; [013] is the low-risk subset. diff --git a/perf-investigations/014-link-isactive-client-prechecks.md b/perf-investigations/014-link-isactive-client-prechecks.md new file mode 100644 index 0000000000..c90b40f7e8 --- /dev/null +++ b/perf-investigations/014-link-isactive-client-prechecks.md @@ -0,0 +1,44 @@ +# 014 — port the server branch's `isActive` pre-checks to the client branch of Link + +| | | +| --------------- | -------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/react-router/src/link.tsx`, possibly `packages/router-core/src/utils.ts` | +| Benchmarks | client-nav (react) | +| Expected impact | Small (deepEqual over search objects ×22 links per navigation) — but essentially free, and deduplication may _shrink_ the bundle | +| Confidence | High (the exact logic already exists and runs in the SSR branch) | +| Risk | Very low | +| Prior art | none found | + +## Problem + +Link computes `isActive` in two places with different cost profiles: + +- **Server branch** (`link.tsx:244-266`): guards the search comparison with cheap pre-checks — `if (currentLocation.search !== next.search)` reference check plus empty-object short-circuits via `hasKeys` — before calling `deepEqual`. +- **Client branch** (`link.tsx:497-505`): calls `deepEqual(currentLocation.search, next.search, {partial: ...})` **unconditionally** for every link with `includeSearch` (the default) on every location change. + +Since `next.search` is rebuilt per `buildLocation` (`nullReplaceEqualDeep` stabilizes it against the freshly built `fromSearch`, not against `currentLocation.search`), `deepEqual`'s internal `a === b` fast path (`utils.ts:371`) rarely hits, so the full walk runs for ~22 links on every navigation. + +## Proposed approach + +Hoist the server branch's pre-check logic into a small shared helper used by both branches: + +- reference equality of the two search objects, +- `hasKeys` empty-object short-circuits, +- then the existing `deepEqual` with identical `partial`/`ignoreUndefined` flags. + +Deduplicating the two implementations into one function likely produces a net-negative bundle delta, which can help pay for other findings' additions. + +## Risks & constraints + +- Semantics already proven by the SSR path (it exists for hydration parity) — keep the `exact`/`explicitUndefined` flags identical between branches. +- None beyond standard test coverage. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Focus tests: link active-state tests (`link.test.tsx`), activeOptions matrix (exact/includeSearch/includeHash), SSR hydration-parity tests. diff --git a/perf-investigations/015-link-flushsync-click.md b/perf-investigations/015-link-flushsync-click.md new file mode 100644 index 0000000000..9cc1d792c8 --- /dev/null +++ b/perf-investigations/015-link-flushsync-click.md @@ -0,0 +1,56 @@ +# 015 — Link click handler: `flushSync` + per-click `onResolved` subscription + +| | | +| --------------- | ----------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/react-router/src/link.tsx` | +| Benchmarks | client-nav (react) | +| Expected impact | Small-medium (a forced synchronous React render+commit per click, before navigation even starts; 10 clicks per benchmark iteration) | +| Confidence | Medium (correctness is easy; need to confirm no test relies on synchronous attribute visibility) | +| Risk | Medium (observable timing of `data-transitioning`) | +| Prior art | none found | + +## Problem + +`link.tsx:618-639` — on every internal-navigation click: + +```ts +e.preventDefault() +flushSync(() => { + setIsTransitioning(true) +}) +const unsub = router.subscribe('onResolved', () => { + unsub() + setIsTransitioning(false) +}) +router.navigate({ ... }) +``` + +- `flushSync` forces React out of batching and synchronously renders+commits the clicked link's subtree **before** `router.navigate` runs. Per [013](013-link-options-memo-stability.md), that render re-runs `buildLocation` when option props are inline. +- The only consumer of this state is the `data-transitioning` attribute (`link.tsx:716, 724`) — the render-prop children only ever receive `isActive` (`link.tsx:929-934`). +- A one-shot `onResolved` subscription is allocated per click. + +## Proposed approach + +Option A (most conservative): drop `flushSync` and let the urgent state update batch with the navigation's first store write — the attribute still appears within the same frame. + +Option B (bigger win): set the attribute via direct DOM mutation on `innerRef.current` (`el.setAttribute('data-transitioning', '')` / `removeAttribute`) instead of React state, removing both renders per click. The ref is stable; re-renders that recreate props must keep the attribute consistent (set it in both the DOM path and the rendered props while transitioning). + +Either way, the `flushSync` import from `react-dom` may become removable — a bundle-size win. + +## Risks & constraints + +- Timing of `data-transitioning` becomes asynchronous relative to the click (Option A) or non-React-managed (Option B). Check `link.test.tsx` for assertions on synchronous visibility. +- Option B must handle: component re-render while transitioning (React would not know about the attribute — ensure rendered props agree), unmount mid-transition (the `onResolved` unsubscribe already handles state; DOM node is gone anyway). +- Keep `preventDefault`/navigation ordering identical. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +## Related + +- [007](007-transitioner-transition-refcount.md) (the Transitioner-level transition churn) and [013](013-link-options-memo-stability.md) (what each forced render costs). diff --git a/perf-investigations/016-link-render-granularity.md b/perf-investigations/016-link-render-granularity.md new file mode 100644 index 0000000000..cd15326c50 --- /dev/null +++ b/perf-investigations/016-link-render-granularity.md @@ -0,0 +1,65 @@ +# 016 — Link re-render granularity: derive `{href, isActive}` in the store selector + +| | | +| --------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/react-router/src/link.tsx` (structural refactor) | +| Benchmarks | client-nav (react) | +| Expected impact | **Potentially large** (React render/commit work — `updateReducerImpl` 15.8%, `useLinkProps` 3.2% of the client profile — dominates the benchmark), but highest effort/risk in this series | +| Confidence | Medium (direction is sound; sizing requires prototyping) | +| Risk | High (hook restructuring, bundle-size pressure) | +| Prior art | `origin/perf/react-link-location-deps` ("remove search validation from buildLocation" + link deps experiments; author's own assessment: "i dont think this is good, just technically fun") — mine it for pitfalls before designing | + +## Problem + +`link.tsx:403-413` — every Link subscribes to the whole location with `prev.href === next.href` equality and recomputes everything in render. So **all ~22 links re-render on every navigation** even when their own rendered output (`href`, `isActive`, `data-status`) is unchanged — e.g. `/search?page=2 → page=3` leaves the 8 `/items` links' DOM identical, yet they re-render through React. + +The actual recomputation (`buildLocation`) gets much cheaper with [002](002-match-routes-lightweight-memo.md)+[003](003-build-middleware-chain-cache.md), but the React render/commit per link remains. + +## Proposed approach + +Move `buildLocation` + active-state derivation **into the `useStore` selector**, selecting a small derived record and comparing it shallowly: + +```ts +useStore( + router.stores.location, + (loc) => { + const next = router.buildLocation({ + _fromLocation: loc, + ...optsRef.current, + }) + return { + href: next.publicHref, + isActive: computeIsActive(loc, next, activeOptionsRef.current), + } + }, + shallowRecordCompare, +) +``` + +- Computation still runs per link per location change (cheap after 002/003), but **React re-render/commit is skipped** when the record is unchanged. +- Unstable inline option objects/functions must be read via refs inside the selector (`optsRef` updated in render), since the selector identity must not capture per-render values. +- `isTransitioning` (see [015](015-link-flushsync-click.md)) should be excluded from the record or handled via direct DOM attribute, otherwise it forces renders back in. + +## Risks & constraints + +- Significant restructure of `useLinkProps` hook order; preload-on-viewport/intent logic, event handlers, and `activeProps`/`inactiveProps` merging all consume the derived values — they need to read from the selected record. +- Selector must stay pure w.r.t. the store value; reading refs is fine, but updating them must happen before notifications (render order guarantees this for prop changes; verify for the `from`-route context). +- **Bundle-size pressure is real here** — this is a rewrite of the hottest component file; budget the gzip delta from the start and aim to pay for it by deleting the now-redundant memo plumbing. +- SSR branch (`link.tsx:157-266`) must keep working without stores. +- Recommended approach: prototype behind a small benchmark-only build first; measure render counts (React Profiler) before committing to the full refactor. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:flame:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Success criterion: links whose href/active state didn't change produce zero React commits per navigation; flamegraph shows reduced `beginWork`/`updateProperties` share. + +## Related + +- Depends on [002](002-match-routes-lightweight-memo.md)/[003](003-build-middleware-chain-cache.md) to make per-link selector work cheap. +- Subsumes [013](013-link-options-memo-stability.md) if fully implemented (do 013 first anyway — it's low-risk and informs this design). diff --git a/perf-investigations/017-interpolate-path-template-cache.md b/perf-investigations/017-interpolate-path-template-cache.md new file mode 100644 index 0000000000..6633da843b --- /dev/null +++ b/perf-investigations/017-interpolate-path-template-cache.md @@ -0,0 +1,38 @@ +# 017 — cache parsed route-template segments for client `interpolatePath` + +| | | +| --------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/path.ts`, `new-process-route-tree.ts` (shared client+server) | +| Benchmarks | client-nav (react); also SSR cache-miss URLs | +| Expected impact | Medium (cheap per call, but ~130 template re-parses per client-nav iteration, plus once per route per `matchRoutes`) | +| Confidence | High on mechanism, medium on win size | +| Risk | Low-medium (bundle-size pressure is the main constraint) | +| Prior art | `origin/perf-path` (`ecdd3b2eb6` "optimize path.ts") and `origin/perf-process-route-tree` (`ddb360d575`) touch this area — **check both before implementing** to avoid conflicts/duplication | + +## Problem + +- `path.ts:244-402` — `interpolatePath` has a fast path **gated server-only** (`if (isServer ?? rest.server)` at `path.ts:263`; `isServer/client.ts` exports `false`, so the block is dead-code-eliminated from client bundles). +- On the client, every interpolation of a param-bearing template (e.g. `/items/$id`) re-runs `parseSegment` (`new-process-route-tree.ts:73-184`) per segment: `indexOf('/')`, `substring`, `includes('$')`, brace scanning — for the **same handful of immutable `route.fullPath` strings** every time. +- Call sites: `router.ts:1527` (per matched route per `matchRoutes`) and `router.ts:1950` (per `buildLocation`, i.e. per Link per location change). +- Templates without `$` early-return (`path.ts:260-261`), so only param routes pay; client-nav has ~10 param-bearing links + ~3 matched routes ≈ 13 template re-parses per navigation, ~130 per iteration. + +## Proposed approach + +1. Cache parsed segments per template string in a lazily populated module-level `Map` (e.g. a flattened `Uint16Array` of 6 values per segment, or an array of segment descriptors); the interpolation loop iterates the cached structure instead of calling `parseSegment`. The route set is bounded — no LRU needed (or reuse `createLRUCache` if worried about dynamically generated templates). +2. Alternative: attach the parsed-segment array lazily to the route instance and pass it into `interpolatePath` from both call sites (avoids a global map; slightly more plumbing). +3. **Do not** un-gate the server fast path for the client — that would re-add ~55 lines to the client bundle. + +## Risks & constraints + +- Bundle: estimated +~150 bytes gzip — this is the finding most likely to need an offsetting deletion (candidate: deduplicate the unguarded `trimPathRight` copy in `new-process-route-tree.ts:761-763`, which duplicates `path.ts:35-38` with the guard missing; note path.ts already imports from new-process-route-tree.ts, so inline the guard rather than importing the other way). +- Cache key must be the exact template string (it is — `ParsedSegment` offsets index into that string). +- Unbounded growth only with user-generated dynamic templates — rare; LRU if needed. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit # path.test.ts + new-process-route-tree.test.ts (173 tests) +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` diff --git a/perf-investigations/018-match-routes-internal-overheads.md b/perf-investigations/018-match-routes-internal-overheads.md new file mode 100644 index 0000000000..345b46d9a6 --- /dev/null +++ b/perf-investigations/018-match-routes-internal-overheads.md @@ -0,0 +1,48 @@ +# 018 — `matchRoutesInternal` / `build()` per-navigation fixed overheads + +| | | +| --------------- | ------------------------------------------------------------------------------------------------------------------ | +| Area | `packages/router-core/src/router.ts` (shared client+server) | +| Benchmarks | client-nav (react); scales with app size | +| Expected impact | Small-medium today; the snapshot loop is O(all stored matches) and grows with `gcTime`-cached matches in real apps | +| Confidence | Medium-high (one item needs a semantics check) | +| Risk | Medium | +| Prior art | none found | + +Four independent micro-inefficiencies in the once-per-navigation full matching path (`matchRoutesInternal` runs once per `load()`, profiled at 22.7 ms self in a 5.4 s client profile) and in `build()`: + +## 1. `previousActiveMatchesByRouteId` iterates ALL match stores + +`router.ts:1457-1465` — builds a fresh Map by iterating **every** entry of `this.stores.matchStores` and calling `store.get()` on each. `matchStores` holds active + pending + cached pools (`stores.ts:99-140`), so with `gcTime: 60_000` (benchmark) cached matches accumulate and the loop grows beyond the 2-3 active matches actually needed. + +**Strategy**: build the snapshot only for routeIds relevant to `matchedRoutes`, e.g. by iterating `stores.matchesId` (the active list) instead of all stores, or maintain an incremental routeId→matchId index. +**Risk**: confirm which pools are _supposed_ to contribute — if cached matches were intentionally included (param-reuse semantics), restrict differently. Verify against `stores.ts:99-140` and the params-reuse tests. + +## 2. `loaderDepsHash` JSON.stringify per route per navigation + +`router.ts:1520-1525` — `loaderDepsHash = loaderDeps ? JSON.stringify(loaderDeps) : ''` recomputed even when deps are unchanged. + +**Strategy**: compute the hash only after the `replaceEqualDeep(previousMatch.loaderDeps, loaderDeps)` equality pass; when the previous reference is returned, reuse the previous hash. + +## 3. Triple search spreads per route + +`router.ts:1481-1495` — three object spreads per matched route for search stabilization (`{...parentSearch}`, `{...parentSearch, ...strictSearch}`, `{...parentStrictSearch, ...strictSearch}`), plus `validateSearch` re-runs although `buildAndCommitLocation` already ran it with `_includeValidateSearch: true` (`router.ts:2277-2280`). + +**Strategy**: skip spreads when `strictSearch` is empty/unchanged; investigate reusing the validated search from the built location (guarded — see [009](009-parse-location-reuse-built-location.md) for the precedent of being careful here). + +## 4. `params.stringify` loop in `build()` re-resolves options per call + +`router.ts:1928-1944` — for every `buildLocation` (~23×/navigation), the loop does the two-level option lookup (`route.options.params?.stringify ?? route.options.stringifyParams`) per destRoute, and runs user stringify fns (fresh object + `Object.assign`) even when `opts.leaveParams` makes the result unused. + +**Strategy**: precompute a per-branch `stringifyFns` array (or `null` when none) cached alongside `getRouteBranch`/`match.branch`, so branches without stringify fns skip the loop entirely; check whether `leaveParams` can skip it (verify `usedParams` consumers first). Semantic result-caching of user fns is NOT safe — don't attempt. +**Risk**: invalidate the per-branch cache on `route.update()`. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Items 1-3 are independent of each other; item 1 is the most valuable for real apps (turns O(stored matches) into O(active matches)). diff --git a/perf-investigations/019-match-identity-reuse.md b/perf-investigations/019-match-identity-reuse.md new file mode 100644 index 0000000000..50b4dd2f94 --- /dev/null +++ b/perf-investigations/019-match-identity-reuse.md @@ -0,0 +1,41 @@ +# 019 — reuse match object identity for unchanged matches in `matchRoutesInternal` + +| | | +| --------------- | ------------------------------------------------------------------------------------------ | +| Area | `packages/router-core/src/router.ts`, `stores.ts` (shared client+server) | +| Benchmarks | client-nav (react); bigger in real apps with stable layout routes | +| Expected impact | Medium-large **when combined with 006/008/020**; small alone | +| Confidence | Medium (highest-behavior-risk finding in the core series) | +| Risk | High — copy-on-write discipline required | +| Prior art | none found; `origin/perf-replaceEqualDeep` optimizes the equality machinery this relies on | + +## Problem + +Every navigation rebuilds **every** matched route's match object, even when nothing about it changed: + +- `router.ts:1580-1590` — unconditional `{...existingMatch, ...}` spread with new `cause`, new `updatedAt`, and (via `router.ts:1651-1655`, `1698-1702`) a fresh `context` object identity. `search`/`params`/`loaderDeps` are already structurally shared (`router.ts:1586-1588`, `1627-1629`, `1667-1669`), and `_strictSearch` is not (`router.ts:1495`). +- At commit, `reconcileMatchPool` (`stores.ts:306-342`) sets a match store whenever `existing.get() !== nextMatch` (`stores.ts:333-335`) — which is **always**, because the spread guarantees fresh identity. +- Consequence: the **root match** notifies its full subscriber set on every navigation. In the benchmark that's the largest fan-out in the app: 20 root-level selector hooks (strict:false `useParams`/`useSearch`) re-run per navigation even when root params/search are untouched. + +## Proposed approach + +After computing the new fields for an existing match, compare the reactive fields against `existingMatch` — `search`, `params`, `_strictSearch` (give it `replaceEqualDeep` first), `loaderDeps`, `searchError`, `globalNotFound`, `cause`, `context` (after [006](006-update-match-write-coalescing.md)'s context bail-out stabilizes it). If all are reference-equal, **return `existingMatch` itself**, so `reconcileMatchPool` skips the `set` and no subscriber is notified. + +Apply `replaceEqualDeep` to `_strictSearch` and the `context` spread regardless — that alone improves downstream selector short-circuiting ([010](010-hook-selector-slice-memoization.md)). + +## Risks & constraints + +- **Copy-on-write hazard**: `matchRoutesInternal` currently mutates the _new_ object after the spread (`router.ts:1643-1655` — `cause` 'enter'→'stay' transitions, `globalNotFound`/`searchError` resets). Reusing `existingMatch` means those mutations would write into **live store state**. The equality check must cover every post-spread mutation, and any difference must fall back to the spread path. +- `updatedAt`/`cause` semantics if a "stay" navigation is skipped: `cause` transitions are observable (`onStay` hooks, `match.cause` in loaders). Only reuse identity when `cause` is also unchanged. +- Only worthwhile **after** [006](006-update-match-write-coalescing.md)/[008](008-lazy-controlled-promise.md): otherwise the later `fetchCount`/`abortController`/`context` writes in `loadMatches` break identity again anyway. +- Structural-sharing test suites should catch regressions; add a test asserting the root match store does not notify on an unrelated child param change. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Success criterion: navigating `/items/1 → /items/2` produces zero notifications on the root match store's subscribers. diff --git a/perf-investigations/020-build-match-context-incremental.md b/perf-investigations/020-build-match-context-incremental.md new file mode 100644 index 0000000000..4ed28f8d61 --- /dev/null +++ b/perf-investigations/020-build-match-context-incremental.md @@ -0,0 +1,40 @@ +# 020 — accumulate `buildMatchContext` incrementally instead of re-walking ancestors + +| | | +| --------------- | ---------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/load-matches.ts`, `router.ts` (shared client+server) | +| Benchmarks | client-nav (react); grows with route depth | +| Expected impact | Medium (O(depth²) → O(depth) per navigation; ~6-8 context rebuilds per navigation today) | +| Confidence | Medium-high | +| Risk | Medium (concurrency/ordering assumptions must be verified) | +| Prior art | none found | + +## Problem + +- `load-matches.ts:61-78` — `buildMatchContext` re-spreads `router.options.context` and `Object.assign`s **every ancestor's** `__routeContext`/`__beforeLoadContext`, calling `getMatch` per ancestor; `getMatch` (`router.ts:2729-2735`) probes up to 3 Maps (cached → pending → active pools) per lookup. +- Callers: `load-matches.ts:144`, `:196` (`syncMatchContext`), `:456` (`executeBeforeLoad`), `:618` (`getLoaderContext`), `:720`, `:738` — so per navigation with depth-3 matches, contexts are rebuilt ~6-8 times, each O(depth): **O(depth²)** total. +- `matchRoutesInternal` independently builds `match.context` again (`router.ts:1651-1655`, `1698-1702`). + +## Proposed approach + +Matches are processed serially in index order in both the beforeLoad loop and the loader phase, so accumulate in `InnerLoadContext`: + +- Keep `contextAccumulator[index]`; extend from `index - 1` instead of re-walking from the root. +- Invalidate the suffix when a match's `__beforeLoadContext` is written (beforeLoad of index i completes before anything at index > i reads context). +- Reuse the previous context object when no ancestor contributed changes — this stabilizes `match.context` identity, enabling the `syncMatchContext` bail-out in [006](006-update-match-write-coalescing.md) and identity reuse in [019](019-match-identity-reuse.md). + +## Risks & constraints + +- The loader phase runs matches "concurrently" via `Promise.all`, but context for index i depends only on beforeLoad results, which are strictly serial and complete before loaders start — verify the redirect/error mid-stream cases (`load-matches.ts:144`) still see correct partial context. +- The accumulator must not leak across `loadMatches` invocations (preload vs navigate overlap). +- Bundle delta roughly neutral (replaces a loop with an index read + invalidation bookkeeping). + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Focus tests: beforeLoad context inheritance, context invalidation, redirect-from-beforeLoad, preload. diff --git a/perf-investigations/021-commit-cached-pool-single-pass.md b/perf-investigations/021-commit-cached-pool-single-pass.md new file mode 100644 index 0000000000..71275abb46 --- /dev/null +++ b/perf-investigations/021-commit-cached-pool-single-pass.md @@ -0,0 +1,47 @@ +# 021 — single-pass cached-pool reconciliation in the commit path + +| | | +| --------------- | -------------------------------------------------------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/router.ts`, `stores.ts` (shared client+server) | +| Benchmarks | client-nav (react) | +| Expected impact | Small (clean win, low risk; O(matches + cached) once per navigation) | +| Confidence | High | +| Risk | Low-medium (eviction-ordering semantics) | +| Prior art | `origin/refactor-router-core-ready-transition-performance` (`cd6db1f5dc`) reworks this exact onReady region — **read/rebase it first** | + +## Problem + +The commit batch (`router.ts:2505-2565`, `onReady`) reconciles the cached pool **twice** per navigation and allocates ~6 intermediate collections: + +- `exitingMatches` filter (`router.ts:2510-2515`), `pendingRouteIds`/`activeRouteIds` Sets (`:2519-2526`), three hook-filter arrays (`:2528-2542`). +- First reconciliation: `setCached([...cached, ...exitingMatchesToKeep])` (`:2555-2563`). +- Immediately after: `clearExpiredCache()` (`:2564`) → `clearCache` → a **second** `setCached` (`router.ts:2840-2865`) that re-filters everything just appended. +- Each `setCached` runs `reconcileMatchPool` (`stores.ts:306-342`): ids map + Set + per-entry pool ops + `arraysEqual`. + +Notably, `clearExpiredCache`'s filter evicts any route **without a loader**, so loaderless exiting matches (items/ctx in the benchmark) are appended to the cache by the first `setCached` and immediately removed by the second — pure churn, every navigation. + +## Proposed approach + +Compute the final cached list in one pass: + +1. Filter `exitingMatches` by status AND gc-eligibility/loader-presence **before** a single `setCached`. +2. Fold the gc sweep of pre-existing cached entries into the same pass. +3. Optionally compute the `exiting`/`entering`/`staying` partitions in one loop over `currentMatches` + `pendingMatches` instead of 4 filters + 2 Sets. + +Keep `clearExpiredCache` as a public/elsewhere-used method; just stop calling it redundantly inside commit. + +## Risks & constraints + +- Eviction ordering vs the `status !== 'error'` filters must match current behavior exactly (a match evicted by gc must not receive onLeave-adjacent handling differently). +- Coordinate with the prior-art branch — it already restructures this block; landing it may subsume this file. +- Bundle delta neutral-to-negative. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Focus tests: gcTime/staleTime cache retention, cache invalidation, onEnter/onStay/onLeave ordering. diff --git a/perf-investigations/022-commit-path-micro-allocations.md b/perf-investigations/022-commit-path-micro-allocations.md new file mode 100644 index 0000000000..d18eae4a85 --- /dev/null +++ b/perf-investigations/022-commit-path-micro-allocations.md @@ -0,0 +1,43 @@ +# 022 — commit-path micro-allocations: memory-history re-parse + residual async wrappers + +| | | +| --------------- | --------------------------------------------------------------------------------------- | +| Area | `packages/history/src/index.ts`, `packages/router-core/src/router.ts` | +| Benchmarks | client-nav (react) — uses memory history; SSR also creates a memory history per request | +| Expected impact | Small (once per navigation, but near-zero risk; several items are pure deletions) | +| Confidence | High that it's wasted work | +| Risk | Low | +| Prior art | follows up merged #7582 (`5127d861ae`), which de-promised parts of this path | + +Two clusters of small, independent wins on the once-per-navigation commit path. + +## A. Memory history re-parses and over-allocates per notify + +- `history/index.ts:595` — `createMemoryHistory.getLocation = () => parseHref(entries[index]!, states[index])`, and `notify` (`history/index.ts:119-122`) calls `getLocation()` on every push/replace. Each call runs `sanitizePath` (regex + `startsWith` + possible second regex) and three `indexOf`/`substring` passes. Compare `createBrowserHistory`, which caches `currentLocation = parseHref(destHref, state)` in `queueHistoryAction` (`history/index.ts:383`). +- `history/index.ts:653-685` — `parseHref` calls `createRandomKey()` **unconditionally** (`:661`) even though the result is discarded whenever `state` is provided (`state: state || {...}` at `:683`) — which is always, on this path. +- `history/index.ts:129-159` — `tryNavigation` is `async`, allocating a promise per push/replace even when there are no blockers and the path is fully synchronous. + +**Strategies** + +1. Move `createRandomKey()` into the `state ||` fallback branch of `parseHref` — zero risk, negative bundle delta. +2. Cache the parsed location in `createMemoryHistory`: update a `currentLocation` variable inside `pushState`/`replaceState` (path + state already in hand); re-parse only in `go`/`back`/`forward`. Mirrors the browser-history pattern. +3. Sync fast path in `tryNavigation` when `ignoreBlocker || blockers.length === 0`: just call `task()`; keep the async path for blockers. `push`/`replace` already ignore the returned promise. + +## B. Residual promise wrappers in `navigate`/`commitLocation` + +Per navigation, before `loadMatches` even starts, ~7 promises are allocated. The avoidable ones: + +- `router.ts:2143` — `commitLocation` is `async` but contains **no await** (ends with `return this.commitLocationPromise`): the keyword only adds a wrapper promise + an extra microtask hop for awaiters. De-`async` it and return the promise directly. +- `router.ts:2313` — `navigate` is `async`; its only `await` is in the `reloadDocument` blocker loop. Restructure so the hot path returns `this.buildAndCommitLocation(...)` directly and the `reloadDocument` branch lives in an async helper. + +**Risk note**: a non-async function throws synchronously where an async one returned a rejected promise. Both can throw from `buildLocation` (user `params.stringify`, search updaters). Verify test expectations; if needed, wrap in try/catch returning `Promise.reject(err)` to preserve semantics. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/history:test:unit && pnpm nx run @tanstack/router-core:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` + +Focus tests: history blockers, back/forward in memory history, navigate() rejection behavior. diff --git a/perf-investigations/023-ssr-router-update-three-times.md b/perf-investigations/023-ssr-router-update-three-times.md new file mode 100644 index 0000000000..0dc9ace9f2 --- /dev/null +++ b/perf-investigations/023-ssr-router-update-three-times.md @@ -0,0 +1,45 @@ +# 023 — SSR: `router.update()` runs 3× per request; the third re-parses the location for one context key + +| | | +| --------------- | -------------------------------------------------------------------------------------------- | +| Area | `packages/start-server-core/src/createStartHandler.ts`, `packages/router-core/src/router.ts` | +| Benchmarks | ssr (react) | +| Expected impact | Small-medium (~1-2% of request CPU; more for apps with expensive custom `parseSearch`) | +| Confidence | High on mechanism, medium on win size | +| Risk | Low-medium | +| Prior art | none found | + +## Problem + +Per SSR request, `RouterCore.update()` runs three times: + +1. In the `RouterCore` constructor (`router.ts:1024`) — unavoidable. +2. In `createStartHandler`'s `getRouter` (`createStartHandler.ts:466-479`) — necessary: sets history, triggers the only needed `parseLocation`. +3. At `createStartHandler.ts:587` — `routerInstance.update({ additionalContext: { serverContext } })` — exists **only to inject one context key**, yet `update()` (`router.ts:1056-1214`): + - re-spreads all options (`:1076-1079`), + - rebuilds the `protocolAllowlist` `Set` (`:1083`), + - calls `updateLatestLocation()` (`:1117-1119`) → a full `parseLocation` on the **same href**, including a `parseSearch` + `stringifySearch` round trip (and, pre-[001](001-search-params-json-parse-exception-storm.md), JSON.parse throws). + +Profile: `RouterCore.update` 0.6% self + constructor 0.7% + the redundant parse work. + +## Proposed approach + +Pick one: + +- **(a)** In `executeRouter`, set the key directly: `routerInstance.options.additionalContext = { serverContext }`. It is only consumed via `this.options.additionalContext` (`load-matches.ts:487`). Smallest diff; bypasses `update()` side effects for this key (fine today — verify no other consumer). +- **(b)** Add a fast path in `update()` that skips `updateLatestLocation`, the Set rebuild, and basepath logic when none of `history`/`routeTree`/`basepath`/`rewrite`/`protocolAllowlist` changed. More general (also helps client HMR-ish update calls), needs a careful key whitelist. +- (c) Passing `additionalContext` through update #2 is **not** possible — middleware context resolves later in the request. + +## Risks & constraints + +- (a) couples start-server-core to the options-mutation detail; add a comment or a tiny dedicated router method (`router.setAdditionalContext(ctx)`) if preferred. +- (b) touches shared code — keep the guard byte-cheap (client bundle). + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit +# e2e start tests cover server context propagation: +# verify additionalContext/serverContext reaches beforeLoad/loaders in start e2e suites +``` diff --git a/perf-investigations/024-ssr-handler-hoisting.md b/perf-investigations/024-ssr-handler-hoisting.md new file mode 100644 index 0000000000..f2d845024e --- /dev/null +++ b/perf-investigations/024-ssr-handler-hoisting.md @@ -0,0 +1,42 @@ +# 024 — SSR: hoist per-request handler boilerplate (URL parses, options factory, middleware flattening) + +| | | +| --------------- | ------------------------------------------------------------------------------------------------------- | +| Area | `packages/start-server-core/src/createStartHandler.ts`, `ssr-server.ts`, `createStart.ts` (server-only) | +| Benchmarks | ssr (react) | +| Expected impact | Small (~1%), but item 1 is essentially free | +| Confidence | High on item 1; medium on item 2 | +| Risk | Low (server-only code, no bundle constraint) | +| Prior art | none found | + +## Problem + +`createStartHandler.ts:410-449` runs on 100% of requests, before any routing: + +1. **Redundant URL parsing**: `getNormalizedURL(request.url)` (`:410`) parses the URL twice internally (`ssr-server.ts:762-781`: `rawUrl` + final `new URL`), then `getOrigin(request)` (`:412`) does a third `new URL(request.url)` (`ssr-server.ts:749-754`), and `new H3Event(request)` in `requestHandler` (`request-response.ts:127`) builds a fourth (`FastURL`). +2. **Per-request re-derivation of per-process-constant data**: + - `await entries.startEntry.startInstance?.getOptions()` (`:421`) re-invokes the user's options factory and re-dedupes serialization adapters every request (`createStart.ts:138-149`: new `Set` + `Array.from`). + - `serializationAdapters` array rebuilt (`:427-431`), `requestStartOptions` spread (`:433-439`), `flattenMiddlewares` re-flattened + `new Set` (`:442-449`). + +Profile: `startRequestResolver` 0.2% self plus scattered URL/Set work inside the node-internals bucket (7.8%). + +## Proposed approach + +1. Return `origin` from `getNormalizedURL` (it already has `rawUrl.origin`) and delete the `getOrigin` call. Free. +2. Memoize the derived data keyed by the resolved options object identity (`WeakMap`/last-value cache): `getOptions()` result → `serializationAdapters` → flattened request middlewares. Dynamic factories returning fresh objects still work (cache miss); static ones (the common case) hit. +3. Consider feeding the H3Event's already-parsed URL into normalization, or vice versa. + +## Risks & constraints + +- `getOptions()` is user code and may be intentionally dynamic — never cache across differing returned objects; key strictly by identity (or document that options must be stable). +- The `executedRequestMiddlewares` `Set` is mutated per request (`createStartHandler.ts:783`) — only the flattened **array** is cacheable; the Set must stay per-request. +- Server-only: no client bundle pressure. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/start-server-core:test:unit +``` + +Also run a start e2e suite touching request middleware ordering and custom basepath/origin handling. diff --git a/perf-investigations/025-ssr-html-boundary-scan.md b/perf-investigations/025-ssr-html-boundary-scan.md new file mode 100644 index 0000000000..7520c226b0 --- /dev/null +++ b/perf-investigations/025-ssr-html-boundary-scan.md @@ -0,0 +1,39 @@ +# 025 — SSR: `findHtmlBoundary` scans every closing tag per chunk + +| | | +| --------------- | ------------------------------------------------------------------------------------------------------- | +| Area | `packages/router-core/src/ssr/transformStreamWithRouter.ts` (server-only) | +| Benchmarks | ssr (react) — small here (~0.3%); medium for chunk-heavy streaming pages | +| Expected impact | Small in the benchmark; medium for suspense-streaming apps with many chunks | +| Confidence | Medium (code-read; not hot in this benchmark because it renders ~1 big chunk per request) | +| Risk | Low (server-only) | +| Prior art | `reserveStreamFastPath` (`transformStreamWithRouter.ts:208-212`) already exists — extend, don't replace | + +## Problem + +- `transformStreamWithRouter.ts:70-125` — `findHtmlBoundary` walks **all** `` case-insensitively, even after the last valid closing tag is identified. Cost is O(number of closing tags) JS-loop iterations per chunk instead of one native scan. +- Used in the read loop at `:796-848` — every app chunk pays it. +- `noteBarrierMarker`'s `chunk.includes(TSR_SCRIPT_BARRIER_ID)` (`:561-566`) adds another full chunk scan until the barrier is seen. + +The SSR benchmark renders with `progressiveChunkSize: Number.POSITIVE_INFINITY` (`renderRouterToStream.tsx:53,141`) → ~1 big chunk per request, so this is ~0.3% there. Real streaming apps (suspense boundaries, deferred data) produce many chunks and re-scan repeatedly. + +## Proposed approach + +Fast path first: a single forward `chunk.indexOf('`: + +- React/Solid/Vue renderers emit lowercase; user-injected HTML may not. Either probe both `'` detection exactly (current code handles ``). +- Must preserve the "safe split point" semantics for chunks that end mid-tag — that's the function's real job; the fast path only short-circuits the common whole-tag case. +- Server-only file: no client bundle pressure. + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit # transformStreamWithRouter tests +``` + +To see the real effect, also benchmark a streaming scenario (small `progressiveChunkSize` or a suspense-heavy page); consider adding such a variant to `benchmarks/ssr` as part of this work. diff --git a/perf-investigations/026-ssr-trie-allocations.md b/perf-investigations/026-ssr-trie-allocations.md new file mode 100644 index 0000000000..de16c76218 --- /dev/null +++ b/perf-investigations/026-ssr-trie-allocations.md @@ -0,0 +1,38 @@ +# 026 — route-matching trie: per-candidate frame allocations and LRU eviction churn under non-repeating URLs + +| | | +| --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| Area | `packages/router-core/src/new-process-route-tree.ts`, `lru-cache.ts` (shared client+server) | +| Benchmarks | ssr (react) — the bench generates a unique random URL per request, so the match LRU never hits; negligible for client-nav (cache hits after warmup) | +| Expected impact | Small-medium for SSR/unique-URL workloads | +| Confidence | Medium | +| Risk | High-ish relative to payoff — this is the most intricate code in the package (priority bitmasks, fuzzy resumable extraction) | +| Prior art | **`origin/perf-process-route-tree`** (`ddb360d575` + result notes `f36a2afcfa`) already optimizes this file (removes `sortTreeNodes` walk, adds a `%`-check fast path before `decodeURIComponent` in `extractParams`). **Land/rebase that branch first; only then evaluate what remains below.** | + +## Problem + +For a URL that misses the match cache (every request in the SSR bench; first-visit URLs in real servers): + +- `new-process-route-tree.ts:725-758` — `findRouteMatch`: `matchCache.get` (miss) + full trie walk + `buildRouteBranch` + `matchCache.set`; once the 1000-entry LRU is warm, **every set also evicts** (`lru-cache.ts:44-67`: delete + linked-list pointer surgery). Note the cache is still useful _within_ a request (subsequent `getMatchedRoutes` calls for rendered Links hit the just-inserted entry) — do not remove it. +- `new-process-route-tree.ts:1041-1051`, `1164-1304` — `getNodeMatch` pushes a 10-field frame object per candidate (`{node, index, skipped, depth, statics, dynamics, optionals, extract, rawParams}`); a 4-level dynamic tree costs ~10+ frames per miss, plus `path.split('/')` and `decodeURIComponent` per dynamic segment in `extractParams`. + +## Proposed approach (after the prior-art branch lands) + +1. **Single-candidate loop instead of push/pop**: when exactly one candidate child exists, advance a cursor in place rather than pushing a speculative frame — most segments in typical trees are unambiguous. +2. **Frame pooling or flat parallel arrays** for the speculative stack — cuts per-miss allocation; weigh against bundle size (shared code). +3. LRU: consider a cheaper eviction (ring buffer / clock) or a smaller per-set cost; only if profiling post-1/2 still shows `lru-cache.set` (`origin/perf-lru-cache` may already address internals — check it). + +## Risks & constraints + +- The full `new-process-route-tree.test.ts` suite (173 tests) is the safety net; fuzzy matching, optional params, and priority ordering are easy to subtly break. +- Bundle pressure: pooling code ships to the client; keep it minimal or server-gated only if it can be tree-shaken (`isServer` const folding). +- Verify against the SSR bench (unique URLs) _and_ client-nav (must not regress the cache-hit path). + +## Validation + +```bash +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +pnpm nx run @tanstack/router-core:test:unit +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +``` diff --git a/perf-investigations/README.md b/perf-investigations/README.md new file mode 100644 index 0000000000..ad8722d3df --- /dev/null +++ b/perf-investigations/README.md @@ -0,0 +1,109 @@ +# Performance Investigations + +One file per runtime-performance improvement opportunity for the core + react packages +(`router-core`, `history`, `react-router`, `start-*`). Each file is self-contained: an +implementation agent should be able to pick up a single file and produce a change. + +Generated 2026-06-10 from a code audit + CPU profiling of the benchmarks at commit `6f1daf5104`. + +## Ground rules (apply to every investigation) + +- **Goal: speed.** Wall-clock/CPU improvements in `benchmarks/client-nav` and `benchmarks/ssr`. + Memory only matters via GC pressure. +- **Hard constraint: client bundle size must not regress.** Gzip of all emitted client JS is + tracked in CI (`benchmarks/bundle-size`). Any change to code shipped to the client must be + validated against it. Server-only code (`start-server-core`, `router-core/src/ssr/ssr-server`) + has more freedom. +- **Behavior must not change.** The router has extensive unit tests; run the touched package's + `test:unit` target. Several proposals note specific semantic hazards — read them. + +## Validation commands + +```bash +# client navigation speed (the main client benchmark; jsdom) +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache +# flamegraph of the same loop +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:flame:react --outputStyle=stream --skipRemoteCache + +# SSR request-loop speed +CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache + +# bundle-size guard (always run for client-shipped code changes) +pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full +pnpm benchmark:bundle-size:query --id react-router.minimal + +# unit tests +pnpm nx run @tanstack/router-core:test:unit +pnpm nx run @tanstack/react-router:test:unit +``` + +Profiling baselines measured during this audit: client-nav ≈ **1.77 ms/navigation** +(prebuilt dist, node script over jsdom), SSR ≈ **0.957 ms/request** (NODE_ENV=production — +without it you profile dev React). + +## ⚠️ Check unmerged prior-art branches first + +Several opportunities already have in-flight branches. Before implementing a file that lists +prior art, inspect/rebase the branch instead of starting from scratch: + +| Branch | Covers | +| -------------------------------------------------------------- | ------------------------------------------------------------ | +| `origin/flo/search-params-json-parse-guard` | 001 (most recent, has review feedback applied) | +| `origin/refactor-router-core-stringify-parse-search-json-perf` | 001 (older alternative) | +| `origin/flo/current-location-lightweight-cache` | 002 | +| `origin/flo/load-matches-sync-fastpaths` | 004 (partially 008) | +| `origin/refactor-router-core-ready-transition-performance` | 007 / 021 vicinity | +| `origin/perf/react-link-location-deps` | 013/016 exploration (author considered it unsatisfactory) | +| `origin/perf-path` | 017 vicinity | +| `origin/perf-process-route-tree` | 026 + param-decode fast paths | +| `origin/perf-replaceEqualDeep` | `replaceEqualDeep` internals (touches many files' hot paths) | +| `origin/refactor-react-router-head-use-tags-performance` | head/useTags (not covered by a file here) | + +## Index, ordered by expected impact + +### Tier 1 — measured, large + +- [001 — search-params JSON.parse exception storm](001-search-params-json-parse-exception-storm.md) — **validated −42% SSR request time**; 5.7% of client profile +- [002 — memoize matchRoutesLightweight per location](002-match-routes-lightweight-memo.md) — validated additional −7% SSR; removes ~22/23 of per-Link match work on client +- [004 — sync fast paths in the loader phase of loadMatches](004-load-matches-sync-fastpaths.md) +- [006 — updateMatch write coalescing per match](006-update-match-write-coalescing.md) +- [007 — Transitioner startTransition/setIsTransitioning churn](007-transitioner-transition-refcount.md) + +### Tier 2 — high confidence, medium + +- [003 — cache buildMiddlewareChain](003-build-middleware-chain-cache.md) (explicit TODO in code) +- [005 — skip runLoader for loaderless routes](005-loaderless-runloader-fast-path.md) +- [008 — lazy ControlledPromise allocation](008-lazy-controlled-promise.md) +- [009 — parseLocation reuse of just-built location](009-parse-location-reuse-built-location.md) +- [010 — hook selector slice memoization](010-hook-selector-slice-memoization.md) +- [016 — Link re-render granularity](016-link-render-granularity.md) (largest potential, largest effort) +- [018 — matchRoutesInternal per-navigation overheads](018-match-routes-internal-overheads.md) +- [019 — match identity reuse for unchanged matches](019-match-identity-reuse.md) + +### Tier 3 — smaller / localized + +- [011 — useMatch selector throws Error per unmounting subscriber](011-use-match-invariant-throw.md) +- [012 — Outlet/Match/MatchInner selector compares](012-outlet-match-selector-compares.md) +- [013 — Link \_options memo defeated by inline props](013-link-options-memo-stability.md) +- [014 — port server isActive pre-checks to client](014-link-isactive-client-prechecks.md) +- [015 — Link flushSync on click](015-link-flushsync-click.md) +- [017 — interpolatePath template parse cache](017-interpolate-path-template-cache.md) +- [020 — incremental buildMatchContext](020-build-match-context-incremental.md) +- [021 — single-pass cached-pool reconciliation at commit](021-commit-cached-pool-single-pass.md) +- [022 — commit-path micro-allocations (memory history, async wrappers)](022-commit-path-micro-allocations.md) + +### Tier 4 — SSR/server-only + +- [023 — router.update() runs 3× per SSR request](023-ssr-router-update-three-times.md) +- [024 — hoist per-request handler boilerplate](024-ssr-handler-hoisting.md) +- [025 — findHtmlBoundary chunk scanning](025-ssr-html-boundary-scan.md) +- [026 — route-matching trie allocations under non-repeating URLs](026-ssr-trie-allocations.md) + +## Composition notes + +- 001, 002 and 003 compose: after all three, a Link re-render's `buildLocation` is mostly + LRU hits + `interpolatePath` + cheap stringify. +- 004, 005, 006, 008 touch overlapping call sites in `load-matches.ts` — coordinate or land + sequentially with rebases. +- 019 only pays off after 006 (otherwise later writes break identity anyway). +- 010, 011, 012 are independent react-router changes but all reduce per-store-set subscriber cost. From fafed4fbcc12da470aa691192ccddbb378348f17 Mon Sep 17 00:00:00 2001 From: Sheraff Date: Thu, 11 Jun 2026 00:04:45 +0200 Subject: [PATCH 3/5] Revert "fix scroll restoration" This reverts commit 620e9674fa8735aa3e261312b60aa80cdd6035fa. --- packages/react-router/src/Match.tsx | 23 +--- ...earch-params-json-parse-exception-storm.md | 71 ------------ .../002-match-routes-lightweight-memo.md | 67 ----------- .../003-build-middleware-chain-cache.md | 56 --------- .../004-load-matches-sync-fastpaths.md | 49 -------- .../005-loaderless-runloader-fast-path.md | 50 -------- .../006-update-match-write-coalescing.md | 48 -------- .../007-transitioner-transition-refcount.md | 52 --------- .../008-lazy-controlled-promise.md | 44 ------- ...009-parse-location-reuse-built-location.md | 58 ---------- .../010-hook-selector-slice-memoization.md | 49 -------- .../011-use-match-invariant-throw.md | 59 ---------- .../012-outlet-match-selector-compares.md | 47 -------- .../013-link-options-memo-stability.md | 41 ------- .../014-link-isactive-client-prechecks.md | 44 ------- .../015-link-flushsync-click.md | 56 --------- .../016-link-render-granularity.md | 65 ----------- .../017-interpolate-path-template-cache.md | 38 ------ .../018-match-routes-internal-overheads.md | 48 -------- .../019-match-identity-reuse.md | 41 ------- .../020-build-match-context-incremental.md | 40 ------- .../021-commit-cached-pool-single-pass.md | 47 -------- .../022-commit-path-micro-allocations.md | 43 ------- .../023-ssr-router-update-three-times.md | 45 -------- .../024-ssr-handler-hoisting.md | 42 ------- .../025-ssr-html-boundary-scan.md | 39 ------- .../026-ssr-trie-allocations.md | 38 ------ perf-investigations/README.md | 109 ------------------ 28 files changed, 6 insertions(+), 1403 deletions(-) delete mode 100644 perf-investigations/001-search-params-json-parse-exception-storm.md delete mode 100644 perf-investigations/002-match-routes-lightweight-memo.md delete mode 100644 perf-investigations/003-build-middleware-chain-cache.md delete mode 100644 perf-investigations/004-load-matches-sync-fastpaths.md delete mode 100644 perf-investigations/005-loaderless-runloader-fast-path.md delete mode 100644 perf-investigations/006-update-match-write-coalescing.md delete mode 100644 perf-investigations/007-transitioner-transition-refcount.md delete mode 100644 perf-investigations/008-lazy-controlled-promise.md delete mode 100644 perf-investigations/009-parse-location-reuse-built-location.md delete mode 100644 perf-investigations/010-hook-selector-slice-memoization.md delete mode 100644 perf-investigations/011-use-match-invariant-throw.md delete mode 100644 perf-investigations/012-outlet-match-selector-compares.md delete mode 100644 perf-investigations/013-link-options-memo-stability.md delete mode 100644 perf-investigations/014-link-isactive-client-prechecks.md delete mode 100644 perf-investigations/015-link-flushsync-click.md delete mode 100644 perf-investigations/016-link-render-granularity.md delete mode 100644 perf-investigations/017-interpolate-path-template-cache.md delete mode 100644 perf-investigations/018-match-routes-internal-overheads.md delete mode 100644 perf-investigations/019-match-identity-reuse.md delete mode 100644 perf-investigations/020-build-match-context-incremental.md delete mode 100644 perf-investigations/021-commit-cached-pool-single-pass.md delete mode 100644 perf-investigations/022-commit-path-micro-allocations.md delete mode 100644 perf-investigations/023-ssr-router-update-three-times.md delete mode 100644 perf-investigations/024-ssr-handler-hoisting.md delete mode 100644 perf-investigations/025-ssr-html-boundary-scan.md delete mode 100644 perf-investigations/026-ssr-trie-allocations.md delete mode 100644 perf-investigations/README.md diff --git a/packages/react-router/src/Match.tsx b/packages/react-router/src/Match.tsx index acecde9043..f5cffd5b4d 100644 --- a/packages/react-router/src/Match.tsx +++ b/packages/react-router/src/Match.tsx @@ -23,7 +23,6 @@ import { useLayoutEffect } from './utils' import type { AnyRoute, AnyRouteMatch, - ParsedLocation, RootRouteOptions, } from '@tanstack/router-core' @@ -248,15 +247,7 @@ function OnRendered() { } // eslint-disable-next-line react-hooks/rules-of-hooks - // @ts-expect-error -- init to `undefined` but don't write `undefined` to shave bytes - // Track the resolvedLocation as of the last render so that onRendered can - // report the correct fromLocation. By the time this effect fires, - // resolvedLocation has already been updated to the new location by - // Transitioner, so we cannot use router.stores.resolvedLocation.get() - // directly as the fromLocation. - const prevResolvedLocationRef = React.useRef< - ParsedLocation | undefined - >() + const prevHrefRef = React.useRef(undefined) // eslint-disable-next-line react-hooks/rules-of-hooks const renderedLocationKey = useStore( router.stores.resolvedLocation, @@ -265,23 +256,21 @@ function OnRendered() { // eslint-disable-next-line react-hooks/rules-of-hooks useLayoutEffect(() => { - const currentResolvedLocation = router.stores.resolvedLocation.get() - const previousResolvedLocation = prevResolvedLocationRef.current + const currentHref = router.latestLocation.href if ( - currentResolvedLocation && - (!previousResolvedLocation || - previousResolvedLocation.href !== currentResolvedLocation.href) + prevHrefRef.current === undefined || + prevHrefRef.current !== currentHref ) { router.emit({ type: 'onRendered', ...getLocationChangeInfo( router.stores.location.get(), - previousResolvedLocation ?? currentResolvedLocation, + router.stores.resolvedLocation.get(), ), }) + prevHrefRef.current = currentHref } - prevResolvedLocationRef.current = currentResolvedLocation }, [renderedLocationKey, router]) return null diff --git a/perf-investigations/001-search-params-json-parse-exception-storm.md b/perf-investigations/001-search-params-json-parse-exception-storm.md deleted file mode 100644 index 778ecce11c..0000000000 --- a/perf-investigations/001-search-params-json-parse-exception-storm.md +++ /dev/null @@ -1,71 +0,0 @@ -# 001 — search-params JSON.parse exception storm - -| | | -| --------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/searchParams.ts` (shared client+server) | -| Benchmarks | ssr (react) **and** client-nav (react) | -| Expected impact | **Large** — validated −42% SSR request time; `stringifyValue` is 5.7% self-time of the client-nav profile and the largest single router-core frame | -| Confidence | High (CPU-profiled on both benchmarks; SSR fix validated experimentally with byte-identical HTML output) | -| Risk | Low | -| Prior art | `origin/flo/search-params-json-parse-guard` (**most recent**, includes review feedback + tests), `origin/refactor-router-core-stringify-parse-search-json-perf` (older alternative) — **inspect/rebase these before writing new code** | - -## Problem - -The default search serializers use "try to JSON.parse it" as a type probe for every string value: - -`searchParams.ts:63-81` (`stringifyValue` inside `stringifySearchWith`): - -```ts -} else if (hasParser && typeof val === 'string') { - try { - // Check if it's a valid parseable string. - // If it is, then stringify it again. - parser(val) - return stringify(val) - } catch (_err) { - // silent - } -} -``` - -`parseSearchWith` (`searchParams.ts:~31-40`) has the symmetric `try { query[key] = parser(value) } catch {}` per string value. - -`parser` is `JSON.parse`. For any ordinary string (`'all'`, `'group-0'`, `'updater-2'`, `'q-abc123'`), **a real SyntaxError is constructed, thrown, and swallowed**. V8 error construction + stack capture ≈ 2.5 µs vs ~43 ns for a successful parse vs ~18 ns for a regex pre-check (micro-benchmarked). - -## Why it's hot - -- `stringifySearch` runs once per `buildLocation` (`router.ts:2009`). Every mounted `` calls `buildLocation` when the location changes (`react-router/src/link.tsx:410-413`), so the client-nav benchmark (≈22 links) does ~23 stringify passes per navigation, each throwing for every plain-string param. `parseSearch` adds a parse+stringify round trip per `parseLocation` (`router.ts:1309-1310`). -- On the server, each SSR request renders all Links → measured **35.0% self time** for `stringifyValue` in the SSR benchmark profile (1140 ms of 3.26 s over 3000 requests). -- Thrown `Error` objects also feed GC (8.6% of client profile). - -## Validated result - -An ESM-loader patch applying the pre-check to the built SSR bench: **0.957 → 0.557 ms/request (−42%)**, HTML byte-identical. Client micro-bench of `encode()` on the benchmark's search shape: 714 ms → 20.6 ms per 100k calls (~35×). - -## Proposed approach - -Add a cheap first-character pre-check before attempting `JSON.parse`. A string can only be valid JSON if its first non-whitespace char is one of `" { [ - 0-9 t f n` (true/false/null): - -1. In `stringifyValue`: skip the `parser(val)` probe (and return `val` directly) when the pre-check fails. -2. In `parseSearchWith`'s value loop: keep the string as-is when the pre-check fails. -3. Strings that _pass_ the pre-check but are still invalid JSON (`'1abc'`, `'true123'`) keep the existing try/catch — behavior is byte-identical. -4. **Scope the fast path to the default JSON implementations** (build it into `defaultParseSearch`/`defaultStringifySearch`, or gate on `parser === JSON.parse`) so custom serializers (JSURL, zipson…) keep exact semantics. - -Note: leading whitespace must count as "could be JSON" (`JSON.parse(' 1')` succeeds), so test the first _non-whitespace_ char or include `\s` in the class: `/^[\s"{[\-\dtfn]/`. - -## Risks & constraints - -- Pre-check must be a strict **superset** of valid JSON starts — never reject a parseable string. -- ~40-60 bytes added to shared client code; CI gzip check applies. This is the one finding where a tiny bundle increase is clearly worth it (it also cuts client CPU), but run the bundle benchmark and report the delta. -- An optional second step explored during the audit — replacing `URLSearchParams` in `qss.ts encode` with manual `encodeURIComponent` concatenation — changes space encoding (`+` vs `%20`) and should be treated as a separate, lower-priority investigation. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit # searchParams.test.ts has coverage; the prior-art branch adds ~75 lines of tests -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Expect a large, unambiguous SSR win and a visible client-nav win. The SSR bench must run with `NODE_ENV=production`. diff --git a/perf-investigations/002-match-routes-lightweight-memo.md b/perf-investigations/002-match-routes-lightweight-memo.md deleted file mode 100644 index 4a6706dd33..0000000000 --- a/perf-investigations/002-match-routes-lightweight-memo.md +++ /dev/null @@ -1,67 +0,0 @@ -# 002 — memoize `matchRoutesLightweight` per location - -| | | -| --------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/router.ts` (shared client+server) | -| Benchmarks | client-nav (react) **and** ssr (react) | -| Expected impact | **Large** (client: removes ~22/23 of per-navigation match/validate/param work; SSR: validated additional −7% on top of 001) | -| Confidence | High (profiled; SSR variant validated experimentally with byte-identical output) | -| Risk | Medium-low | -| Prior art | `origin/flo/current-location-lightweight-cache` (commits `b745f7d39f` + fix `b845204fa7`) — same idea, stale vs current main; **rebase it rather than re-implementing**. Local branch `flo/router-current-location-cache-guard` may contain related guard work. | - -## Problem - -Every `buildLocation` call starts by re-deriving the _current_ location's matches: - -- `router.ts:1829-1841` — `build()` does `const lightweightResult = this.matchRoutesLightweight(currentLocation)`. -- `router.ts:1722-1789` — `matchRoutesLightweight` runs: - - `this.getMatchedRoutes(location.pathname)` (LRU-cached match, but still `trimPathRight` + `Object.create(null)` + `Object.assign(routeParams, match.rawParams)` per call, `router.ts:3092-3100`), - - `const accumulatedSearch = { ...location.search }` plus the **user `validateSearch` for every matched route** (`router.ts:1743-1753`), - - a params-reuse check against `stores.matchesId`/match stores, or a fresh `extractStrictParams` chain re-running user `params.parse` (`router.ts:1756-1781`). - -Every mounted `` subscribes to `router.stores.location` with `prev.href === next.href` equality (`react-router/src/link.tsx:403-407`) and recomputes `router.buildLocation({ _fromLocation: currentLocation, ..._options })` when it changes (`link.tsx:410-413`). All links pass the **same** location object (the store value), and `router.navigate` itself uses `this.latestLocation`. - -## Why it's hot - -- client-nav: ~22 links + 1 navigate = **~23 byte-identical `matchRoutesLightweight` computations per navigation** (~230 per benchmark iteration). The user's `validateSearch` runs ~23× per navigation for the current location alone — in real apps this is often a zod schema, so the real-world cost is much higher than the benchmark shows. -- Profile share (client): `build` 61.8 ms + `matchRoutesLightweight` 40.1 ms + `getMatchedRoutes` 27.2 ms in a 5.4 s profile, plus a large share of the 8.6% GC. -- SSR: 10 Links per request → 10 identical computations; `getMatchedRoutes` alone was 4.7% self post-001. Validated patch: **0.557 → 0.516 ms/request**. - -## Proposed approach - -Single-entry memo on the router instance keyed by reference equality: - -```ts -private _lightweightCache?: { - location: ParsedLocation - lastMatchId: string | undefined // last(this.stores.matchesId.get()) - result: LightweightResult -} -``` - -- Hit when `location` is reference-equal **and** `last(this.stores.matchesId.get())` is unchanged (the result's `canReuseParams` branch at `router.ts:1756-1781` reads match-store state, so the key must cover it). -- Location objects are immutable and fresh per commit (`parseLocation` creates a new object), so reference identity is a sound key. -- Sharing the result object across callers is safe today: `build()` defensively copies params before mutating (`router.ts:1876-1879` `Object.assign(Object.create(null), lightweightResult.params)`) and treats `fromSearch` as read-only (`nullReplaceEqualDeep` input at `router.ts:2006`). Re-verify this when rebasing. -- Invalidate (or simply rely on key mismatch) when the route tree is swapped (`update()`/HMR). - -The prior-art branch additionally replaced the returned `search` with `currentLocation.search` — verify that's still correct after the retained-search fixes that landed later on main (`df1076c03a`, `7a83e67e65`); the follow-up commit `b845204fa7` on that branch ("preserve validated search in buildLocation") addresses exactly this, so read it. - -## Risks & constraints - -- Impure user `validateSearch` would now run once per navigation instead of N times — arguably a fix, but observable. -- Stale-cache bugs surface as wrong relative-link targets; cover with the existing link/buildLocation test suites. -- Bundle delta ≈ +10-15 lines in router-core; partially offset by deleting the dead commented-out block at `router.ts:1733-1740`. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -## Related - -- Composes with [003](003-build-middleware-chain-cache.md) (the other repeated block inside `buildLocation`) and [001](001-search-params-json-parse-exception-storm.md). -- [016](016-link-render-granularity.md) reduces how often links recompute at all. diff --git a/perf-investigations/003-build-middleware-chain-cache.md b/perf-investigations/003-build-middleware-chain-cache.md deleted file mode 100644 index 7dc5c05391..0000000000 --- a/perf-investigations/003-build-middleware-chain-cache.md +++ /dev/null @@ -1,56 +0,0 @@ -# 003 — cache `buildMiddlewareChain` per route branch - -| | | -| --------------- | -------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/router.ts` (shared client+server) | -| Benchmarks | client-nav (react), ssr (react) | -| Expected impact | Medium (allocation/GC churn removal on a ×23-per-navigation path; multiplies with 002) | -| Confidence | High — the code carries an explicit TODO asking for exactly this | -| Risk | Low-medium (re-entrancy detail) | -| Prior art | none found | - -## Problem - -Every `buildLocation` call rebuilds the entire search-middleware closure chain: - -- `router.ts:3111-3124` — `applySearchMiddleware` calls `buildMiddlewareChain(destRoutes)` unconditionally. -- `router.ts:3126-3230` — `buildMiddlewareChain` loops over all destRoutes, reading `route.options` (`'search' in routeOptions`, `preSearchFilters`, `validateSearch`) and allocating a `middlewares` array plus fresh `validate`/`legacyMiddleware`/`applyNext`/`next` closures per call — even for links with zero middleware. - -The code already anticipates the fix (`router.ts:3107-3110`): - -``` -TODO: once caches are persisted across requests on the server, -we can cache the built middleware chain using `last(destRoutes)` as the key -``` - -That precondition is now met: router caches **are** persisted cross-request via `globalThis.__TSR_CACHE__` (`router.ts:1121-1149`). - -The returned chain is already written to be reusable — it is parameterized per invocation via `middleware(search, dest, includeValidateSearch)` with mutable captured vars assigned at `router.ts:3221-3229`. - -## Why it's hot - -Runs inside every `buildLocation`: ~23×/navigation on client-nav (22 links + navigate), once per Link per SSR request. Pure repeated allocation; user code isn't the cost, the closure construction is. SSR profile: ~0.4% self + GC share. - -## Proposed approach - -1. Cache the built chain in a `WeakMap` keyed by **branch array identity** (`WeakMap, chain>`). `destRoutes` identity is stable in the hot paths: `getRouteBranch` memoizes branches (`router.ts:1386-1393`) and `findRouteMatch` caches `result.branch` (`new-process-route-tree.ts:755`). The notFound case spreads a fresh array (`router.ts:1923`) and will simply rebuild — fine. - - Alternative per the TODO: key on `last(destRoutes)` — but the appended `notFoundRoute` leaf can be shared across branches; array-identity keying avoids that hazard. -2. While here, consider removing the closure-captured mutable `let dest / includeValidateSearch` (`router.ts:3127-3128`) and threading them as arguments through `applyNext`, making the cached chain re-entrant (protects against a user middleware that synchronously calls `buildLocation` again). - -## Risks & constraints - -- **Re-entrancy**: today each call gets fresh closures; a cached chain with captured mutable state must not be invoked recursively (middleware → `buildLocation` → same chain). Step 2 eliminates this class of bug; if skipped, document/guard. -- Route objects are recreated on HMR/`update()`, so a WeakMap keyed on routes/branches self-invalidates. -- Users mutating `route.options.search.middlewares` at runtime would see stale chains — not a supported pattern. -- Bundle delta ≈ +5 lines. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -The win is mostly GC/allocation; expect a small direct delta on client-nav, clearer when stacked on [002](002-match-routes-lightweight-memo.md). diff --git a/perf-investigations/004-load-matches-sync-fastpaths.md b/perf-investigations/004-load-matches-sync-fastpaths.md deleted file mode 100644 index c0db3b1d43..0000000000 --- a/perf-investigations/004-load-matches-sync-fastpaths.md +++ /dev/null @@ -1,49 +0,0 @@ -# 004 — sync fast paths in the loader phase of `loadMatches` - -| | | -| --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/load-matches.ts`, `router.ts` (shared client+server) | -| Benchmarks | client-nav (react), ssr (react) | -| Expected impact | Medium-large — shortens the microtask chain in front of every render commit | -| Confidence | High | -| Risk | Medium (error/redirect propagation paths) | -| Prior art | `origin/flo/load-matches-sync-fastpaths` (commits `14ecec2e52`, `4fc676c334`, `9be80233eb` — incl. a fix preserving sync beforeLoad abortController and store-update-count test adjustments). **Rebase/extend this branch rather than starting fresh.** Follows up on merged #7582 ("avoid creating promises when not necessary"), which only removed 4 trivial `Promise.resolve()`s. | - -## Problem - -The beforeLoad phase already uses a sync-continuation pattern (`handleBeforeLoad`, `load-matches.ts:533-558`, only awaits when `isPromise(beforeLoad)`), but the loader phase is unconditionally async: - -- `load-matches.ts:784` — `loadRouteMatch` is `async`. -- `load-matches.ts:789-854` — nested `async function handleLoader` is **re-created per match per navigation** (closure over `matchId`, `loaderShouldRunAsync`, …). -- `load-matches.ts:641` — `runLoader` is `async`. -- `load-matches.ts:972, 1025-1030` — every match pushes a promise into `matchPromises`, then `await Promise.all(matchPromises)`. -- `router.ts:2486` — `onReady: async () => {...}` has a fully synchronous body; the `async` keyword forces `triggerOnReady` (`load-matches.ts:1171-1174`) to await a promise. - -When all loaders are sync (the benchmark; also the common cache-hit case in real apps), this is 3 async-function frames per match — each a promise allocation + 1-2 microtask hops — and the React commit the benchmark waits for is queued behind all of them. Depth-2/3 match trees × 10 navigations per iteration. - -## Proposed approach - -1. Convert `loadRouteMatch` to return `AnyRouteMatch | Promise` using the same `isPromise` continuation style as `handleBeforeLoad`. Only push actual promises into `matchPromises`; skip `Promise.all` entirely when none are. -2. Hoist `handleLoader` to module scope with explicit args (also removes one closure per match per navigation). The `serverSsr`/`execute`/`queueExecution` closures in `handleBeforeLoad` (`load-matches.ts:540-555`) can be similarly flattened on the client, where `isServer` is statically `false` and dead-code-eliminated. -3. Make `onReady` (`router.ts:2486`) a plain sync function — `triggerOnReady` already handles a `void` return. - -## Risks & constraints - -- `getLoaderContext` (`load-matches.ts:614`) passes `matchPromises[index - 1]` as `parentMatchPromise` to loaders. When a loader actually runs it must still receive a promise — lazily wrap with `Promise.resolve(match)`. -- Error propagation: `handleRedirectAndNotFound` throws; the sync path needs equivalent try/catch placement so redirects/notFound behave identically (the existing branch has test updates around this). -- Solid/vue store-update-count tests assert exact store write counts; the prior-art branch already adjusted them (`9be80233eb`) — those frameworks are out of scope for changes but their tests still run. -- Code size roughly neutral (restructure, not addition) but verify with the bundle benchmark. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -## Related - -- [005](005-loaderless-runloader-fast-path.md), [006](006-update-match-write-coalescing.md), [008](008-lazy-controlled-promise.md) touch the same call sites — coordinate. -- [022](022-commit-path-micro-allocations.md) covers the analogous de-async work in `navigate`/`commitLocation`. diff --git a/perf-investigations/005-loaderless-runloader-fast-path.md b/perf-investigations/005-loaderless-runloader-fast-path.md deleted file mode 100644 index ff9e22d797..0000000000 --- a/perf-investigations/005-loaderless-runloader-fast-path.md +++ /dev/null @@ -1,50 +0,0 @@ -# 005 — skip `runLoader` for routes that have no loader - -| | | -| --------------- | --------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/load-matches.ts` (shared client+server) | -| Benchmarks | client-nav (react) | -| Expected impact | Medium-large — removes one full match-store write + context rebuild + detached async chain per entered match per navigation | -| Confidence | High (branch conditions verified against the benchmark's route options) | -| Risk | Medium (must enumerate everything `runLoader` does besides calling the loader) | -| Prior art | none found; adjacent to `origin/flo/load-matches-sync-fastpaths` ([004](004-load-matches-sync-fastpaths.md)) | - -## Problem - -The stale-while-revalidate path fires `runLoader` even when the route has **no loader at all**: - -- `load-matches.ts:802` — `staleAge` defaults to 0, so for a `status === 'success'` match, `staleMatchShouldReload` (`load-matches.ts:818-823`) is true whenever `cause === 'enter'` or the route at that position changed — i.e. every `/items/1 → /items/2` style navigation. -- `load-matches.ts:835-848` — `loaderShouldRunAsync` triggers a fire-and-forget IIFE running `runLoader` **without checking `route.options.loader` exists**. -- `runLoader` (`load-matches.ts:641-782`) then: calls `loadRouteChunk(route)` (`:660`), finds no loader, and still finishes with a full `updateMatch` that re-spreads the ~35-field match object, rebuilds `context` via `buildMatchContext`, and stamps `updatedAt: Date.now()` (`:717-724`) — plus the IIFE's own promise chain resolving `loaderPromise`/`loadPromise` in detached microtasks (`:838-842`). - -In the client-nav benchmark, the `/items/$id`, `/items/$id/details`, and `/ctx/$id` routes are all loaderless, so this path runs ~1-2× per navigation. Real apps with layout/param routes without loaders pay the same on every param change. - -## Proposed approach - -In `handleLoader`, add a fast path when **all** of: - -- `!route.options.loader` -- no lazy route module pending (`route._lazyPromise`/`_lazyLoaded` state — a `lazyFn` could still add a loader) -- components already loaded (`route._componentsPromise`/`_componentsLoaded`) -- no `match._nonReactive.minPendingPromise` - -→ perform the status/`updatedAt`/context update synchronously, merged with the `syncMatchContext` write, instead of invoking `runLoader`. - -Also handle the **blocking** branch (`load-matches.ts:849-850`): loaderless routes whose status is `'pending'` (e.g. a route with only `beforeLoad`, like the benchmark's `/ctx/$id`) go through `await runLoader(...)` and need the same treatment. - -## Risks & constraints - -- `runLoader` is responsible for more than the loader: `route._lazyPromise`, `_componentsPromise` preloading, `errorComponent` head preloading, `updatedAt` freshness, status transitions. The fast path must be provably equivalent for the loaderless case — enumerate each side effect and reproduce or consciously skip it. -- `updatedAt` semantics feed staleness decisions on the next navigation; keep stamping it. -- Coordinate with [004](004-load-matches-sync-fastpaths.md) and [006](006-update-match-write-coalescing.md) — overlapping call sites; implementing together avoids double refactors. -- Bundle: restructure, roughly neutral; verify. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Watch specifically: lazy-route tests, preload tests, pending-component tests. diff --git a/perf-investigations/006-update-match-write-coalescing.md b/perf-investigations/006-update-match-write-coalescing.md deleted file mode 100644 index ee802d41dc..0000000000 --- a/perf-investigations/006-update-match-write-coalescing.md +++ /dev/null @@ -1,48 +0,0 @@ -# 006 — coalesce per-match store writes (`pending`/`resolve`/`syncMatchContext`) and reuse AbortControllers - -| | | -| --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/load-matches.ts`, `router.ts` (shared client+server) | -| Benchmarks | client-nav (react) | -| Expected impact | **Large** in combination — fewer 35-field object copies, fewer store flushes, an order of magnitude fewer React transition state updates per navigation (with [007](007-transitioner-transition-refcount.md)) | -| Confidence | High | -| Risk | Medium (observable `isFetching`/`fetchCount` semantics) | -| Prior art | none found directly; `origin/refactor-router-core-ready-transition-performance` touches the adjacent onReady/transition area | - -## Problem - -On the fully-sync / cache-hit path, each match still receives 3-5 separate store writes per navigation, each a full spread of the ~35-field match object, and each routed through `router.startTransition`: - -- `load-matches.ts:418-447` — `executeBeforeLoad` calls `pending()` then `resolve()` **even when `route.options.beforeLoad` is absent**: `pending()` spreads the match with `fetchCount + 1`, a `new AbortController()`, `isFetching: 'beforeLoad'`; `resolve()` spreads again with `isFetching: false`. -- `load-matches.ts:191-204` — `syncMatchContext` always writes a brand-new `context` object (identity churn for `useRouteContext` subscribers). -- Additional writes at `load-matches.ts:683-687`, `:700-704`, `:717-724` (loader phase), `:927-931`, `:949-955`. -- `router.ts:2698-2727` — every `updateMatch` wraps its write in `this.startTransition`, which on React is two `setState` calls on the Transitioner (see [007](007-transitioner-transition-refcount.md)). ~8-12 `updateMatch` calls per navigation → ~20 React state updates, in separate microtask continuations so they don't batch. -- `load-matches.ts:415` — a fresh `AbortController` per match per navigation even though the previous one was never aborted (the fast path completes synchronously; `cancelMatches`, `router.ts:1801-1820`, only aborts pending/loading matches). New matches even get two (`router.ts:1624` in `matchRoutesInternal` + `executeBeforeLoad`). In jsdom (the benchmark environment) AbortController/AbortSignal are pure-JS event-target objects, notably more expensive than native. - -Writes during loading go to pending-pool atoms with no component subscribers, but still dirty the computed graph (`stores.ts:146-158` `pendingMatches`/`hasPending`) and flush effects. - -## Proposed approach - -1. **No-beforeLoad collapse**: when `!route.options.beforeLoad`, replace `pending()+resolve()` with a single write (`{...prev, fetchCount: prev.fetchCount + 1, abortController}` — the `isFetching` round-trip nets out within the batch). Keep the two-phase writes when `beforeLoad` is genuinely async (its `isFetching: 'beforeLoad'` state is observable by `useMatch` subscribers mid-load). -2. **`syncMatchContext` bail-out**: shallow-compare the rebuilt context against `prev.context` and skip the write when equal — also stabilizes context identity for `useRouteContext` selectors (enables [019](019-match-identity-reuse.md)). -3. **AbortController reuse**: keep `match.abortController` when `!signal.aborted`; allocate a new one only after an actual abort. Drop the duplicate eager creation for brand-new matches (`router.ts:1624`) or create lazily when beforeLoad/loader exists. -4. **Transition dedup** belongs to [007](007-transitioner-transition-refcount.md) (react side); alternatively add a core-side "already in transition" flag set by `load()` (`router.ts:2454`) so nested `updateMatch` calls skip the wrapper. - -## Risks & constraints - -- `fetchCount` increments on every navigation today, even without a fetch — merging writes preserves that; _skipping_ writes would change it. -- Loaders/beforeLoad receive `abortController` in context; a reused signal spans multiple fetches of the same match. That matches the documented intent (abort on navigate-away), but tests may assert fresh controllers — check `load-matches`-related tests. The error path (`load-matches.ts:242`) deliberately resets the controller; keep that. -- Solid/vue adapters install their own `startTransition`; any core-side flag must be adapter-agnostic. Their store-update-count tests will need count adjustments (precedent: `9be80233eb` on the 004 branch). - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -## Related - -- Implement together with [004](004-load-matches-sync-fastpaths.md)/[005](005-loaderless-runloader-fast-path.md) (same call sites). -- [019](019-match-identity-reuse.md) only pays off after this lands. diff --git a/perf-investigations/007-transitioner-transition-refcount.md b/perf-investigations/007-transitioner-transition-refcount.md deleted file mode 100644 index cac8bd3feb..0000000000 --- a/perf-investigations/007-transitioner-transition-refcount.md +++ /dev/null @@ -1,52 +0,0 @@ -# 007 — ref-count Transitioner `setIsTransitioning` instead of toggling per `updateMatch` - -| | | -| --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| Area | `packages/react-router/src/Transitioner.tsx`, `packages/router-core/src/router.ts` | -| Benchmarks | client-nav (react) | -| Expected impact | Medium — React update processing (`updateReducerImpl`) is the single largest frame in the client profile at 15.8%; Transitioner churn is a significant driver beyond legitimate content re-renders | -| Confidence | Medium-high (mechanism verified; exact attribution within the 15.8% not isolated) | -| Risk | Medium (`onResolved` emission ordering) | -| Prior art | solid got an analogous cleanup in #7584 (`41e7a24f69`); React's side untouched. `origin/refactor-router-core-ready-transition-performance` (`cd6db1f5dc`) reworks the adjacent onReady/transition code in router.ts — read it first. | - -## Problem - -`Transitioner.tsx:26-32` installs: - -```ts -router.startTransition = (fn) => { - setIsTransitioning(true) - React.startTransition(() => { - fn() - setIsTransitioning(false) - }) -} -``` - -and `router.ts:2698-2727` routes **every** `updateMatch` through `this.startTransition`. With 3-8 `updateMatch` calls per navigation (`load-matches.ts:421,435,505,683,700,717` …) plus `load()` itself (`router.ts:2454`), each navigation produces ~8-14 `startTransition` invocations = ~16-28 React `setState` calls on the Transitioner. They happen in separate async continuations of `loadMatches`, so they don't batch; each toggle re-renders the Transitioner and re-evaluates its 3 `useLayoutEffect`s + `usePrevious` hooks (`Transitioner.tsx:86-128`), and oscillates `isAnyPending`. - -## Proposed approach - -1. **Ref-count edges**: keep a depth counter in a ref inside the Transitioner-installed `startTransition`; call `setIsTransitioning(true)` only on the 0→1 edge and `setIsTransitioning(false)` only when the count returns to 0 (checked inside the transition callback). The boolean is only consumed as "any transition pending", so edge-triggering preserves observable semantics exactly. -2. Alternative (riskier): stop routing `updateMatch` through the bookkeeping wrapper entirely and use bare `React.startTransition` for match writes — but `isTransitioning` gates `onResolved` emission, so semantics need careful verification. - -## Risks & constraints - -- `isTransitioning` feeds `isAnyPending` → `onResolved`/`onBeforeRouteMount` emission timing (`Transitioner.tsx:86-128`). The client-nav benchmark itself awaits `onRendered`/`onResolved` — the final 1→0 edge must fire at the same point as today. The ref-count variant is the one that preserves this. -- Tests exist around `onResolved` ordering; run react-router unit tests. -- Bundle delta: a few bytes (one ref + two comparisons). - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Use the flame benchmark (`test:flame:react`) before/after: the win should show as a reduction in `updateReducerImpl`/commit frames. - -## Related - -- [006](006-update-match-write-coalescing.md) reduces how many `updateMatch` calls happen at all; this file makes each remaining one cheaper on the React side. Independent but complementary. -- [015](015-link-flushsync-click.md) covers the Link-side `flushSync(setIsTransitioning)` per click. diff --git a/perf-investigations/008-lazy-controlled-promise.md b/perf-investigations/008-lazy-controlled-promise.md deleted file mode 100644 index e2bd703893..0000000000 --- a/perf-investigations/008-lazy-controlled-promise.md +++ /dev/null @@ -1,44 +0,0 @@ -# 008 — create `ControlledPromise`s lazily on the sync path - -| | | -| --------------- | ---------------------------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/router.ts`, `load-matches.ts`, `utils.ts` (shared client+server) | -| Benchmarks | client-nav (react) | -| Expected impact | Medium (60-90 controlled-promise allocations per benchmark iteration eliminated) | -| Confidence | Medium-high (beforeLoadPromise part is high; loadPromise part needs a consumer audit) | -| Risk | Medium (suspense + concurrent-load coordination) | -| Prior art | partially adjacent to `origin/flo/load-matches-sync-fastpaths` ([004](004-load-matches-sync-fastpaths.md)) | - -## Problem - -On the fully-sync fast path, each match allocates 2-3 `ControlledPromise`s per navigation (3 for brand-new matches), all resolved and nulled before `loadMatches` returns, with nothing ever awaiting them: - -- `router.ts:1620` — every new match in `matchRoutesInternal` gets `_nonReactive: { loadPromise: createControlledPromise() }`. -- `load-matches.ts:397-401` — `executeBeforeLoad` unconditionally replaces `loadPromise` with a fresh controlled promise chaining the previous one (so the constructor-time one is immediately superseded). -- `load-matches.ts:451` — `beforeLoadPromise` is created **before** knowing whether `beforeLoad` is sync. -- `load-matches.ts:925` — `loaderPromise` created on every non-skip `loadRouteMatch`. -- `utils.ts:462-486` — each `createControlledPromise` = native promise + executor closure + resolve/reject closures + status fields. - -The only consumers are React suspense (`react-router/src/Match.tsx:316/338/434/461` via `getMatchPromise`, only when status is `'pending'`/`_displayPending`) and overlapping-load coordination (`load-matches.ts:364-368, 893-905`). - -## Proposed approach - -1. **`beforeLoadPromise`**: create it only inside the `isPromise(beforeLoadContext)` branch (`load-matches.ts:516`) — move creation after the call; sync callers never observe it. (Highest confidence, smallest diff.) -2. **`loaderPromise`**: create only when `handleLoader` decides async work will actually happen; it's needed by concurrent `loadRouteMatch` calls only once a load is genuinely in flight. -3. **Constructor-time `loadPromise`** (`router.ts:1620`): audit consumers — it's superseded at `:397-401` on every load; either drop the eager creation or create lazily via a `getMatchPromise`-side fallback. Must guarantee existence whenever a match can render as `'pending'` (suspense reads it). - -## Risks & constraints - -- Overlap detection: a second `loadMatches` (preload or rapid double navigation) checks `prevMatch._nonReactive.loaderPromise` (`load-matches.ts:893`) to detect in-flight loads. Creating it later shrinks but doesn't eliminate the window (everything before the first await is synchronous) — verify the preload/navigate race tests. -- Suspense: `Match.tsx` reads `loadPromise` while status is `'pending'`; the lazy path must cover `_displayPending`/`pendingComponent` scenarios. -- Bundle delta neutral-ish; verify. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Focus tests: suspense/pending-component, preload, concurrent navigation. diff --git a/perf-investigations/009-parse-location-reuse-built-location.md b/perf-investigations/009-parse-location-reuse-built-location.md deleted file mode 100644 index 754031a138..0000000000 --- a/perf-investigations/009-parse-location-reuse-built-location.md +++ /dev/null @@ -1,58 +0,0 @@ -# 009 — `parseLocation` reuses the location `navigate` just built - -| | | -| --------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/router.ts` (shared client+server) | -| Benchmarks | client-nav (react); small SSR benefit | -| Expected impact | Small-medium (once per navigation, but it's the densest string-processing block in the commit path; combos with 001) | -| Confidence | Medium (a previous broader attempt was reverted — see history) | -| Risk | Medium-high — **this exact area has a revert in its history**; keep the change strictly guarded | -| Prior art | #6398 "don't reparse upon navigation" was **reverted in #6468**. Read both PRs before starting. This proposal is deliberately a much narrower, byte-equality-guarded variant. | - -## Problem - -A navigation builds a complete `ParsedLocation`, commits it to history, and then immediately re-derives the same thing from the href: - -1. `buildLocation` produces `{href, publicHref, pathname, search, searchStr, hash, state}`; `buildAndCommitLocation` stores it as `this.pendingBuiltLocation` (`router.ts:2282`). -2. `commitLocation` pushes only `publicHref` + state into history (`router.ts:2226-2230`). -3. The history subscriber (`react-router/src/Transitioner.tsx:37`) calls `router.load()` → `beforeLoad()` → `updateLatestLocation()` (`router.ts:1220-1225`) → `parseLocation(this.history.location)` (`router.ts:1294-1374`), which re-runs `parseSearch` (URLSearchParams decode + JSON.parse attempts incl. thrown exceptions, see [001](001-search-params-json-parse-exception-storm.md)), re-runs `stringifySearch` for normalization (`router.ts:1309-1310`), `decodePath` ×2, `nullReplaceEqualDeep` on search and `replaceEqualDeep` on state — reconstructing what `pendingBuiltLocation` already holds. - -Timing is favorable: history `notify` fires synchronously inside `commitLocation`, before the `queueMicrotask` at `router.ts:2297-2301` clears `pendingBuiltLocation`, and `beforeLoad` runs synchronously inside `load()`'s `startTransition`. - -It also breaks structural identity: the re-parsed `search` is a fresh object, re-shared via `nullReplaceEqualDeep` against the _previous_ location rather than reusing the built object. - -## Proposed approach - -A guarded fast path in `updateLatestLocation` (or a dedicated branch in `parseLocation`): - -``` -if (this.pendingBuiltLocation - && this.pendingBuiltLocation.href === this.history.location.href - && !this.history.location.state.__tempLocation - && !this.rewrite) { - // reuse pathname/search/searchStr/hash/href/publicHref from the built location; - // take only state via replaceEqualDeep(previous?.state, history.location.state) - // (history adds __TSR_key/__TSR_index) -} -``` - -Fall back to the full parse for: masked locations (masked commits store `__tempLocation` state, `router.ts:2190-2217`), rewrites/external URLs, any href mismatch, and back/forward events (no pending built location). - -Alternative lighter form: an LRU of `rawSearchString → {search, searchStr}` inside the router, which also dedupes repeated parses of equal strings across history events — less reuse but fewer preconditions. - -## Risks & constraints - -- **Why the previous attempt was reverted**: read #6468's description/tests and make those cases explicit fall-backs. Keep the byte-equality guard (`href` exact match) so non-canonical hrefs still go through re-canonicalization. -- Custom `createHref`/browser history mutating the committed href: the post-roundtrip `history.location.href` comparison handles this (memory history's `createHref` is identity, `history/index.ts:628`). -- `parseSearch` impurity: search would skip the parse round-trip; the built object was produced by validators/middleware — which is what `matchRoutes` consumes anyway — but only behind exact href equality. -- Async history adapters: verify `pendingBuiltLocation` lifetime holds (it's cleared on a microtask). -- Bundle delta ≈ +15 guarded lines in router-core. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit -``` - -Focus tests: location masking, rewrites/basepath, back/forward, custom parseSearch/stringifySearch, hash handling. Find and re-run whatever test motivated the #6468 revert. diff --git a/perf-investigations/010-hook-selector-slice-memoization.md b/perf-investigations/010-hook-selector-slice-memoization.md deleted file mode 100644 index 36fe32e262..0000000000 --- a/perf-investigations/010-hook-selector-slice-memoization.md +++ /dev/null @@ -1,49 +0,0 @@ -# 010 — skip user `select` when the extracted match slice is referentially unchanged - -| | | -| --------------- | ---------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/react-router/src/` hooks (`useParams`, `useSearch`, `useLoaderData`, `useLoaderDeps`, `useRouteContext`, `useMatch`) | -| Benchmarks | client-nav (react) | -| Expected impact | Medium (the benchmark's 52 hook subscribers run deliberately-expensive selectors; real apps run arbitrary user code per store set) | -| Confidence | High on mechanism; medium on benchmark magnitude | -| Risk | Medium (stale-select hazards; must key on select identity) | -| Prior art | continues the consolidation from merged #7577 (`689d88e04c`, shared hook structuralSharing logic) | - -## Problem - -Every derived hook funnels to `useMatch` with a _composed_ selector: - -- `useParams.tsx:101-105`, `useSearch.tsx:101-103`, `useLoaderData.tsx:87-89`, `useLoaderDeps.tsx:65-67`, `useRouteContext.ts:27-28` — each wraps as `(match) => { const slice = match.X; return opts.select ? opts.select(slice) : slice }`. -- On **every** match-store `set`, `useSyncExternalStoreWithSelector` re-runs this selector for every subscriber (shared path `useMatch.tsx:53-66` `useStructuralSharing`, `useMatch.tsx:182-198`). - -But router-core deliberately stabilizes the slices: `match.params`/`match.search`/`loaderDeps` go through `replaceEqualDeep`/`nullReplaceEqualDeep` against the previous match (`router.ts:1586-1588`, `1627-1629`, `1667-1669`). So when a match object is replaced (new `cause`/`updatedAt`/identity) but the slice reference is unchanged, all subscribers still re-run their user `select` for nothing. In the benchmark: 20 root-level subscribers (strict:false `useParams`/`useSearch`, each running a 40-iteration computation) × every root-match set (≥1/navigation, more with post-commit context/loaderData updates), plus 6-18 route-level subscribers. - -## Proposed approach - -Refactor the hooks to pass `extract` (slice getter) and `select` separately to `useMatch`, then extend `useStructuralSharing` (or a sibling helper) to memoize: - -``` -slice = extract(match) -if (slice === prevSlice && select === prevSelect) return prevResult -prevResult = structuralSharing(select ? select(slice) : slice) -``` - -stored in the existing `previousResult` ref. Likely bundle-neutral or negative: it removes five near-identical wrapper closures. - -## Risks & constraints - -- A user `select` closing over changing component values must not get a stale cached result. Keying on `select` **identity** handles this: inline selects get a new identity each render, so the cache only short-circuits _store notifications between renders_ — exactly the safe hot path (same trick React Query uses). Users passing memoized selectors that close over other changing values were already broken under `useSyncExternalStoreWithSelector`'s own memoization assumptions. -- `shouldThrow`/undefined-match path must bypass the cache (see [011](011-use-match-invariant-throw.md)). -- Navigations that genuinely change the slice (e.g. `params.id`) must still re-run select — guaranteed by the reference check. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -## Related - -- [019](019-match-identity-reuse.md) (match identity reuse) attacks the same waste one level lower — if the match object itself keeps identity, stores don't notify at all. Both are worthwhile; this one also covers post-commit sets that legitimately replace the match. diff --git a/perf-investigations/011-use-match-invariant-throw.md b/perf-investigations/011-use-match-invariant-throw.md deleted file mode 100644 index 2131966e00..0000000000 --- a/perf-investigations/011-use-match-invariant-throw.md +++ /dev/null @@ -1,59 +0,0 @@ -DONE IN https://github.com/TanStack/router/pull/7595 - -# 011 — `useMatch` selector throws (and React swallows) an Error per unmounting subscriber per navigation - -| | | -| --------------- | -------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/react-router/src/useMatch.tsx` | -| Benchmarks | client-nav (react) | -| Expected impact | Small-medium (~1% of client profile + GC), very low risk | -| Confidence | High (profiled: `invariant` 50.8 ms self in a 5.4 s profile; mechanism verified in React's `checkIfSnapshotChanged`) | -| Risk | Low | -| Prior art | none found | - -## Problem - -`useMatch.tsx:182-198` — the selector passed to `useStore` throws when the match is missing: - -```ts -return useStore(matchStore ?? dummyStore, (match) => { - if (!match) { - if (opts.shouldThrow ?? true) { - ... - invariant() // throws Error('Invariant failed') - } - return undefined - } - return selector(match as any) -}) -``` - -When navigating away from a route (e.g. `/search` → `/items`), the subscribed match store resolves to `undefined` during the commit. The store notification re-runs each hook's selector via React's `checkIfSnapshotChanged`; the selector throws, **React's try/catch swallows the error and forces a re-render**, and the component unmounts before any render observes the throw. The Error (with stack capture) is pure overhead. - -In the benchmark: ~30 route-level subscribers on `/search` (10×3 hooks) and ~12 on `/ctx` each construct+throw+catch an Error on the navigation that removes them — every iteration. - -## Proposed approach - -Return a sentinel (or `undefined`) from the selector when the match is missing, and move the `shouldThrow` invariant into the hook body, applied to the value returned by `useStore` (i.e. on the render path): - -- Renders with a genuinely missing match still throw, with identical public behavior and message. -- Transient store notifications on about-to-unmount subscribers stop allocating/throwing. - -Needs a sentinel distinct from a legitimate `undefined` selector result (a module-level `const MISSING = Symbol()` or object) so `shouldThrow: false` keeps returning `undefined` to the caller while the hook can distinguish "missing match" from "select returned undefined". - -## Risks & constraints - -- Preserve `shouldThrow: false` → `undefined` exactly. -- The dev-mode descriptive error message (`useMatch.tsx:185-189`) must still be thrown from the same user-visible point (component render). -- Solid/vue have analogous code; out of scope here but note for parity. -- Bundle delta ≈ neutral (moves a branch). - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Focus tests: `useMatch`/hooks with `shouldThrow` both values, hooks used outside a matching route, unmount-during-navigation. diff --git a/perf-investigations/012-outlet-match-selector-compares.md b/perf-investigations/012-outlet-match-selector-compares.md deleted file mode 100644 index 269d994ac3..0000000000 --- a/perf-investigations/012-outlet-match-selector-compares.md +++ /dev/null @@ -1,47 +0,0 @@ -# 012 — `Outlet`/`Match`/`MatchInner` re-render on every match-store set (fresh-tuple selectors, `===` compare) - -| | | -| --------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/react-router/src/Match.tsx` | -| Benchmarks | client-nav (react) | -| Expected impact | Small-medium (one Outlet+Match+MatchInner per active level × every active-store set; `MatchView` rebuilds ~6 boundary elements + closures per re-render) | -| Confidence | High | -| Risk | Low-medium (under-selection of fields read in throw paths) | -| Prior art | none found | - -## Problem - -- `Match.tsx:524-527` (Outlet): - ```ts - const [routeId, parentGlobalNotFound] = useStore( - parentMatchStore, - (match) => [match?.routeId, match?.globalNotFound ?? false], - ) - ``` - A new array per selector run with the default `===` compare (`@tanstack/react-store` `defaultCompare`) → **every** parent match-store `set` forces an Outlet re-render even when neither field changed. -- `Match.tsx:78-80` — `Match` subscribes with an identity selector (`(value) => value`), re-rendering on every set; it only consumes `routeId`/`ssr`/`_displayPending` (memoized into `matchState` at `:82-93`), but the component render + `MatchView` element-tree rebuild (`:163-218`, ~6 boundary elements, `getResetKey`/`onCatch`/not-found `fallback` closures) still happens. -- `Match.tsx:371` — `MatchInner` likewise subscribes with identity and re-renders fully on every set (its `out` element is memoized, the render isn't). - -Each navigation performs several match-store sets per level (see [006](006-update-match-write-coalescing.md)), so with 3 active levels this is a steady stream of avoidable React renders. - -## Proposed approach - -1. **Outlet**: split into two scalar `useStore` selections, or pass a compare fn (`(a,b) => a[0]===b[0] && a[1]===b[1]`). -2. **Match**: select only the consumed fields (`routeId`, `ssr`, `_displayPending`) with a field-wise compare. -3. **MatchInner**: add a compare checking the identity of the fields actually read during render (`status`, `error`, `_displayPending`, `_forcePending`, `loaderDeps`, `_strictParams`, `_strictSearch`, …) — enumerate by reading the component body before finalizing the list. - -## Risks & constraints - -- Under-selecting a field read in a throw/suspense path is the failure mode — audit the full render body (note `match._nonReactive` is read fresh through `router.getMatch`, not via the subscription, so it's unaffected). -- Compare functions run per set per component — keep them field-identity checks only. -- Bundle delta: ± a few bytes. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Effect is amplified while [006](006-update-match-write-coalescing.md) hasn't landed (more sets today); still worthwhile after. diff --git a/perf-investigations/013-link-options-memo-stability.md b/perf-investigations/013-link-options-memo-stability.md deleted file mode 100644 index 32bfd15ad1..0000000000 --- a/perf-investigations/013-link-options-memo-stability.md +++ /dev/null @@ -1,41 +0,0 @@ -# 013 — Link `_options` memo defeated by inline `search`/`params` props - -| | | -| --------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/react-router/src/link.tsx` | -| Benchmarks | client-nav (react) | -| Expected impact | Small-medium (+2 avoidable `buildLocation` calls per click today; medium in apps whose link parents re-render often) | -| Confidence | High | -| Risk | Low | -| Prior art | `origin/perf/react-link-location-deps` explored this area (incl. removing search validation from buildLocation); its author judged the approach unsatisfactory ("i dont think this is good, just technically fun") — read it for pitfalls, don't resurrect it wholesale | - -## Problem - -- `link.tsx:385-400` — the `_options` memo's dependency array includes `options.search`, `options.params`, `options.state`, `options.mask` **by identity**. -- Typical usage (and the benchmark) passes inline literals and updater arrows: `search={{ page: 1, ... }}`, `params={{ id: itemsId }}`, `search={(prev) => ...}` — so `_options` is new on **every render**, invalidating the `next` memo (`link.tsx:410-413`) → full `buildLocation`, and recreating `doPreload`/`preloadViewportIoCallback` (`link.tsx:565-582`). -- Concretely: the clicked link re-renders twice per click from `setIsTransitioning(true/false)` (`link.tsx:620-627`) and pays two extra `buildLocation`s each time. Any parent re-render makes it O(`buildLocation` × links). - -## Proposed approach - -Stabilize `_options` with a previous-value ref doing a cheap equivalence check: - -- Keep prior dep values; compare `search`/`params`/`state`/`mask` objects via `deepEqual`/shallow compare (small POJOs — cheap; `deepEqual` is already imported in link.tsx), functions by identity only (same as today, no regression). -- Return the previous `_options` object when equivalent, so `next` truly recomputes only when `currentLocation` or real option values change. - -## Risks & constraints - -- `deepEqual` on every render costs something — these are tiny objects; ensure the compare short-circuits on reference equality first. -- Function props (updaters) can't be deep-compared; identity-only means inline updaters still invalidate — that matches current behavior, so no regression, but the win for updater-links comes from [002](002-match-routes-lightweight-memo.md)/[003](003-build-middleware-chain-cache.md) instead. -- Bundle delta small; reuse existing imports. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -## Related - -- [016](016-link-render-granularity.md) is the structural version of this fix; [013] is the low-risk subset. diff --git a/perf-investigations/014-link-isactive-client-prechecks.md b/perf-investigations/014-link-isactive-client-prechecks.md deleted file mode 100644 index c90b40f7e8..0000000000 --- a/perf-investigations/014-link-isactive-client-prechecks.md +++ /dev/null @@ -1,44 +0,0 @@ -# 014 — port the server branch's `isActive` pre-checks to the client branch of Link - -| | | -| --------------- | -------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/react-router/src/link.tsx`, possibly `packages/router-core/src/utils.ts` | -| Benchmarks | client-nav (react) | -| Expected impact | Small (deepEqual over search objects ×22 links per navigation) — but essentially free, and deduplication may _shrink_ the bundle | -| Confidence | High (the exact logic already exists and runs in the SSR branch) | -| Risk | Very low | -| Prior art | none found | - -## Problem - -Link computes `isActive` in two places with different cost profiles: - -- **Server branch** (`link.tsx:244-266`): guards the search comparison with cheap pre-checks — `if (currentLocation.search !== next.search)` reference check plus empty-object short-circuits via `hasKeys` — before calling `deepEqual`. -- **Client branch** (`link.tsx:497-505`): calls `deepEqual(currentLocation.search, next.search, {partial: ...})` **unconditionally** for every link with `includeSearch` (the default) on every location change. - -Since `next.search` is rebuilt per `buildLocation` (`nullReplaceEqualDeep` stabilizes it against the freshly built `fromSearch`, not against `currentLocation.search`), `deepEqual`'s internal `a === b` fast path (`utils.ts:371`) rarely hits, so the full walk runs for ~22 links on every navigation. - -## Proposed approach - -Hoist the server branch's pre-check logic into a small shared helper used by both branches: - -- reference equality of the two search objects, -- `hasKeys` empty-object short-circuits, -- then the existing `deepEqual` with identical `partial`/`ignoreUndefined` flags. - -Deduplicating the two implementations into one function likely produces a net-negative bundle delta, which can help pay for other findings' additions. - -## Risks & constraints - -- Semantics already proven by the SSR path (it exists for hydration parity) — keep the `exact`/`explicitUndefined` flags identical between branches. -- None beyond standard test coverage. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Focus tests: link active-state tests (`link.test.tsx`), activeOptions matrix (exact/includeSearch/includeHash), SSR hydration-parity tests. diff --git a/perf-investigations/015-link-flushsync-click.md b/perf-investigations/015-link-flushsync-click.md deleted file mode 100644 index 9cc1d792c8..0000000000 --- a/perf-investigations/015-link-flushsync-click.md +++ /dev/null @@ -1,56 +0,0 @@ -# 015 — Link click handler: `flushSync` + per-click `onResolved` subscription - -| | | -| --------------- | ----------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/react-router/src/link.tsx` | -| Benchmarks | client-nav (react) | -| Expected impact | Small-medium (a forced synchronous React render+commit per click, before navigation even starts; 10 clicks per benchmark iteration) | -| Confidence | Medium (correctness is easy; need to confirm no test relies on synchronous attribute visibility) | -| Risk | Medium (observable timing of `data-transitioning`) | -| Prior art | none found | - -## Problem - -`link.tsx:618-639` — on every internal-navigation click: - -```ts -e.preventDefault() -flushSync(() => { - setIsTransitioning(true) -}) -const unsub = router.subscribe('onResolved', () => { - unsub() - setIsTransitioning(false) -}) -router.navigate({ ... }) -``` - -- `flushSync` forces React out of batching and synchronously renders+commits the clicked link's subtree **before** `router.navigate` runs. Per [013](013-link-options-memo-stability.md), that render re-runs `buildLocation` when option props are inline. -- The only consumer of this state is the `data-transitioning` attribute (`link.tsx:716, 724`) — the render-prop children only ever receive `isActive` (`link.tsx:929-934`). -- A one-shot `onResolved` subscription is allocated per click. - -## Proposed approach - -Option A (most conservative): drop `flushSync` and let the urgent state update batch with the navigation's first store write — the attribute still appears within the same frame. - -Option B (bigger win): set the attribute via direct DOM mutation on `innerRef.current` (`el.setAttribute('data-transitioning', '')` / `removeAttribute`) instead of React state, removing both renders per click. The ref is stable; re-renders that recreate props must keep the attribute consistent (set it in both the DOM path and the rendered props while transitioning). - -Either way, the `flushSync` import from `react-dom` may become removable — a bundle-size win. - -## Risks & constraints - -- Timing of `data-transitioning` becomes asynchronous relative to the click (Option A) or non-React-managed (Option B). Check `link.test.tsx` for assertions on synchronous visibility. -- Option B must handle: component re-render while transitioning (React would not know about the attribute — ensure rendered props agree), unmount mid-transition (the `onResolved` unsubscribe already handles state; DOM node is gone anyway). -- Keep `preventDefault`/navigation ordering identical. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -## Related - -- [007](007-transitioner-transition-refcount.md) (the Transitioner-level transition churn) and [013](013-link-options-memo-stability.md) (what each forced render costs). diff --git a/perf-investigations/016-link-render-granularity.md b/perf-investigations/016-link-render-granularity.md deleted file mode 100644 index cd15326c50..0000000000 --- a/perf-investigations/016-link-render-granularity.md +++ /dev/null @@ -1,65 +0,0 @@ -# 016 — Link re-render granularity: derive `{href, isActive}` in the store selector - -| | | -| --------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/react-router/src/link.tsx` (structural refactor) | -| Benchmarks | client-nav (react) | -| Expected impact | **Potentially large** (React render/commit work — `updateReducerImpl` 15.8%, `useLinkProps` 3.2% of the client profile — dominates the benchmark), but highest effort/risk in this series | -| Confidence | Medium (direction is sound; sizing requires prototyping) | -| Risk | High (hook restructuring, bundle-size pressure) | -| Prior art | `origin/perf/react-link-location-deps` ("remove search validation from buildLocation" + link deps experiments; author's own assessment: "i dont think this is good, just technically fun") — mine it for pitfalls before designing | - -## Problem - -`link.tsx:403-413` — every Link subscribes to the whole location with `prev.href === next.href` equality and recomputes everything in render. So **all ~22 links re-render on every navigation** even when their own rendered output (`href`, `isActive`, `data-status`) is unchanged — e.g. `/search?page=2 → page=3` leaves the 8 `/items` links' DOM identical, yet they re-render through React. - -The actual recomputation (`buildLocation`) gets much cheaper with [002](002-match-routes-lightweight-memo.md)+[003](003-build-middleware-chain-cache.md), but the React render/commit per link remains. - -## Proposed approach - -Move `buildLocation` + active-state derivation **into the `useStore` selector**, selecting a small derived record and comparing it shallowly: - -```ts -useStore( - router.stores.location, - (loc) => { - const next = router.buildLocation({ - _fromLocation: loc, - ...optsRef.current, - }) - return { - href: next.publicHref, - isActive: computeIsActive(loc, next, activeOptionsRef.current), - } - }, - shallowRecordCompare, -) -``` - -- Computation still runs per link per location change (cheap after 002/003), but **React re-render/commit is skipped** when the record is unchanged. -- Unstable inline option objects/functions must be read via refs inside the selector (`optsRef` updated in render), since the selector identity must not capture per-render values. -- `isTransitioning` (see [015](015-link-flushsync-click.md)) should be excluded from the record or handled via direct DOM attribute, otherwise it forces renders back in. - -## Risks & constraints - -- Significant restructure of `useLinkProps` hook order; preload-on-viewport/intent logic, event handlers, and `activeProps`/`inactiveProps` merging all consume the derived values — they need to read from the selected record. -- Selector must stay pure w.r.t. the store value; reading refs is fine, but updating them must happen before notifications (render order guarantees this for prop changes; verify for the `from`-route context). -- **Bundle-size pressure is real here** — this is a rewrite of the hottest component file; budget the gzip delta from the start and aim to pay for it by deleting the now-redundant memo plumbing. -- SSR branch (`link.tsx:157-266`) must keep working without stores. -- Recommended approach: prototype behind a small benchmark-only build first; measure render counts (React Profiler) before committing to the full refactor. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:flame:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Success criterion: links whose href/active state didn't change produce zero React commits per navigation; flamegraph shows reduced `beginWork`/`updateProperties` share. - -## Related - -- Depends on [002](002-match-routes-lightweight-memo.md)/[003](003-build-middleware-chain-cache.md) to make per-link selector work cheap. -- Subsumes [013](013-link-options-memo-stability.md) if fully implemented (do 013 first anyway — it's low-risk and informs this design). diff --git a/perf-investigations/017-interpolate-path-template-cache.md b/perf-investigations/017-interpolate-path-template-cache.md deleted file mode 100644 index 6633da843b..0000000000 --- a/perf-investigations/017-interpolate-path-template-cache.md +++ /dev/null @@ -1,38 +0,0 @@ -# 017 — cache parsed route-template segments for client `interpolatePath` - -| | | -| --------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/path.ts`, `new-process-route-tree.ts` (shared client+server) | -| Benchmarks | client-nav (react); also SSR cache-miss URLs | -| Expected impact | Medium (cheap per call, but ~130 template re-parses per client-nav iteration, plus once per route per `matchRoutes`) | -| Confidence | High on mechanism, medium on win size | -| Risk | Low-medium (bundle-size pressure is the main constraint) | -| Prior art | `origin/perf-path` (`ecdd3b2eb6` "optimize path.ts") and `origin/perf-process-route-tree` (`ddb360d575`) touch this area — **check both before implementing** to avoid conflicts/duplication | - -## Problem - -- `path.ts:244-402` — `interpolatePath` has a fast path **gated server-only** (`if (isServer ?? rest.server)` at `path.ts:263`; `isServer/client.ts` exports `false`, so the block is dead-code-eliminated from client bundles). -- On the client, every interpolation of a param-bearing template (e.g. `/items/$id`) re-runs `parseSegment` (`new-process-route-tree.ts:73-184`) per segment: `indexOf('/')`, `substring`, `includes('$')`, brace scanning — for the **same handful of immutable `route.fullPath` strings** every time. -- Call sites: `router.ts:1527` (per matched route per `matchRoutes`) and `router.ts:1950` (per `buildLocation`, i.e. per Link per location change). -- Templates without `$` early-return (`path.ts:260-261`), so only param routes pay; client-nav has ~10 param-bearing links + ~3 matched routes ≈ 13 template re-parses per navigation, ~130 per iteration. - -## Proposed approach - -1. Cache parsed segments per template string in a lazily populated module-level `Map` (e.g. a flattened `Uint16Array` of 6 values per segment, or an array of segment descriptors); the interpolation loop iterates the cached structure instead of calling `parseSegment`. The route set is bounded — no LRU needed (or reuse `createLRUCache` if worried about dynamically generated templates). -2. Alternative: attach the parsed-segment array lazily to the route instance and pass it into `interpolatePath` from both call sites (avoids a global map; slightly more plumbing). -3. **Do not** un-gate the server fast path for the client — that would re-add ~55 lines to the client bundle. - -## Risks & constraints - -- Bundle: estimated +~150 bytes gzip — this is the finding most likely to need an offsetting deletion (candidate: deduplicate the unguarded `trimPathRight` copy in `new-process-route-tree.ts:761-763`, which duplicates `path.ts:35-38` with the guard missing; note path.ts already imports from new-process-route-tree.ts, so inline the guard rather than importing the other way). -- Cache key must be the exact template string (it is — `ParsedSegment` offsets index into that string). -- Unbounded growth only with user-generated dynamic templates — rare; LRU if needed. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit # path.test.ts + new-process-route-tree.test.ts (173 tests) -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` diff --git a/perf-investigations/018-match-routes-internal-overheads.md b/perf-investigations/018-match-routes-internal-overheads.md deleted file mode 100644 index 345b46d9a6..0000000000 --- a/perf-investigations/018-match-routes-internal-overheads.md +++ /dev/null @@ -1,48 +0,0 @@ -# 018 — `matchRoutesInternal` / `build()` per-navigation fixed overheads - -| | | -| --------------- | ------------------------------------------------------------------------------------------------------------------ | -| Area | `packages/router-core/src/router.ts` (shared client+server) | -| Benchmarks | client-nav (react); scales with app size | -| Expected impact | Small-medium today; the snapshot loop is O(all stored matches) and grows with `gcTime`-cached matches in real apps | -| Confidence | Medium-high (one item needs a semantics check) | -| Risk | Medium | -| Prior art | none found | - -Four independent micro-inefficiencies in the once-per-navigation full matching path (`matchRoutesInternal` runs once per `load()`, profiled at 22.7 ms self in a 5.4 s client profile) and in `build()`: - -## 1. `previousActiveMatchesByRouteId` iterates ALL match stores - -`router.ts:1457-1465` — builds a fresh Map by iterating **every** entry of `this.stores.matchStores` and calling `store.get()` on each. `matchStores` holds active + pending + cached pools (`stores.ts:99-140`), so with `gcTime: 60_000` (benchmark) cached matches accumulate and the loop grows beyond the 2-3 active matches actually needed. - -**Strategy**: build the snapshot only for routeIds relevant to `matchedRoutes`, e.g. by iterating `stores.matchesId` (the active list) instead of all stores, or maintain an incremental routeId→matchId index. -**Risk**: confirm which pools are _supposed_ to contribute — if cached matches were intentionally included (param-reuse semantics), restrict differently. Verify against `stores.ts:99-140` and the params-reuse tests. - -## 2. `loaderDepsHash` JSON.stringify per route per navigation - -`router.ts:1520-1525` — `loaderDepsHash = loaderDeps ? JSON.stringify(loaderDeps) : ''` recomputed even when deps are unchanged. - -**Strategy**: compute the hash only after the `replaceEqualDeep(previousMatch.loaderDeps, loaderDeps)` equality pass; when the previous reference is returned, reuse the previous hash. - -## 3. Triple search spreads per route - -`router.ts:1481-1495` — three object spreads per matched route for search stabilization (`{...parentSearch}`, `{...parentSearch, ...strictSearch}`, `{...parentStrictSearch, ...strictSearch}`), plus `validateSearch` re-runs although `buildAndCommitLocation` already ran it with `_includeValidateSearch: true` (`router.ts:2277-2280`). - -**Strategy**: skip spreads when `strictSearch` is empty/unchanged; investigate reusing the validated search from the built location (guarded — see [009](009-parse-location-reuse-built-location.md) for the precedent of being careful here). - -## 4. `params.stringify` loop in `build()` re-resolves options per call - -`router.ts:1928-1944` — for every `buildLocation` (~23×/navigation), the loop does the two-level option lookup (`route.options.params?.stringify ?? route.options.stringifyParams`) per destRoute, and runs user stringify fns (fresh object + `Object.assign`) even when `opts.leaveParams` makes the result unused. - -**Strategy**: precompute a per-branch `stringifyFns` array (or `null` when none) cached alongside `getRouteBranch`/`match.branch`, so branches without stringify fns skip the loop entirely; check whether `leaveParams` can skip it (verify `usedParams` consumers first). Semantic result-caching of user fns is NOT safe — don't attempt. -**Risk**: invalidate the per-branch cache on `route.update()`. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Items 1-3 are independent of each other; item 1 is the most valuable for real apps (turns O(stored matches) into O(active matches)). diff --git a/perf-investigations/019-match-identity-reuse.md b/perf-investigations/019-match-identity-reuse.md deleted file mode 100644 index 50b4dd2f94..0000000000 --- a/perf-investigations/019-match-identity-reuse.md +++ /dev/null @@ -1,41 +0,0 @@ -# 019 — reuse match object identity for unchanged matches in `matchRoutesInternal` - -| | | -| --------------- | ------------------------------------------------------------------------------------------ | -| Area | `packages/router-core/src/router.ts`, `stores.ts` (shared client+server) | -| Benchmarks | client-nav (react); bigger in real apps with stable layout routes | -| Expected impact | Medium-large **when combined with 006/008/020**; small alone | -| Confidence | Medium (highest-behavior-risk finding in the core series) | -| Risk | High — copy-on-write discipline required | -| Prior art | none found; `origin/perf-replaceEqualDeep` optimizes the equality machinery this relies on | - -## Problem - -Every navigation rebuilds **every** matched route's match object, even when nothing about it changed: - -- `router.ts:1580-1590` — unconditional `{...existingMatch, ...}` spread with new `cause`, new `updatedAt`, and (via `router.ts:1651-1655`, `1698-1702`) a fresh `context` object identity. `search`/`params`/`loaderDeps` are already structurally shared (`router.ts:1586-1588`, `1627-1629`, `1667-1669`), and `_strictSearch` is not (`router.ts:1495`). -- At commit, `reconcileMatchPool` (`stores.ts:306-342`) sets a match store whenever `existing.get() !== nextMatch` (`stores.ts:333-335`) — which is **always**, because the spread guarantees fresh identity. -- Consequence: the **root match** notifies its full subscriber set on every navigation. In the benchmark that's the largest fan-out in the app: 20 root-level selector hooks (strict:false `useParams`/`useSearch`) re-run per navigation even when root params/search are untouched. - -## Proposed approach - -After computing the new fields for an existing match, compare the reactive fields against `existingMatch` — `search`, `params`, `_strictSearch` (give it `replaceEqualDeep` first), `loaderDeps`, `searchError`, `globalNotFound`, `cause`, `context` (after [006](006-update-match-write-coalescing.md)'s context bail-out stabilizes it). If all are reference-equal, **return `existingMatch` itself**, so `reconcileMatchPool` skips the `set` and no subscriber is notified. - -Apply `replaceEqualDeep` to `_strictSearch` and the `context` spread regardless — that alone improves downstream selector short-circuiting ([010](010-hook-selector-slice-memoization.md)). - -## Risks & constraints - -- **Copy-on-write hazard**: `matchRoutesInternal` currently mutates the _new_ object after the spread (`router.ts:1643-1655` — `cause` 'enter'→'stay' transitions, `globalNotFound`/`searchError` resets). Reusing `existingMatch` means those mutations would write into **live store state**. The equality check must cover every post-spread mutation, and any difference must fall back to the spread path. -- `updatedAt`/`cause` semantics if a "stay" navigation is skipped: `cause` transitions are observable (`onStay` hooks, `match.cause` in loaders). Only reuse identity when `cause` is also unchanged. -- Only worthwhile **after** [006](006-update-match-write-coalescing.md)/[008](008-lazy-controlled-promise.md): otherwise the later `fetchCount`/`abortController`/`context` writes in `loadMatches` break identity again anyway. -- Structural-sharing test suites should catch regressions; add a test asserting the root match store does not notify on an unrelated child param change. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit && pnpm nx run @tanstack/react-router:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Success criterion: navigating `/items/1 → /items/2` produces zero notifications on the root match store's subscribers. diff --git a/perf-investigations/020-build-match-context-incremental.md b/perf-investigations/020-build-match-context-incremental.md deleted file mode 100644 index 4ed28f8d61..0000000000 --- a/perf-investigations/020-build-match-context-incremental.md +++ /dev/null @@ -1,40 +0,0 @@ -# 020 — accumulate `buildMatchContext` incrementally instead of re-walking ancestors - -| | | -| --------------- | ---------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/load-matches.ts`, `router.ts` (shared client+server) | -| Benchmarks | client-nav (react); grows with route depth | -| Expected impact | Medium (O(depth²) → O(depth) per navigation; ~6-8 context rebuilds per navigation today) | -| Confidence | Medium-high | -| Risk | Medium (concurrency/ordering assumptions must be verified) | -| Prior art | none found | - -## Problem - -- `load-matches.ts:61-78` — `buildMatchContext` re-spreads `router.options.context` and `Object.assign`s **every ancestor's** `__routeContext`/`__beforeLoadContext`, calling `getMatch` per ancestor; `getMatch` (`router.ts:2729-2735`) probes up to 3 Maps (cached → pending → active pools) per lookup. -- Callers: `load-matches.ts:144`, `:196` (`syncMatchContext`), `:456` (`executeBeforeLoad`), `:618` (`getLoaderContext`), `:720`, `:738` — so per navigation with depth-3 matches, contexts are rebuilt ~6-8 times, each O(depth): **O(depth²)** total. -- `matchRoutesInternal` independently builds `match.context` again (`router.ts:1651-1655`, `1698-1702`). - -## Proposed approach - -Matches are processed serially in index order in both the beforeLoad loop and the loader phase, so accumulate in `InnerLoadContext`: - -- Keep `contextAccumulator[index]`; extend from `index - 1` instead of re-walking from the root. -- Invalidate the suffix when a match's `__beforeLoadContext` is written (beforeLoad of index i completes before anything at index > i reads context). -- Reuse the previous context object when no ancestor contributed changes — this stabilizes `match.context` identity, enabling the `syncMatchContext` bail-out in [006](006-update-match-write-coalescing.md) and identity reuse in [019](019-match-identity-reuse.md). - -## Risks & constraints - -- The loader phase runs matches "concurrently" via `Promise.all`, but context for index i depends only on beforeLoad results, which are strictly serial and complete before loaders start — verify the redirect/error mid-stream cases (`load-matches.ts:144`) still see correct partial context. -- The accumulator must not leak across `loadMatches` invocations (preload vs navigate overlap). -- Bundle delta roughly neutral (replaces a loop with an index read + invalidation bookkeeping). - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Focus tests: beforeLoad context inheritance, context invalidation, redirect-from-beforeLoad, preload. diff --git a/perf-investigations/021-commit-cached-pool-single-pass.md b/perf-investigations/021-commit-cached-pool-single-pass.md deleted file mode 100644 index 71275abb46..0000000000 --- a/perf-investigations/021-commit-cached-pool-single-pass.md +++ /dev/null @@ -1,47 +0,0 @@ -# 021 — single-pass cached-pool reconciliation in the commit path - -| | | -| --------------- | -------------------------------------------------------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/router.ts`, `stores.ts` (shared client+server) | -| Benchmarks | client-nav (react) | -| Expected impact | Small (clean win, low risk; O(matches + cached) once per navigation) | -| Confidence | High | -| Risk | Low-medium (eviction-ordering semantics) | -| Prior art | `origin/refactor-router-core-ready-transition-performance` (`cd6db1f5dc`) reworks this exact onReady region — **read/rebase it first** | - -## Problem - -The commit batch (`router.ts:2505-2565`, `onReady`) reconciles the cached pool **twice** per navigation and allocates ~6 intermediate collections: - -- `exitingMatches` filter (`router.ts:2510-2515`), `pendingRouteIds`/`activeRouteIds` Sets (`:2519-2526`), three hook-filter arrays (`:2528-2542`). -- First reconciliation: `setCached([...cached, ...exitingMatchesToKeep])` (`:2555-2563`). -- Immediately after: `clearExpiredCache()` (`:2564`) → `clearCache` → a **second** `setCached` (`router.ts:2840-2865`) that re-filters everything just appended. -- Each `setCached` runs `reconcileMatchPool` (`stores.ts:306-342`): ids map + Set + per-entry pool ops + `arraysEqual`. - -Notably, `clearExpiredCache`'s filter evicts any route **without a loader**, so loaderless exiting matches (items/ctx in the benchmark) are appended to the cache by the first `setCached` and immediately removed by the second — pure churn, every navigation. - -## Proposed approach - -Compute the final cached list in one pass: - -1. Filter `exitingMatches` by status AND gc-eligibility/loader-presence **before** a single `setCached`. -2. Fold the gc sweep of pre-existing cached entries into the same pass. -3. Optionally compute the `exiting`/`entering`/`staying` partitions in one loop over `currentMatches` + `pendingMatches` instead of 4 filters + 2 Sets. - -Keep `clearExpiredCache` as a public/elsewhere-used method; just stop calling it redundantly inside commit. - -## Risks & constraints - -- Eviction ordering vs the `status !== 'error'` filters must match current behavior exactly (a match evicted by gc must not receive onLeave-adjacent handling differently). -- Coordinate with the prior-art branch — it already restructures this block; landing it may subsume this file. -- Bundle delta neutral-to-negative. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Focus tests: gcTime/staleTime cache retention, cache invalidation, onEnter/onStay/onLeave ordering. diff --git a/perf-investigations/022-commit-path-micro-allocations.md b/perf-investigations/022-commit-path-micro-allocations.md deleted file mode 100644 index d18eae4a85..0000000000 --- a/perf-investigations/022-commit-path-micro-allocations.md +++ /dev/null @@ -1,43 +0,0 @@ -# 022 — commit-path micro-allocations: memory-history re-parse + residual async wrappers - -| | | -| --------------- | --------------------------------------------------------------------------------------- | -| Area | `packages/history/src/index.ts`, `packages/router-core/src/router.ts` | -| Benchmarks | client-nav (react) — uses memory history; SSR also creates a memory history per request | -| Expected impact | Small (once per navigation, but near-zero risk; several items are pure deletions) | -| Confidence | High that it's wasted work | -| Risk | Low | -| Prior art | follows up merged #7582 (`5127d861ae`), which de-promised parts of this path | - -Two clusters of small, independent wins on the once-per-navigation commit path. - -## A. Memory history re-parses and over-allocates per notify - -- `history/index.ts:595` — `createMemoryHistory.getLocation = () => parseHref(entries[index]!, states[index])`, and `notify` (`history/index.ts:119-122`) calls `getLocation()` on every push/replace. Each call runs `sanitizePath` (regex + `startsWith` + possible second regex) and three `indexOf`/`substring` passes. Compare `createBrowserHistory`, which caches `currentLocation = parseHref(destHref, state)` in `queueHistoryAction` (`history/index.ts:383`). -- `history/index.ts:653-685` — `parseHref` calls `createRandomKey()` **unconditionally** (`:661`) even though the result is discarded whenever `state` is provided (`state: state || {...}` at `:683`) — which is always, on this path. -- `history/index.ts:129-159` — `tryNavigation` is `async`, allocating a promise per push/replace even when there are no blockers and the path is fully synchronous. - -**Strategies** - -1. Move `createRandomKey()` into the `state ||` fallback branch of `parseHref` — zero risk, negative bundle delta. -2. Cache the parsed location in `createMemoryHistory`: update a `currentLocation` variable inside `pushState`/`replaceState` (path + state already in hand); re-parse only in `go`/`back`/`forward`. Mirrors the browser-history pattern. -3. Sync fast path in `tryNavigation` when `ignoreBlocker || blockers.length === 0`: just call `task()`; keep the async path for blockers. `push`/`replace` already ignore the returned promise. - -## B. Residual promise wrappers in `navigate`/`commitLocation` - -Per navigation, before `loadMatches` even starts, ~7 promises are allocated. The avoidable ones: - -- `router.ts:2143` — `commitLocation` is `async` but contains **no await** (ends with `return this.commitLocationPromise`): the keyword only adds a wrapper promise + an extra microtask hop for awaiters. De-`async` it and return the promise directly. -- `router.ts:2313` — `navigate` is `async`; its only `await` is in the `reloadDocument` blocker loop. Restructure so the hot path returns `this.buildAndCommitLocation(...)` directly and the `reloadDocument` branch lives in an async helper. - -**Risk note**: a non-async function throws synchronously where an async one returned a rejected promise. Both can throw from `buildLocation` (user `params.stringify`, search updaters). Verify test expectations; if needed, wrap in try/catch returning `Promise.reject(err)` to preserve semantics. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/history:test:unit && pnpm nx run @tanstack/router-core:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` - -Focus tests: history blockers, back/forward in memory history, navigate() rejection behavior. diff --git a/perf-investigations/023-ssr-router-update-three-times.md b/perf-investigations/023-ssr-router-update-three-times.md deleted file mode 100644 index 0dc9ace9f2..0000000000 --- a/perf-investigations/023-ssr-router-update-three-times.md +++ /dev/null @@ -1,45 +0,0 @@ -# 023 — SSR: `router.update()` runs 3× per request; the third re-parses the location for one context key - -| | | -| --------------- | -------------------------------------------------------------------------------------------- | -| Area | `packages/start-server-core/src/createStartHandler.ts`, `packages/router-core/src/router.ts` | -| Benchmarks | ssr (react) | -| Expected impact | Small-medium (~1-2% of request CPU; more for apps with expensive custom `parseSearch`) | -| Confidence | High on mechanism, medium on win size | -| Risk | Low-medium | -| Prior art | none found | - -## Problem - -Per SSR request, `RouterCore.update()` runs three times: - -1. In the `RouterCore` constructor (`router.ts:1024`) — unavoidable. -2. In `createStartHandler`'s `getRouter` (`createStartHandler.ts:466-479`) — necessary: sets history, triggers the only needed `parseLocation`. -3. At `createStartHandler.ts:587` — `routerInstance.update({ additionalContext: { serverContext } })` — exists **only to inject one context key**, yet `update()` (`router.ts:1056-1214`): - - re-spreads all options (`:1076-1079`), - - rebuilds the `protocolAllowlist` `Set` (`:1083`), - - calls `updateLatestLocation()` (`:1117-1119`) → a full `parseLocation` on the **same href**, including a `parseSearch` + `stringifySearch` round trip (and, pre-[001](001-search-params-json-parse-exception-storm.md), JSON.parse throws). - -Profile: `RouterCore.update` 0.6% self + constructor 0.7% + the redundant parse work. - -## Proposed approach - -Pick one: - -- **(a)** In `executeRouter`, set the key directly: `routerInstance.options.additionalContext = { serverContext }`. It is only consumed via `this.options.additionalContext` (`load-matches.ts:487`). Smallest diff; bypasses `update()` side effects for this key (fine today — verify no other consumer). -- **(b)** Add a fast path in `update()` that skips `updateLatestLocation`, the Set rebuild, and basepath logic when none of `history`/`routeTree`/`basepath`/`rewrite`/`protocolAllowlist` changed. More general (also helps client HMR-ish update calls), needs a careful key whitelist. -- (c) Passing `additionalContext` through update #2 is **not** possible — middleware context resolves later in the request. - -## Risks & constraints - -- (a) couples start-server-core to the options-mutation detail; add a comment or a tiny dedicated router method (`router.setAdditionalContext(ctx)`) if preferred. -- (b) touches shared code — keep the guard byte-cheap (client bundle). - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit -# e2e start tests cover server context propagation: -# verify additionalContext/serverContext reaches beforeLoad/loaders in start e2e suites -``` diff --git a/perf-investigations/024-ssr-handler-hoisting.md b/perf-investigations/024-ssr-handler-hoisting.md deleted file mode 100644 index f2d845024e..0000000000 --- a/perf-investigations/024-ssr-handler-hoisting.md +++ /dev/null @@ -1,42 +0,0 @@ -# 024 — SSR: hoist per-request handler boilerplate (URL parses, options factory, middleware flattening) - -| | | -| --------------- | ------------------------------------------------------------------------------------------------------- | -| Area | `packages/start-server-core/src/createStartHandler.ts`, `ssr-server.ts`, `createStart.ts` (server-only) | -| Benchmarks | ssr (react) | -| Expected impact | Small (~1%), but item 1 is essentially free | -| Confidence | High on item 1; medium on item 2 | -| Risk | Low (server-only code, no bundle constraint) | -| Prior art | none found | - -## Problem - -`createStartHandler.ts:410-449` runs on 100% of requests, before any routing: - -1. **Redundant URL parsing**: `getNormalizedURL(request.url)` (`:410`) parses the URL twice internally (`ssr-server.ts:762-781`: `rawUrl` + final `new URL`), then `getOrigin(request)` (`:412`) does a third `new URL(request.url)` (`ssr-server.ts:749-754`), and `new H3Event(request)` in `requestHandler` (`request-response.ts:127`) builds a fourth (`FastURL`). -2. **Per-request re-derivation of per-process-constant data**: - - `await entries.startEntry.startInstance?.getOptions()` (`:421`) re-invokes the user's options factory and re-dedupes serialization adapters every request (`createStart.ts:138-149`: new `Set` + `Array.from`). - - `serializationAdapters` array rebuilt (`:427-431`), `requestStartOptions` spread (`:433-439`), `flattenMiddlewares` re-flattened + `new Set` (`:442-449`). - -Profile: `startRequestResolver` 0.2% self plus scattered URL/Set work inside the node-internals bucket (7.8%). - -## Proposed approach - -1. Return `origin` from `getNormalizedURL` (it already has `rawUrl.origin`) and delete the `getOrigin` call. Free. -2. Memoize the derived data keyed by the resolved options object identity (`WeakMap`/last-value cache): `getOptions()` result → `serializationAdapters` → flattened request middlewares. Dynamic factories returning fresh objects still work (cache miss); static ones (the common case) hit. -3. Consider feeding the H3Event's already-parsed URL into normalization, or vice versa. - -## Risks & constraints - -- `getOptions()` is user code and may be intentionally dynamic — never cache across differing returned objects; key strictly by identity (or document that options must be stable). -- The `executedRequestMiddlewares` `Set` is mutated per request (`createStartHandler.ts:783`) — only the flattened **array** is cacheable; the Set must stay per-request. -- Server-only: no client bundle pressure. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/start-server-core:test:unit -``` - -Also run a start e2e suite touching request middleware ordering and custom basepath/origin handling. diff --git a/perf-investigations/025-ssr-html-boundary-scan.md b/perf-investigations/025-ssr-html-boundary-scan.md deleted file mode 100644 index 7520c226b0..0000000000 --- a/perf-investigations/025-ssr-html-boundary-scan.md +++ /dev/null @@ -1,39 +0,0 @@ -# 025 — SSR: `findHtmlBoundary` scans every closing tag per chunk - -| | | -| --------------- | ------------------------------------------------------------------------------------------------------- | -| Area | `packages/router-core/src/ssr/transformStreamWithRouter.ts` (server-only) | -| Benchmarks | ssr (react) — small here (~0.3%); medium for chunk-heavy streaming pages | -| Expected impact | Small in the benchmark; medium for suspense-streaming apps with many chunks | -| Confidence | Medium (code-read; not hot in this benchmark because it renders ~1 big chunk per request) | -| Risk | Low (server-only) | -| Prior art | `reserveStreamFastPath` (`transformStreamWithRouter.ts:208-212`) already exists — extend, don't replace | - -## Problem - -- `transformStreamWithRouter.ts:70-125` — `findHtmlBoundary` walks **all** `` case-insensitively, even after the last valid closing tag is identified. Cost is O(number of closing tags) JS-loop iterations per chunk instead of one native scan. -- Used in the read loop at `:796-848` — every app chunk pays it. -- `noteBarrierMarker`'s `chunk.includes(TSR_SCRIPT_BARRIER_ID)` (`:561-566`) adds another full chunk scan until the barrier is seen. - -The SSR benchmark renders with `progressiveChunkSize: Number.POSITIVE_INFINITY` (`renderRouterToStream.tsx:53,141`) → ~1 big chunk per request, so this is ~0.3% there. Real streaming apps (suspense boundaries, deferred data) produce many chunks and re-scan repeatedly. - -## Proposed approach - -Fast path first: a single forward `chunk.indexOf('`: - -- React/Solid/Vue renderers emit lowercase; user-injected HTML may not. Either probe both `'` detection exactly (current code handles ``). -- Must preserve the "safe split point" semantics for chunks that end mid-tag — that's the function's real job; the fast path only short-circuits the common whole-tag case. -- Server-only file: no client bundle pressure. - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit # transformStreamWithRouter tests -``` - -To see the real effect, also benchmark a streaming scenario (small `progressiveChunkSize` or a suspense-heavy page); consider adding such a variant to `benchmarks/ssr` as part of this work. diff --git a/perf-investigations/026-ssr-trie-allocations.md b/perf-investigations/026-ssr-trie-allocations.md deleted file mode 100644 index de16c76218..0000000000 --- a/perf-investigations/026-ssr-trie-allocations.md +++ /dev/null @@ -1,38 +0,0 @@ -# 026 — route-matching trie: per-candidate frame allocations and LRU eviction churn under non-repeating URLs - -| | | -| --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| Area | `packages/router-core/src/new-process-route-tree.ts`, `lru-cache.ts` (shared client+server) | -| Benchmarks | ssr (react) — the bench generates a unique random URL per request, so the match LRU never hits; negligible for client-nav (cache hits after warmup) | -| Expected impact | Small-medium for SSR/unique-URL workloads | -| Confidence | Medium | -| Risk | High-ish relative to payoff — this is the most intricate code in the package (priority bitmasks, fuzzy resumable extraction) | -| Prior art | **`origin/perf-process-route-tree`** (`ddb360d575` + result notes `f36a2afcfa`) already optimizes this file (removes `sortTreeNodes` walk, adds a `%`-check fast path before `decodeURIComponent` in `extractParams`). **Land/rebase that branch first; only then evaluate what remains below.** | - -## Problem - -For a URL that misses the match cache (every request in the SSR bench; first-visit URLs in real servers): - -- `new-process-route-tree.ts:725-758` — `findRouteMatch`: `matchCache.get` (miss) + full trie walk + `buildRouteBranch` + `matchCache.set`; once the 1000-entry LRU is warm, **every set also evicts** (`lru-cache.ts:44-67`: delete + linked-list pointer surgery). Note the cache is still useful _within_ a request (subsequent `getMatchedRoutes` calls for rendered Links hit the just-inserted entry) — do not remove it. -- `new-process-route-tree.ts:1041-1051`, `1164-1304` — `getNodeMatch` pushes a 10-field frame object per candidate (`{node, index, skipped, depth, statics, dynamics, optionals, extract, rawParams}`); a 4-level dynamic tree costs ~10+ frames per miss, plus `path.split('/')` and `decodeURIComponent` per dynamic segment in `extractParams`. - -## Proposed approach (after the prior-art branch lands) - -1. **Single-candidate loop instead of push/pop**: when exactly one candidate child exists, advance a cursor in place rather than pushing a speculative frame — most segments in typical trees are unambiguous. -2. **Frame pooling or flat parallel arrays** for the speculative stack — cuts per-miss allocation; weigh against bundle size (shared code). -3. LRU: consider a cheaper eviction (ring buffer / clock) or a smaller per-set cost; only if profiling post-1/2 still shows `lru-cache.set` (`origin/perf-lru-cache` may already address internals — check it). - -## Risks & constraints - -- The full `new-process-route-tree.test.ts` suite (173 tests) is the safety net; fuzzy matching, optional params, and priority ordering are easy to subtly break. -- Bundle pressure: pooling code ships to the client; keep it minimal or server-gated only if it can be tree-shaken (`isServer` const folding). -- Verify against the SSR bench (unique URLs) _and_ client-nav (must not regress the cache-hit path). - -## Validation - -```bash -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -pnpm nx run @tanstack/router-core:test:unit -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -``` diff --git a/perf-investigations/README.md b/perf-investigations/README.md deleted file mode 100644 index ad8722d3df..0000000000 --- a/perf-investigations/README.md +++ /dev/null @@ -1,109 +0,0 @@ -# Performance Investigations - -One file per runtime-performance improvement opportunity for the core + react packages -(`router-core`, `history`, `react-router`, `start-*`). Each file is self-contained: an -implementation agent should be able to pick up a single file and produce a change. - -Generated 2026-06-10 from a code audit + CPU profiling of the benchmarks at commit `6f1daf5104`. - -## Ground rules (apply to every investigation) - -- **Goal: speed.** Wall-clock/CPU improvements in `benchmarks/client-nav` and `benchmarks/ssr`. - Memory only matters via GC pressure. -- **Hard constraint: client bundle size must not regress.** Gzip of all emitted client JS is - tracked in CI (`benchmarks/bundle-size`). Any change to code shipped to the client must be - validated against it. Server-only code (`start-server-core`, `router-core/src/ssr/ssr-server`) - has more freedom. -- **Behavior must not change.** The router has extensive unit tests; run the touched package's - `test:unit` target. Several proposals note specific semantic hazards — read them. - -## Validation commands - -```bash -# client navigation speed (the main client benchmark; jsdom) -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:react --outputStyle=stream --skipRemoteCache -# flamegraph of the same loop -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:flame:react --outputStyle=stream --skipRemoteCache - -# SSR request-loop speed -CI=1 NX_DAEMON=false pnpm nx run @benchmarks/ssr:test:perf:react --outputStyle=stream --skipRemoteCache - -# bundle-size guard (always run for client-shipped code changes) -pnpm nx run @benchmarks/bundle-size:build -- --scenario react-router.minimal,react-router.full -pnpm benchmark:bundle-size:query --id react-router.minimal - -# unit tests -pnpm nx run @tanstack/router-core:test:unit -pnpm nx run @tanstack/react-router:test:unit -``` - -Profiling baselines measured during this audit: client-nav ≈ **1.77 ms/navigation** -(prebuilt dist, node script over jsdom), SSR ≈ **0.957 ms/request** (NODE_ENV=production — -without it you profile dev React). - -## ⚠️ Check unmerged prior-art branches first - -Several opportunities already have in-flight branches. Before implementing a file that lists -prior art, inspect/rebase the branch instead of starting from scratch: - -| Branch | Covers | -| -------------------------------------------------------------- | ------------------------------------------------------------ | -| `origin/flo/search-params-json-parse-guard` | 001 (most recent, has review feedback applied) | -| `origin/refactor-router-core-stringify-parse-search-json-perf` | 001 (older alternative) | -| `origin/flo/current-location-lightweight-cache` | 002 | -| `origin/flo/load-matches-sync-fastpaths` | 004 (partially 008) | -| `origin/refactor-router-core-ready-transition-performance` | 007 / 021 vicinity | -| `origin/perf/react-link-location-deps` | 013/016 exploration (author considered it unsatisfactory) | -| `origin/perf-path` | 017 vicinity | -| `origin/perf-process-route-tree` | 026 + param-decode fast paths | -| `origin/perf-replaceEqualDeep` | `replaceEqualDeep` internals (touches many files' hot paths) | -| `origin/refactor-react-router-head-use-tags-performance` | head/useTags (not covered by a file here) | - -## Index, ordered by expected impact - -### Tier 1 — measured, large - -- [001 — search-params JSON.parse exception storm](001-search-params-json-parse-exception-storm.md) — **validated −42% SSR request time**; 5.7% of client profile -- [002 — memoize matchRoutesLightweight per location](002-match-routes-lightweight-memo.md) — validated additional −7% SSR; removes ~22/23 of per-Link match work on client -- [004 — sync fast paths in the loader phase of loadMatches](004-load-matches-sync-fastpaths.md) -- [006 — updateMatch write coalescing per match](006-update-match-write-coalescing.md) -- [007 — Transitioner startTransition/setIsTransitioning churn](007-transitioner-transition-refcount.md) - -### Tier 2 — high confidence, medium - -- [003 — cache buildMiddlewareChain](003-build-middleware-chain-cache.md) (explicit TODO in code) -- [005 — skip runLoader for loaderless routes](005-loaderless-runloader-fast-path.md) -- [008 — lazy ControlledPromise allocation](008-lazy-controlled-promise.md) -- [009 — parseLocation reuse of just-built location](009-parse-location-reuse-built-location.md) -- [010 — hook selector slice memoization](010-hook-selector-slice-memoization.md) -- [016 — Link re-render granularity](016-link-render-granularity.md) (largest potential, largest effort) -- [018 — matchRoutesInternal per-navigation overheads](018-match-routes-internal-overheads.md) -- [019 — match identity reuse for unchanged matches](019-match-identity-reuse.md) - -### Tier 3 — smaller / localized - -- [011 — useMatch selector throws Error per unmounting subscriber](011-use-match-invariant-throw.md) -- [012 — Outlet/Match/MatchInner selector compares](012-outlet-match-selector-compares.md) -- [013 — Link \_options memo defeated by inline props](013-link-options-memo-stability.md) -- [014 — port server isActive pre-checks to client](014-link-isactive-client-prechecks.md) -- [015 — Link flushSync on click](015-link-flushsync-click.md) -- [017 — interpolatePath template parse cache](017-interpolate-path-template-cache.md) -- [020 — incremental buildMatchContext](020-build-match-context-incremental.md) -- [021 — single-pass cached-pool reconciliation at commit](021-commit-cached-pool-single-pass.md) -- [022 — commit-path micro-allocations (memory history, async wrappers)](022-commit-path-micro-allocations.md) - -### Tier 4 — SSR/server-only - -- [023 — router.update() runs 3× per SSR request](023-ssr-router-update-three-times.md) -- [024 — hoist per-request handler boilerplate](024-ssr-handler-hoisting.md) -- [025 — findHtmlBoundary chunk scanning](025-ssr-html-boundary-scan.md) -- [026 — route-matching trie allocations under non-repeating URLs](026-ssr-trie-allocations.md) - -## Composition notes - -- 001, 002 and 003 compose: after all three, a Link re-render's `buildLocation` is mostly - LRU hits + `interpolatePath` + cheap stringify. -- 004, 005, 006, 008 touch overlapping call sites in `load-matches.ts` — coordinate or land - sequentially with rebases. -- 019 only pays off after 006 (otherwise later writes break identity anyway). -- 010, 011, 012 are independent react-router changes but all reduce per-store-set subscriber cost. From 9926ac4f55c48145636f2cae058eb4f858906040 Mon Sep 17 00:00:00 2001 From: Sheraff Date: Thu, 11 Jun 2026 00:05:27 +0200 Subject: [PATCH 4/5] fix scroll restoration --- packages/react-router/src/Match.tsx | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/react-router/src/Match.tsx b/packages/react-router/src/Match.tsx index f5cffd5b4d..acecde9043 100644 --- a/packages/react-router/src/Match.tsx +++ b/packages/react-router/src/Match.tsx @@ -23,6 +23,7 @@ import { useLayoutEffect } from './utils' import type { AnyRoute, AnyRouteMatch, + ParsedLocation, RootRouteOptions, } from '@tanstack/router-core' @@ -247,7 +248,15 @@ function OnRendered() { } // eslint-disable-next-line react-hooks/rules-of-hooks - const prevHrefRef = React.useRef(undefined) + // @ts-expect-error -- init to `undefined` but don't write `undefined` to shave bytes + // Track the resolvedLocation as of the last render so that onRendered can + // report the correct fromLocation. By the time this effect fires, + // resolvedLocation has already been updated to the new location by + // Transitioner, so we cannot use router.stores.resolvedLocation.get() + // directly as the fromLocation. + const prevResolvedLocationRef = React.useRef< + ParsedLocation | undefined + >() // eslint-disable-next-line react-hooks/rules-of-hooks const renderedLocationKey = useStore( router.stores.resolvedLocation, @@ -256,21 +265,23 @@ function OnRendered() { // eslint-disable-next-line react-hooks/rules-of-hooks useLayoutEffect(() => { - const currentHref = router.latestLocation.href + const currentResolvedLocation = router.stores.resolvedLocation.get() + const previousResolvedLocation = prevResolvedLocationRef.current if ( - prevHrefRef.current === undefined || - prevHrefRef.current !== currentHref + currentResolvedLocation && + (!previousResolvedLocation || + previousResolvedLocation.href !== currentResolvedLocation.href) ) { router.emit({ type: 'onRendered', ...getLocationChangeInfo( router.stores.location.get(), - router.stores.resolvedLocation.get(), + previousResolvedLocation ?? currentResolvedLocation, ), }) - prevHrefRef.current = currentHref } + prevResolvedLocationRef.current = currentResolvedLocation }, [renderedLocationKey, router]) return null From 3579a4ce7727c2a91389a496c3e076b4855ddd7c Mon Sep 17 00:00:00 2001 From: Sheraff Date: Thu, 11 Jun 2026 09:36:42 +0200 Subject: [PATCH 5/5] lint rules of hooks --- packages/react-router/src/Match.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-router/src/Match.tsx b/packages/react-router/src/Match.tsx index acecde9043..5de5546aeb 100644 --- a/packages/react-router/src/Match.tsx +++ b/packages/react-router/src/Match.tsx @@ -247,13 +247,13 @@ function OnRendered() { return null } - // eslint-disable-next-line react-hooks/rules-of-hooks - // @ts-expect-error -- init to `undefined` but don't write `undefined` to shave bytes // Track the resolvedLocation as of the last render so that onRendered can // report the correct fromLocation. By the time this effect fires, // resolvedLocation has already been updated to the new location by // Transitioner, so we cannot use router.stores.resolvedLocation.get() // directly as the fromLocation. + // @ts-expect-error -- init to `undefined` but don't write `undefined` to shave bytes + // eslint-disable-next-line react-hooks/rules-of-hooks const prevResolvedLocationRef = React.useRef< ParsedLocation | undefined >()