Generalize the fastpath for comparisons of unions which are correspondences #41903
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
…dences to unions resulting from the application of intersections
weswigham
force-pushed
the
generalize-fastpath
branch
from
08325dd to
bcc6aa8
Compare
|
@typescript-bot pack this |
|
@typescript-bot perf test this - curious to see if this affects |
|
Heya @weswigham, I've started to run the perf test suite on this PR at bcc6aa8. You can monitor the build here. Update: The results are in! |
|
Hey @amcasey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@weswigham Here they are:Comparison Report - master..41903
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I'm having trouble reasoning about the case where the target type is bigger. Is that interesting too? |
amcasey
approved these changes
Dec 10, 2020
| for (let i = 0; i < sourceTypes.length; i++) { | ||
| const sourceType = sourceTypes[i]; | ||
| if (target.flags & TypeFlags.Union && (target as UnionType).types.length === sourceTypes.length) { | ||
| if (undefinedStrippedTarget.flags & TypeFlags.Union && sourceTypes.length >= (undefinedStrippedTarget as UnionType).types.length && sourceTypes.length % (undefinedStrippedTarget as UnionType).types.length === 0) { |
There was a problem hiding this comment.
We seem to access (undefinedStrippedTarget as UnionType).types.length quite a few times. Would it be worthwhile to extract it out of the loop, since it won't change?
There was a problem hiding this comment.
Ehh, it's a little awkward because we can only get it after the Union flag check, which is done internally to the loop.
| const related = isRelatedTo(sourceType, (target as UnionType).types[i], /*reportErrors*/ false, /*headMessage*/ undefined, intersectionState); | ||
| // such unions will have identical lengths, and their corresponding elements will match up. Another common scenario is where a large | ||
| // union has a union of objects intersected with it. In such cases, if the input was, eg `("a" | "b" | "c") & (string | boolean | {} | {whatever})`, | ||
| // the result will have the structure `"a" | "b" | "c" | "a" & {} | "b" & {} | "c" & {} | "a" & {whatever} | "b" & {whatever} | "c" & {whatever}` |
There was a problem hiding this comment.
Is this order guaranteed somehow? If not, would we have someway of knowing that it changed, breaking this optimization?
There was a problem hiding this comment.
Kind-of. It's not guaranteed, just very often true. Often enough to be a useful heuristic. Unions are sorted by type IDs, type IDs are assigned on type construction, type are constructed in a top-down/left-to-right/as-they-appear kinda way when normalizing or mapping (mostly as an artifact of our mostly lazy compiler design). A notable thing that can easily break this ordering heuristic is code which pulls one of the type out of the union and makes it in advance, for example:
type A = {a};
type B = {b};
type FooB = "foo" & B;
type Union = ("foo" | "bar") & (A | B);The presence of the FooB type changes the order in the Union type. Not much to be done when that happens, except hope it doesn't shift around too much. (Since partial correspondences still benefit from this check, just not as much) That's one reason why we still fall back to the exhaustive check if the corresponding element fails to match.
There was a problem hiding this comment.
If not, would we have someway of knowing that it changed, breaking this optimization?
That test that I commented on in the other PR would time out, for one. :V
| @@ -17258,14 +17258,29 @@ namespace ts { | |||
| return Ternary.False; | |||
| } | |||
|
|
|||
| function getUndefinedStrippedTargetIfNeeded(source: Type, target: Type) { | |||
| // As a builtin type, `undefined` is a very low type ID - making it almsot always first, making this a very fast check to see | |||
There was a problem hiding this comment.
What types have lower type IDs? This seems potentially fragile.
There was a problem hiding this comment.
Only the any-likes are manufactured before undefined (yes, this is arbitrary and fragile, but it turns out, also useful), and they can't exist in a union: so undefined will actually always be the first element, if present.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
to include unions resulting from the application of intersections on unions.
This expands the fastpath we added in #37749 to handle structures like those produced in #41517 in linear time, bringing the typecheck time of https://github.com/NoPhaseNoKill/ts-slow-compilation-example/tree/main/src down on my machine to ~3s, down from ~30s.