runem / lit-analyzer

Monorepository for tools that analyze lit-html templates
MIT License
318 stars 36 forks source link

ts-simple-type performance #103

Closed 43081j closed 4 years ago

43081j commented 4 years ago

I'm not sure if this is in ts-simple-type or in lit-analyzer, but there's a huge performance bottleneck somewhere i struggled to track down fully myself.

Basically, when we visit a html assignment, we extract the types of the left and right:

https://github.com/runem/lit-analyzer/blob/950d0e34e7aaa588d53a0146a71dafdd317f22c1/packages/lit-analyzer/src/rules/no-complex-attribute-binding.ts#L22

Within extractBindingTypes, we then compute the right hand side's "simple type":

https://github.com/runem/lit-analyzer/blob/950d0e34e7aaa588d53a0146a71dafdd317f22c1/packages/lit-analyzer/src/rules/util/type/extract-binding-types.ts#L36-L39

This is all fine, but we hit a strange performance issue when we have something like this:

_onKeyUp(ev: KeyboardEvent): void {
  // ...
}
render() {
  return html`<input @keyup=${this._onKeyUp}>`;
}

What this results in is ts-simple-type recursively trying to "resolve" the types and turn them into "simple types". Again, fine.

Until we hit the KeyboardEvent type and try resolve it.. we then hit this:

https://github.com/microsoft/TypeScript/blob/89f7797f7e7c3985285ad8113eccf560bf9fecde/lib/lib.dom.d.ts#L1828-L1831

which as you can see references Window. From then on, it often (almost always) takes 500ms+ just to parse whatever "Window" is, jumping around all over the place...

this results in the majority of my analysis CPU time being spent on resolving types we ultimately don't care about (~9 seconds in my case). im not sure how you'd improve this other than by having a recursion limit or something...

runem commented 4 years ago

I have been aware of this bottleneck in ts-simple-type for quite some time now. After finishing the work on 1.2.0, I'll begin improving this area. I expect this performance bottleneck to be fixed by evaluating types lazily instead.

43081j commented 4 years ago

yup while wondering about this i realised too that lazy types would fix all of it. since it rarely, if ever, has to convert types that deep (especially given the short-circuiting in the convert function).

you'd shave several seconds off my builds if you did that 👍

runem commented 4 years ago

I just introduced a "lazy type" to ts-simple-type (see https://github.com/runem/ts-simple-type/pull/62) alongside a lot of much needed improvements to the type checker :tada: I'll soon be updating the dependency on ts-simple-type :-)

runem commented 4 years ago

My initial tests shows speed improvements of up to 15 times in a project with a lot of complex types :-)

43081j commented 4 years ago

thats great to hear. if you publish a version to npm at some point ill give it a go on our projects too. the main perf loss was on deep type evaluation (i.e. following Window recursively).

runem commented 4 years ago

@43081j I just published a beta version that you are very welcome to try :-)

npx lit-analyzer@1.2.0-next.1 src

I look forward hearing how much the performance has improved for your project!

43081j commented 4 years ago

Next branch: ~25s Master: ~160s

not too bad 👍 good work

runem commented 4 years ago

I just made another improvement to ts-simple-type that in some cases dramatically improves speed when type checking in a large project (500+ files) with a lot of bindings. It would be very interesting to see if the change made any noticeable performance improvements when running it against your project.

You can try it by running: npx lit-analyzer@next

43081j commented 4 years ago

10s roughly, whatever you changed was a good change haha.

well done

do you have a link to the PR/commit that introduced it? out of interest

runem commented 4 years ago

I'm really happy to hear that! I think lit-analyzer is on an acceptable level of performance for now :+1:

The problem was actually really trivial to solve and was just a slip up. When type checking, ts-simple-type (the new, refactored version) caches results of comparisons and has a separate cache for each combination of strict, strictNullChecks and strictFunctionTypes because they all affect the outcome of the type checking. When I built the cache, I simply made the cache key using JSON.stringify (don't ask me why) and forgot all about it. This is of course a big performance problem, especially because I later on added more properties to the config, and because the order of properties on the config can change. The performance bottleneck was noticeable when type checking a lot of bindings because the line added 2ms-4ms to every call to type checking. So here you go, the fix was a one line: https://github.com/runem/ts-simple-type/commit/4dfc3e7f09fdbc7a26c09c7891189c42f3b5fe3a :-)

43081j commented 4 years ago

wow that just shows how a small slow down can blow up. we have a lot of files in this project so makes sense it'd shave off so many seconds. nice work again, i'd say its definitely performant now

runem commented 4 years ago

Thanks! Yeah, shaving off even just 1ms could give a huge performance boost. I think there might also be more low-hanging-fruit-improvements, but I'll focus on getting v1.2.0 ready for a release for now.