Defer indexed accesses on generic potential discriminants #49274
Conversation
This file contains 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 Milestone Bug
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at b6ee4f8. You can monitor the build here. |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at b6ee4f8. You can monitor the build here. |
|
Heya @weswigham, I've started to run the diff-based user code test suite on this PR at b6ee4f8. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the extended test suite on this PR at b6ee4f8. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at b6ee4f8. You can monitor the build here. Update: The results are in! |
|
@weswigham |
|
@weswigham Here they are:Comparison Report - main..49274
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 1af50d7. You can monitor the build here. Update: The results are in! |
sandersn
requested changes
Jun 15, 2022
| @@ -11934,6 +11934,10 @@ namespace ts { | |||
| }); | |||
| } | |||
|
|
|||
| /** | |||
| * Caution: Do you _really_ mean to use this fucntion? It creates fresh property symbols in an uncached manner - if you, the caller, | |||
| * do not cache the result of this function, you may have very poor performance over large unions. | |||
There was a problem hiding this comment.
is there a cached version, or at least a common way to cache its result? If so it would be good to mention that too.
There was a problem hiding this comment.
Not quite? If you have a union, the property cache on the union serves a similar purpose. I added this comment because I'd used it in the first iteration, and it caused horrendous perf regressions because of the lack of caching it caused.
| @@ -12315,6 +12319,10 @@ namespace ts { | |||
| return getReducedType(getApparentType(getReducedType(type))); | |||
| } | |||
|
|
|||
| function isDiscriminableLiteralType(type: Type): boolean { | |||
| return isLiteralType(type) || isPatternLiteralType(type) || type === uniqueLiteralType || (!!(type.flags & TypeFlags.Intersection) && some((type as IntersectionType).types, isDiscriminableLiteralType)); | |||
There was a problem hiding this comment.
I would break the line here. It's too long to fit on any of my screens as-is.
Suggested change
| return isLiteralType(type) || isPatternLiteralType(type) || type === uniqueLiteralType || (!!(type.flags & TypeFlags.Intersection) && some((type as IntersectionType).types, isDiscriminableLiteralType)); | |
| return isLiteralType(type) || isPatternLiteralType(type) || type === uniqueLiteralType | |
| || (!!(type.flags & TypeFlags.Intersection) && some((type as IntersectionType).types, isDiscriminableLiteralType)); |
| @@ -26,19 +26,19 @@ type T01 = keyof Object; | |||
| >T01 : keyof Object | |||
|
|
|||
| type T02 = keyof keyof Object; | |||
| >T02 : number | "length" | "toString" | "charAt" | "charCodeAt" | "concat" | "indexOf" | "lastIndexOf" | "localeCompare" | "match" | "replace" | "search" | "slice" | "split" | "substring" | "toLowerCase" | "toLocaleLowerCase" | "toUpperCase" | "toLocaleUpperCase" | "trim" | "substr" | "valueOf" | |||
There was a problem hiding this comment.
wish we could come up with a fast way to sort union output =(
| ((prop as TransientSymbol).checkFlags & CheckFlags.Discriminant) === CheckFlags.Discriminant && | ||
| !isGenericType(getTypeOfSymbol(prop)); | ||
| ((prop as TransientSymbol).checkFlags & CheckFlags.Discriminant) === CheckFlags.Discriminant ? | ||
| !isGenericType(getTypeOfSymbol(prop)) ? DiscriminantKind.Concrete : DiscriminantKind.Potential : DiscriminantKind.None; |
There was a problem hiding this comment.
Suggested change
| !isGenericType(getTypeOfSymbol(prop)) ? DiscriminantKind.Concrete : DiscriminantKind.Potential : DiscriminantKind.None; | |
| isGenericType(getTypeOfSymbol(prop)) ? DiscriminantKind.Potential : DiscriminantKind.Concrete : DiscriminantKind.None; |
Comment on lines
+15691
to
+15692
| reduceLeft((type as UnionOrIntersectionType).types, (flags, t) => flags | getGenericObjectFlags(t), 0) | | ||
| (type.flags & TypeFlags.Union && isUnionWithGenericDiscriminantProperty(type as UnionType) ? ObjectFlags.IsGenericObjectType : 0); |
There was a problem hiding this comment.
I think it would be easier to read if you flipped the line order. I got confused about whether the new line was inside the reduceLeft call or not.
Suggested change
| reduceLeft((type as UnionOrIntersectionType).types, (flags, t) => flags | getGenericObjectFlags(t), 0) | | |
| (type.flags & TypeFlags.Union && isUnionWithGenericDiscriminantProperty(type as UnionType) ? ObjectFlags.IsGenericObjectType : 0); | |
| (type.flags & TypeFlags.Union && isUnionWithGenericDiscriminantProperty(type as UnionType) ? ObjectFlags.IsGenericObjectType : 0) | | |
| reduceLeft((type as UnionOrIntersectionType).types, (flags, t) => flags | getGenericObjectFlags(t), 0); |
| function isUnionWithGenericDiscriminantProperty(type: UnionType) { | ||
| getPropertiesOfType(type); // initialize property cache (includes partial props) | ||
| const props = arrayFrom((type.propertyCache || emptySymbols).values()); | ||
| for (const prop of props) { |
There was a problem hiding this comment.
Is the for loop better than
return props.find(prop => isDiscriminantProperty(type, prop.escapeName) === DiscriminantKind.Potential)?
There was a problem hiding this comment.
Ehh... It probably can be if I swap the loop to using the iterator directly rather than collecting it, like it was thinking.
There was a problem hiding this comment.
I thought we were switching to target ES6 sometime soon? At least, I'm pretty sure that would let us use for-of to iterate maps finally.
|
@weswigham Here they are:Comparison Report - main..49274
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Haven't looked at the code yet, but boy that's a pretty hefty perf hit in |
|
Awk, well, I just fixed half of it, hopefully the other half is mostly fixable, too, and just indicative of a lack of caching somewhere. |
| @@ -11934,6 +11934,10 @@ namespace ts { | |||
| }); | |||
| } | |||
|
|
|||
| /** | |||
| * Caution: Do you _really_ mean to use this fucntion? It creates fresh property symbols in an uncached manner - if you, the caller, | |||
There was a problem hiding this comment.
Suggested change
| * Caution: Do you _really_ mean to use this fucntion? It creates fresh property symbols in an uncached manner - if you, the caller, | |
| * Caution: Do you _really_ mean to use this function? It creates fresh property symbols in an uncached manner - if you, the caller, | |
|
@weswigham Do you want to come back to fix the slower performance, or should we close this PR? |
typescript-bot
added
For Backlog Bug
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.
The issue was that
{id: ID} & ({id: "yes", ...} | {id: "no", ...})didn't register as an object type indexed accesses needed to be deferred on. Since instantiatingIDcould discriminate the type upon instantiation, and thus change what types are indexed into, the indexing operation must be deferred, as with other more traditional generic object types.Fixes #45560