Skip nested references in relation checking #17947

sandersn · 2017-08-21T16:23:13Z

This change makes relating the following types faster:

interface B<T> {
  contents: T[]
  map<U>(f: (t: T) => U): B<U>
}
interface D<T> extends B<T> {
  contents: List<T>
  map<U>(f: (t: T) => U): D<U>
}

Previously, relating the return types of map, D ===> B, would cause a recursive check. This recursion would continue until the depth limit was hit. However, no new information will result from relating D ===> B vs D<T> ===> B<T> — one type parameter is interchangeable with any other in terms of relatability (with the exception of constrained type parameters).

The resulting rule is as follows: return Ternary.Maybe when relating

Two type references
With identical type arguments
Where the two type references are identical to a pair that is
already being compared in an outer recursion.
And where the constraints of the currently-compared type arguments
are identical to the constraints of the outer-compared of type
arguments.

For example, if relation checking has already started comparing A<T> ===> B<T>, then if it checks A ===> B in a recursive call to isRelatedTo, the previous rules will immediately return Ternary.Maybe, causing relation checking to move on to the rest of the type for checking relatibility. Here are some counter examples:

A ===> B<V>; type arguments are not identical.
A ===> B<number>; type arguments are not identical.
A ===> C; the pair A,C is not currently being compared.
A ===> B; U's constraint V does not
match T's constraint undefined.

Note condition (2) happens when a generic signature is instantiated with the type arguments of another signature when checking assignability. This is a common check, so skipping it should provide a decent speed improvement.

Next steps:

Test as many oomemory crashes as I can find from the last two releases.
This change helps collection libraries, which have lots and lots of generic signatures that look a lot like the example above. Specifically, this change fixes immview mentioned in Upgrading to 2.4.1 runs out of memory. #17097 and immutable-js from Out of memory error when using ImmutableJS #17070. I haven't included tests yet, but a miniaturised version of this library or a similar. I have quite a few repros lying around that crash the compiler with only 10 lines of code or so.
I am not sure that rule (4) is necessary; it would be simpler to just not implement this speedup for type arguments with constraints. That would change it to "And where the type arguments do not have constraints".
After I have a simple test case in place, I'll try to get performance numbers on this change. I'm not sure that the constraint fanciness is needed, so I'll get numbers for both versions of rule (4).
Then I'll run this change on the RWC and DefinitelyTyped to make sure nothing breaks.

commit 6ab553c Author: Nathan Shively-Sanders <nathansa@microsoft.com> Date: Mon Aug 21 08:38:43 2017 -0700 Use Map instead of array. More refactoring/renaming. commit 1e8f244 Author: Nathan Shively-Sanders <nathansa@microsoft.com> Date: Fri Aug 18 16:30:05 2017 -0700 Refactor change to be readable (ish?) commit 2bd0749 Author: Nathan Shively-Sanders <nathansa@microsoft.com> Date: Fri Aug 18 15:24:33 2017 -0700 Revert "cache relationships, needs work on preserving diagnostic output" This reverts commit e7f5c96. commit e7f5c96 Author: Wesley Wigham <wewigham@microsoft.com> Date: Fri Aug 18 14:04:46 2017 -0700 cache relationships, needs work on preserving diagnostic output commit 3eaadc6 Author: Nathan Shively-Sanders <nathansa@microsoft.com> Date: Fri Aug 18 14:06:08 2017 -0700 in isRelatedTo, skip repeated comparisons of generic types

This change makes relating the following types faster: ```ts interface B<T> { contents: T[] map(f: (t: T) => U): B } interface D<T> extends B<T> { contents: List<T> map(f: (t: T) => U): D } ``` Previously, relating the return types of map, `D ===> B`, would cause a recursive check. This recursion would continue until the depth limit was hit. However, no new information will result from relating `D ==> B` vs `D<T> ==> B<T>` — one type parameter is interchangeable with any other in terms of relatability (with the exception of constrained type parameters). The resulting rule is as follows: return Ternary.Maybe when relating 1. Two type references 2. With identical type arguments 3. Where the two type references are identical to a pair that is already being compared in an outer recursion. 4. And where the constraints of the currently-compared type arguments are identical to the constraints of the outer-compared of type arguments. For example, if relation checking has already started comparing `A<T> ==> B<T>`, then if it checks `A ==> B` in a recursive call to `isRelatedTo`, the previous rules will immediately return `Ternary.Maybe`, causing relation checking to move on to the rest of the type for checking relatibility. Here are some counter examples: 1. `A ==> B<V>`; type arguments are not identical. 2. `A ==> B<number>`; type arguments are not identical. 3. `A ==> C`; the pair `A,C` is not currently being compared. 4. `A ==> B`; U's constraint `V` does not match T's constraint `undefined`. Note condition (2) happens when a generic signature is instantiated with the type arguments of another signature when checking assignability. This is a common check, so skipping it should provide a decent speed improvement.

weswigham · 2017-08-21T16:45:08Z

src/compiler/checker.ts

@@ -9230,6 +9263,10 @@ namespace ts {
                        return Ternary.False;
                    }
                }
