perf: geting lazy?, doing less work? generate more (plus some recommendations) #1564

chorobin · 2024-05-06T14:10:26Z

Getting lazy? Doing less work? and generating more (plus some recommendations)

This PR focuses more on complex routes and to some degree many routes. When I say a complex route, I mean routes which have search, params, loaders and external dependencies such as zod or other heavy TS related dependencies.

A bigger issue I've noticed in TSR is in order to get suggestions, the route tree needs to be evaluated. If your routes contain complex things that TS needs to infer like validation schemas then this can slow down TS evaluating the route tree because its hard at work on the types of the validation libraries. But for most suggestions, we only need the fullPath, we don't need to work out the search schema or the params in order to get suggestions for Link for example until search is used. The ideal scenario is we do as little as possible in order to get the suggestions we need

Show me the results!

I've updated our large filed based example to include:

100 routes for absolute pathing
100 routes for relative pathing
100 routes for params with loader
100 routes for search with loader

These include external dependencies which also do TS inference. Note the results may vary depending on what dependencies you use and how the types are written

On main:

> tsc --extendedDiagnostics

Files:                         636
Lines of Library:            38118
Lines of Definitions:        80134
Lines of TypeScript:         18481
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                132113
Symbols:                    345367
Types:                      119980
Instantiations:            2720561
Memory used:               345796K
Assignability cache size:    56379
Identity cache size:          8114
Subtype cache size:           2022
Strict subtype cache size:       0
I/O Read time:               0.09s
Parse time:                  0.67s
ResolveModule time:          0.15s
ResolveTypeReference time:   0.00s
ResolveLibrary time:         0.02s
Program time:                1.03s
Bind time:                   0.29s
Check time:                  7.25s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  8.57s

On current branch:

> tsc --extendedDiagnostics

Files:                         636
Lines of Library:            38118
Lines of Definitions:        80143
Lines of TypeScript:         19712
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                133346
Symbols:                    322837
Types:                      111429
Instantiations:             773068
Memory used:               327186K
Assignability cache size:    45204
Identity cache size:          8109
Subtype cache size:           2022
Strict subtype cache size:       0
I/O Read time:               0.08s
Parse time:                  0.59s
ResolveModule time:          0.12s
ResolveTypeReference time:   0.00s
ResolveLibrary time:         0.02s
Program time:                0.92s
Bind time:                   0.28s
Check time:                  5.51s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  6.71s

Getting lazy? Doing less work?

In TSR one of the largest performance bottlenecks is the route tree building. This node is important to reduce because it is directly linked to TS server suggestion times. What is contributing to this node being so slow is firstly, a lot of work upfront to evaluate search, loader, context and the others, which are possibly inferred from external libraries.

The first interesting change this PR introduces is not a breaking one but introducing a new signature to addChildren. addChildren accepts a tuple of routes like the following:

route.addChildren([childRoute1, childRoute2])

addChildren also in addition can now accept an object route.addChildren({ childRoute1, childRoute2 }).

Why this change? Well, it looks like tuples seem to be forcing TS to be doing something in each route which is quite slow and it gets worse with different external dependencies, where objects seem to be more cheap or perhaps more lazy in nature, maybe there's also caching involved? Depending on the libraries used in for example validateSearch, you may notice a difference in performance improvements.

For example, using valibot instead of zod in the above example with addChildren that accept a tuple

> tsc --extendedDiagnostics

Files:                          638
Lines of Library:             38680
Lines of Definitions:         86278
Lines of TypeScript:          19809
Lines of JavaScript:              0
Lines of JSON:                    0
Lines of Other:                   0
Identifiers:                 141166
Symbols:                     270865
Types:                        90876
Instantiations:            22514251
Memory used:                310778K
Assignability cache size:     33466
Identity cache size:           7787
Subtype cache size:            2830
Strict subtype cache size:        1
I/O Read time:                0.09s
Parse time:                   0.63s
ResolveModule time:           0.14s
ResolveTypeReference time:    0.01s
ResolveLibrary time:          0.02s
Program time:                 1.03s
Bind time:                    0.30s
Check time:                  14.66s
printTime time:               0.00s
Emit time:                    0.00s
Total time:                  15.99s

Now with addChildren that accepts an object

> tsc --extendedDiagnostics

