Cache substitution types #30775
Cache substitution types #30775
Conversation
| @@ -31,7 +31,7 @@ class A { | |||
| >z : A[] | |||
|
|
|||
| whereRelated< // Works // Type is same as A1, but is not assignable to type A | |||
| >whereRelated : <RF extends RelationFields = RelationFields, N extends "x" | "y" | "z" = "x" | "y" | "z", A1 extends A = RF[N] extends A[] ? RF[N][0] : never, A2 extends A = ShouldA<RF, N>>() => number | |||
There was a problem hiding this comment.
ShouldA<RF, N> is identical to RF[N] extends A[] ? RF[N][0] : never, and we make the second before the aliased version, so we cache a version with no alias symbol. Previously we'd make two different types because of the substitution in the true branch. :(
|
Normally it just goes down by a few types when there are, eg, multiple |
|
So is the purpose of this PR completeness/symmetry instead of performance or correctness? |
|
No, it's perf - this is a prereq for any other perf fix that relies on correct conditional type caching to really work. It's more like this acts as a multiplier on degenerate perf issues we have. Fixing it doesn't remove the perf issue, but it does shave a few seconds off the 6s to 90s perf regression reported in another issue (though it doesn't bring it back down to 6s). This is a straight improvement - we should have been caching these from the start - it was an oversight on our part; not caching them defeats our other caching layers, which is not the intent at all. |
|
If this is a perf improvement, can you please post a diff of |
There was a problem hiding this comment.
but it does shave a few seconds off the 6s to 90s perf regression
What am I missing that would make this possible? The type construction that this short-circuits appears to be really cheap; my intuition would think similar cost to generating the id and looking it up in the map?
Cacheing the type causes the type to share identity - the type sharing identity causes relationship comparisons to be shared, which otherwise would have been repeated. For deep structures, these comparisons are costly. |
|
@typescript-bot perf test |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 3fa4fce. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
(Although maybe the bot's perf test won't be the best example and won't show a diff, since it's not really the pathological case; we'll see) |
@andrewbranch Wesley's answer is probably right. In some other cases, lots of allocations of new types means you add GC pressure which leads to heap thrashing. Not sure if that's really the culprit though.
@weswigham right I meant the pathological case diff. |
|
@weswigham Comparison Report - master..30775
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Whoa, so apparently I lied. This is the fix for #30663. With this alone: this PR + master: for some reason I thought I still had other things that were needed to fix this but I guess not |
|
OK! There was more that was needed alongside this to fix which is an improvement, but not a fix. Yet... it is fixed in |
|
@weswigham I saw that you didn't push the cherry-pick for 3.4. Is it planned to release this improvement with 3.4.3? |
|
Well, this is a significant improvement for 3.4 - 90s down to 70s is notable; but it's not the order-of-magnitude improvement Maybe we'll reconsider, but since |
|
Hmm.. In WebStorm I still have an ~8 sec lag after every code change before the language service finishes and the updated squiggles appear. (With the BTW: I can confirm that |
This doesn't fully fix anything, per se, but it does reduce the compounding effects in some of our outstanding perf issues a bit. We cache conditional types heavily, but because we failed to cache substitution types (which are manufactured within conditional type
truebranches), we'd end up making way more conditional type identities than we thought. These not being cached is just an oversight on our part - they're essentially "invisible" intersections, which are already cached.Fixes #30663.