+                if (alreadyComparingTypeReferences(source, target)) {
+                    return Ternary.Maybe;


Should this be cached in maybeKeys?

After discussing it with you: probably yes, as long as we only cache non-constrained type parameters. With the combined maybeKeys, when checking A<T> ==> B<T> and then A ==> B, when we skip A===>B with Ternary.Maybe and later find that they are assignable, we'll also mark A<T>==>B<T> as assignable. Which is what we want.

sandersn · 2017-08-21T23:01:34Z

Some updates:

Based on @weswigham's comment, I changed to using the existing caching, so that when a particular A ===> B is proven to be related, the uninstantiated A<T> ===> B<T> is also proven to be related. This doesn't seem to change performance much, although I haven't taken precise measurements.
ImmutableJS no longer crashes with this change. However, monet.js and ng-typeview (mentioned in [2.4.1] stricter generic checks causes out-of-memory crash #16777) still crash with out-of-memory. I need to investigate them separately.
In our real-world code corpus, fp-ts and RxJs choose different properties when elaborating errors. This is a harmless change.
A couple of our angular sample apps are missing the last 18 (!) elaboration lines of a 23-line (!!) error message. It's the only error in the app, so I don't think it's directly due to caching. And the error is from angular.d.ts.

sandersn · 2017-08-21T23:40:55Z

After the latest commit, the angular sample now just has a different elaboration, like the diffs for RxJs et al. So I think switching to use the shared maybe stack improves the error reporting as well.

ahejlsberg · 2017-08-22T09:23:43Z

I think this can be simplified and optimized a bit. I just pushed a branch named "typeReferenceRelations" that demonstrates what I mean. It constructs special keys for type references containing unconstrained type parameter arguments wherein such type arguments are just given simple numerical indices. Beyond key construction, no other logic is changed. This scheme should work better for type references with non-constrained type arguments or unequal numbers of arguments. For example, A<T, U, number> ===> B<U, string, T> has the same key as A<X, Y, number> ===> B<Y, string, X>.

Haven't had a chance to test it much yet, but perhaps you can.

sandersn · 2017-08-22T19:21:49Z

I like the better integration with the existing caching, but I don't know whether the extra work to handle type arguments besides unconstrained type variables will apply often enough to save much. On the other hand, it's only a little more work, so why not? I'll test both to see whether I can find a performance difference.

sandersn · 2017-08-23T16:55:54Z

I tried making that simpler check I referenced, but the resulting code wasn't that much simpler and the performance wasn't any better either.

So far, Immutable and immview compile about 10% faster with typeReferenceRelations than with skip-nested-references-in-assignability. I'm still running various performance tests, but so far Monaco and TFS don't show any difference in compile time. It may be that this change only helps collections-like libraries. I'll add the numbers when I get them.

sandersn · 2017-08-23T20:25:05Z

This PR is obsoleted by #17984, which does the same thing but slightly better.

sandersn added 2 commits August 21, 2017 08:42

msftclas added the cla-not-required label Aug 21, 2017

sandersn requested review from ahejlsberg, weswigham and mhegazy August 21, 2017 16:23

Fix lint

0404c94

weswigham reviewed Aug 21, 2017

View reviewed changes

Test performance improvement:nested reference skip

a3676df

This was referenced Aug 21, 2017

Out of memory error when using ImmutableJS #17070

Closed

Upgrading to 2.4.1 runs out of memory. #17097

Closed

emmanueltouzery mentioned this pull request Aug 21, 2017

Monet crashes TypeScript compiler 2.4+ monet/monet.js#144

Closed

weswigham mentioned this pull request Aug 21, 2017

Cache most calls to isRelatedTo #17950

Closed

Use maybeKeys, only cache unconstrained type parameters

8fd12db

sandersn mentioned this pull request Aug 23, 2017

Recursive-related-check generic type references based on the id of their targets and type arguments #17984

Merged

sandersn closed this Aug 23, 2017

microsoft locked and limited conversation to collaborators Jun 14, 2018

RyanCavanaugh deleted the skip-nested-references-in-assignability branch June 16, 2022 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip nested references in relation checking #17947

Skip nested references in relation checking #17947

sandersn commented Aug 21, 2017 •

edited

Loading

weswigham Aug 21, 2017

sandersn Aug 21, 2017

sandersn commented Aug 21, 2017

sandersn commented Aug 21, 2017

ahejlsberg commented Aug 22, 2017 •

edited

Loading

sandersn commented Aug 22, 2017

sandersn commented Aug 23, 2017

sandersn commented Aug 23, 2017

Skip nested references in relation checking #17947

Skip nested references in relation checking #17947

Conversation

sandersn commented Aug 21, 2017 • edited Loading

weswigham Aug 21, 2017

Choose a reason for hiding this comment

sandersn Aug 21, 2017

Choose a reason for hiding this comment

sandersn commented Aug 21, 2017

sandersn commented Aug 21, 2017

ahejlsberg commented Aug 22, 2017 • edited Loading

sandersn commented Aug 22, 2017

sandersn commented Aug 23, 2017

sandersn commented Aug 23, 2017

sandersn commented Aug 21, 2017 •

edited

Loading

ahejlsberg commented Aug 22, 2017 •

edited

Loading