router icon indicating copy to clipboard operation
router copied to clipboard

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

Open chorobin opened this issue 1 year ago • 4 comments

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

image

The same node with addChildren accepting an object looks like this

image

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

image

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

image

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.

chorobin avatar May 06 '24 14:05 chorobin

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 236d63c417f90fa44bf95e3fc16bd727c9634286. 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.

nx-cloud[bot] avatar May 06 '24 14:05 nx-cloud[bot]

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

chorobin avatar May 06 '24 14:05 chorobin

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

schiller-manuel avatar May 06 '24 15:05 schiller-manuel

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 avatar May 06 '24 16:05 chorobin

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

chorobin avatar May 11 '24 09:05 chorobin

@SeanCassiere in light of https://github.com/TanStack/router/pull/1584, can you please review this?

schiller-manuel avatar May 11 '24 10:05 schiller-manuel

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 avatar May 11 '24 12:05 SeanCassiere

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

SeanCassiere avatar May 11 '24 12:05 SeanCassiere

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

chorobin avatar May 12 '24 20:05 chorobin

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

perf will trigger a patch release.

schiller-manuel avatar May 12 '24 20:05 schiller-manuel