Files:                         638
Lines of Library:            38680
Lines of Definitions:        86278
Lines of TypeScript:         19813
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                141170
Symbols:                    269855
Types:                       90640
Instantiations:            1093144
Memory used:               316056K
Assignability cache size:    33455
Identity cache size:          7787
Subtype cache size:           2830
Strict subtype cache size:       1
I/O Read time:               0.09s
Parse time:                  0.68s
ResolveModule time:          0.14s
ResolveTypeReference time:   0.01s
ResolveLibrary time:         0.02s
Program time:                1.09s
Bind time:                   0.35s
Check time:                  5.35s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  6.79s

That's a big difference between tuples and objects. I'm still not totally sure why tuples behave this way or if I'm even correct about my assumptions but to me it looks like tuples are forcing some kind of extra work the TS compiler is doing and its related to validateSearch and how the types of the external library are likely written.

Profile

The first link that is checked by TS has to evaluate the route tree and it looks like this with addChildren accepting a tuple

The same node with addChildren accepting an object looks like this

But thats not the end of the story. Each individual Link is also faster with children as an object. Each Link is checked with the following time when addChildren accepts a tuple

And each Link is checked with the following time with addChildren accepts an object

As mentioned before. 30ms might not seem like much. But if you have 30ms for every Link in your code base, it quickly accumulates. 4ms is a huge difference in these cases

In this PR I have switched file based routing over to using objects automatically for a default performance boost. Code based can opt into the signature.

Other stuff

Generating more

File based routing previously has done a lot of string interpretation on the type level in order to calculate things like fullPath, path, id etc. Template literal types are expensive for the compiler and generally in file based routing, these strings can be generated directly by the generator without hurting inference. This speeds up creating routes

Improve params parsing

A more direct implementation of path params parsing without any additional splitting or mapped types

What can blow up your TS Performance

With TSR you want to narrow to the most relevant routes. This means using from and to on Link or other API's.

For example, <Link search={{ page: 0 }} /> will increase the type checking time on this link because TSR will resolve search to all possible routes (a union of all search params) and will therefore spend longer type checking it. This depends on the size of your route tree and how many search params you have but on our large example, I noticed a single Link would type check in roughly 100ms.

It is better to narrow the route as much as you can <Link from={Route.fullPath} search={{ page: 0 }} /> with from or to. In this case the time type checking Link went down to 9ms.

This same logic also applies to LinkOptions. If you're using types from TSR which are by default not narrowed to a route then this can also blow up TS performance when merging this props with Link. Just remember that when from or to is not specified then things can start slowing down, so definitely try to do it as little as possible.

nx-cloud · 2024-05-06T14:10:53Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 236d63c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

chorobin · 2024-05-06T14:11:37Z

Still some work needs to be done so not ready just yet

packages/router-generator/src/generator.ts

packages/react-router/src/route.ts

schiller-manuel · 2024-05-06T15:41:12Z

packages/react-router/src/route.ts

-  addChildren = <const TNewChildren extends ReadonlyArray<AnyRoute>>(
+  addChildren = <
+    const TNewChildren extends
+      | Record<string, AnyRoute>


should this new object syntax be documented?

Added to the performance recommendations

schiller-manuel · 2024-05-06T15:42:01Z

we should add that What can blow up your TS Performance section into the documentation

chorobin · 2024-05-06T16:59:53Z

we should add that What can blow up your TS Performance section into the documentation

Definitely. Think we need to write a section on it so they can go to it and be able to resolve common problems

chorobin · 2024-05-08T10:02:10Z

packages/react-router/src/fileRoute.ts

-    TSearchSchemaUsed = TSearchSchemaInput extends SearchSchemaInput
-      ? Omit<TSearchSchemaInput, keyof SearchSchemaInput>
-      : TSearchSchema,
+    TSearchSchemaInput = {},


I realized these constraints were not necessary because they were just constrained to {} which is already extremely loose

chorobin · 2024-05-08T10:12:01Z

packages/router-generator/tests/generator/nested-layouts/routeTree.expected.ts

@@ -138,78 +144,142 @@ const JestedLayoutB3LayoutC2BarRoute = JestedLayoutB3LayoutC2BarImport.update({
 declare module '@tanstack/react-router' {


@schiller-manuel I've added conditions for the tests in the generator for fullPath, id, path. Are there any situations missing here that I should add?

packages/router-generator/src/generator.ts

schiller-manuel · 2024-05-09T17:48:18Z

packages/router-generator/src/generator.ts

+        )
+
+        return `'${filePathId}': {
+          id: '${id}'


do we need both id and path?

I thought id was slightly different?

~~Correct me if I'm wrong, but you are explicitly setting these values on the declaration so you don't need to resolve the inference again via the TS-resolve types, yes?~~
Edit: Ignore this comment, I didn't properly read the PR description.

Also, just want to confirm that we don't end up in a scenario that a created route configuration (not route type declaration) doesn't ever have both an id and a path.

Like:

const fooRoute = fooImport.update({ id: 'foo', path: '/foo' })

This only affects the type that's printed for module declaration.

I'm not sure if I get your point. I think file based routing always had id and path in the types?

Check more into this, it looks good.

Just wanted to make sure we weren't modifying anything in route configuration, and only touching stuff on the type declaration.

chorobin · 2024-05-11T04:57:17Z

packages/react-router/src/fileRoute.ts

-      TRouteContext
-    >,
-    TRouterContext extends RouteConstraints['TRouterContext'] = AnyContext,
+    TAllParams = ResolveAllParamsFromParent<TParentRoute, TParams>,


It's a bit faster to name all complex types

chorobin · 2024-05-11T09:21:53Z

Don't merge yet. Need to double check some things. Ideas 💡

schiller-manuel · 2024-05-11T10:49:26Z

@SeanCassiere in light of #1584, can you please review this?

SeanCassiere

I don't have the best understanding of all the ts-stuff we do with the router, but trying it out on my machine with out examples, the functionality works identical to what it was like before.

Left some comments, but nothing too crazy, all-in-all amazing, amazing, amazing, work @chorobin 🙌🏼!

packages/router-generator/src/generator.ts

SeanCassiere · 2024-05-11T11:24:35Z

packages/router-generator/src/generator.ts

+        )
+
+        return `'${filePathId}': {
+          id: '${id}'


~~Correct me if I'm wrong, but you are explicitly setting these values on the declaration so you don't need to resolve the inference again via the TS-resolve types, yes?~~
Edit: Ignore this comment, I didn't properly read the PR description.

SeanCassiere · 2024-05-11T11:24:39Z

packages/router-generator/src/generator.ts

+        )
+
+        return `'${filePathId}': {
+          id: '${id}'


Also, just want to confirm that we don't end up in a scenario that a created route configuration (not route type declaration) doesn't ever have both an id and a path.

Like:

const fooRoute = fooImport.update({ id: 'foo', path: '/foo' })

This only affects the type that's printed for module declaration.

packages/router-generator/src/generator.ts

SeanCassiere · 2024-05-11T11:42:00Z

packages/react-router/src/link.tsx

-export type ParsePathParams<T extends string> = keyof {
-  [K in Trim<Split<T>[number], '_'> as K extends `$${infer L}`
-    ? L extends ''
+export type ParsePathParams<


This comment may not really belong here at this line, rather it's something I noticed about the link component when testing out the PR.

In the "kitchen-sink-file-based" example, using a <Link> component it accepts the following to values.

/route-group

route-group

1 works as expected, but 2 then says it requires params.

Property 'params' is missing in type '{ to: "route-group"; }' but required in type 'MakeRequiredPathParams<Router<Route<any, "/", "/", string, "__root__", RootSearchSchema, RootSearchSchema, RootSearchSchema, RootSearchSchema, ... 10 more ..., { ...; }>, TrailingSlashOption, Record<...>, Record<...>>, string, "route-group">'.ts(2741)

Also, something else, I noticed is that the from property doesn't accept just route-group, only allows /route-group unlike the to property shown above.

Yeah, its a separate issue I'm going to be fixing in a separate PR. Its a bug on main as well.

SeanCassiere · 2024-05-11T11:43:05Z

examples/react/basic-file-based/src/routeTree.gen.ts

@@ -73,38 +73,65 @@ const LayoutLayout2LayoutARoute = LayoutLayout2LayoutAImport.update({
 declare module '@tanstack/react-router' {


We'll need to generate the route-trees on all the file-based examples as well and start migrating all the code-based examples over to the object-syntax.

while I agree we should re-generate the route-trees of file based examples, I don't think we MUST migrate the code based ones.

Let's slow down a bit. I'm exploring another idea that I really hope means we can not introduce a new signature. But we will see! I'm not sure if it'll work yet

Ok. Turns out the idea is again limited by the children being tuples, it likely forcing unnecessary instantiations...

We need to keep the signature to allow objects for children

SeanCassiere · 2024-05-11T11:43:43Z

docs/framework/react/guide/type-safety.md

+<Link from={from} to='..' />
+```
+
+### Consider using the object syntax of `createChildren`


I believe this should be addChildren

SeanCassiere · 2024-05-11T12:01:41Z

Also, we probably want to using the feat: or refactor: prefix on the commit name. Not sure if we are using conventional commits or angular commits, but if I've not mistaken on one of them using perf: triggers a major version bump.

SeanCassiere · 2024-05-11T12:05:46Z

@SeanCassiere in light of #1584, can you please review this?

Trying out the changes from my previous PR on this branch yielded no collisions or regressions that I could observe on that specific layout route behaviour.

That being said, @chorobin once the merge conflict has been resolved, you'd want to rebase and confirm the tests I introduced in that PR still work.

schiller-manuel · 2024-05-11T13:54:42Z

docs/framework/react/guide/type-safety.md

+
+## Performance Recommendations
+
+As your application scales, typescript check times will naturally increase. There are a few things to keep in mind when your application scales to keep your TS check times down.


typescript => TypeScript

schiller-manuel · 2024-05-11T13:55:17Z

docs/framework/react/guide/type-safety.md

+<Link search={{ page: 0 }} />
+```
+
+**These examples are bad for TS performance**. Thats because `search` resolves to a union of all `search` params for all routes and TS has to check whatever you pass to the `search` prop against this potentially big union. As your application grows, this check time will increase linearly to number of routes and search params. We have done our best to optimise for this case (typescript will typically do this work once and cache it) but the initial check against this large union is expensive. This also applies to `params` and other API's such as `useSearch`, `useParams`, `useNavigate` etc


Thats => That's

docs/framework/react/guide/type-safety.md

schiller-manuel · 2024-05-11T14:10:46Z

packages/react-router/src/index.tsx

@@ -71,6 +64,7 @@ export {
  type UseLinkPropsOptions,
  type ActiveLinkOptions,
  type LinkProps,
+  type LinkComponentProps,


do we really want to export this type? what would this be used for?

chorobin · 2024-05-12T20:45:35Z

Also, we probably want to using the feat: or refactor: prefix on the commit name. Not sure if we are using conventional commits or angular commits, but if I've not mistaken on one of them using perf: triggers a major version bump.

I've never heard of this. In conventional commits I thought you had to explicitly mention ! or BREAKING CHANGE to bump major version

schiller-manuel · 2024-05-12T20:49:16Z

this can be seen here: https://github.com/TanStack/config/blob/main/src/publish/index.js#L144

perf will trigger a patch release.

chorobin · 2024-05-12T21:48:31Z

packages/react-router/src/link.tsx

-> = ToSubOptions<TRouter, TFrom, TTo> & {
+> = ToSubOptions<TRouter, TFrom, TTo> & MaskOptions<TRouter, TMaskFrom, TMaskTo>
+
+export interface MaskOptions<


Naming all of these types helps TS cache stuff when passing LinkProps to a Link. Slightly more optimised

- add type tests for objects with children - add runtime support for objects with children - add runtime tests for objects with children

schiller-manuel reviewed May 6, 2024

View reviewed changes

packages/router-generator/src/generator.ts Show resolved Hide resolved

schiller-manuel reviewed May 6, 2024

View reviewed changes

packages/react-router/src/route.ts Outdated Show resolved Hide resolved

schiller-manuel reviewed May 6, 2024

View reviewed changes

chorobin commented May 8, 2024

View reviewed changes

packages/router-generator/src/generator.ts Show resolved Hide resolved

chorobin force-pushed the get-lazy-gen branch from d155a42 to d474ec9 Compare May 8, 2024 10:16

schiller-manuel reviewed May 9, 2024

View reviewed changes

chorobin force-pushed the get-lazy-gen branch from 81b994a to b09ebe2 Compare May 10, 2024 13:11

chorobin marked this pull request as ready for review May 10, 2024 13:15

chorobin commented May 11, 2024

View reviewed changes

SeanCassiere reviewed May 11, 2024

View reviewed changes

schiller-manuel reviewed May 11, 2024

View reviewed changes

docs/framework/react/guide/type-safety.md Outdated Show resolved Hide resolved

schiller-manuel reviewed May 11, 2024

View reviewed changes

docs/framework/react/guide/type-safety.md Outdated Show resolved Hide resolved

schiller-manuel reviewed May 11, 2024

View reviewed changes

docs/framework/react/guide/type-safety.md Show resolved Hide resolved

schiller-manuel reviewed May 11, 2024

View reviewed changes

chorobin commented May 12, 2024

View reviewed changes

chorobin force-pushed the get-lazy-gen branch from 6677313 to 71bd768 Compare May 12, 2024 22:03

chorobin added 10 commits May 13, 2024 00:06

perf: get lazy? do less?, generate more

ecb2f40

feat: add support for objects for children at runtime

9ca1577

- add type tests for objects with children - add runtime support for objects with children - add runtime tests for objects with children

perf: apply similar optimisations for merging for code based

3c748ec

perf: improve performance of LinkProps when passed to Link

3933c75

exmaples: add to large based file examples search, params and context

b4ef1bd

docs: add TS performance recommendations to docs

ac74ae7

fix: remove route groups from id

589573b

examples: update linkprops examples

1719bf6

docs: update docs based on comments

dc82b61

chore: remove export of LinkComponentProps

1a7705b

chorobin force-pushed the get-lazy-gen branch 3 times, most recently from c772e8b to cd8d216 Compare May 12, 2024 22:22

chore: clean up PR

1b41048

chorobin force-pushed the get-lazy-gen branch from cd8d216 to 1b41048 Compare May 12, 2024 22:39

lachlancollins mentioned this pull request May 13, 2024

fix(publish): Remove scripts section TanStack/config#84

Merged

Merge branch 'main' into get-lazy-gen

236d63c

chorobin merged commit 0898f2e into main May 13, 2024
7 checks passed

chorobin deleted the get-lazy-gen branch May 13, 2024 14:54

		@@ -138,78 +144,142 @@ const JestedLayoutB3LayoutC2BarRoute = JestedLayoutB3LayoutC2BarImport.update({
		declare module '@tanstack/react-router' {

		@@ -73,38 +73,65 @@ const LayoutLayout2LayoutARoute = LayoutLayout2LayoutAImport.update({
		declare module '@tanstack/react-router' {


		## Performance Recommendations

		As your application scales, typescript check times will naturally increase. There are a few things to keep in mind when your application scales to keep your TS check times down.

perf: geting lazy?, doing less work? generate more (plus some recommendations) #1564

perf: geting lazy?, doing less work? generate more (plus some recommendations) #1564

Conversation

chorobin commented May 6, 2024 • edited Loading

Getting lazy? Doing less work? and generating more (plus some recommendations)

Show me the results!

Getting lazy? Doing less work?

Profile

Other stuff

Generating more

Improve params parsing

What can blow up your TS Performance

nx-cloud bot commented May 6, 2024 • edited Loading

☁️ Nx Cloud Report

chorobin commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schiller-manuel commented May 6, 2024

chorobin commented May 6, 2024

chorobin May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanCassiere May 11, 2024 • edited Loading

Choose a reason for hiding this comment

SeanCassiere May 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chorobin commented May 11, 2024

schiller-manuel commented May 11, 2024

SeanCassiere left a comment

Choose a reason for hiding this comment

SeanCassiere May 11, 2024 • edited Loading

Choose a reason for hiding this comment

SeanCassiere May 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanCassiere commented May 11, 2024

SeanCassiere commented May 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chorobin commented May 12, 2024

schiller-manuel commented May 12, 2024 • edited Loading

Choose a reason for hiding this comment

chorobin commented May 6, 2024 •

edited

Loading

nx-cloud bot commented May 6, 2024 •

edited

Loading

chorobin May 8, 2024 •

edited

Loading

SeanCassiere May 11, 2024 •

edited

Loading

SeanCassiere May 11, 2024 •

edited

Loading

SeanCassiere May 11, 2024 •

edited

Loading

SeanCassiere May 11, 2024 •

edited

Loading

SeanCassiere commented May 11, 2024 •

edited

Loading

schiller-manuel commented May 12, 2024 •

edited

Loading