-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Optimize eagerly calculating CouldContainTypeVariables
during object type instantiation
#54538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize eagerly calculating CouldContainTypeVariables
during object type instantiation
#54538
Conversation
within `getObjectTypeInstantiation` more. The existing check pulls the flags forward onto primitives/unions/intersections which may result from `instantiateMappedType` unconditionally. This means we may mark unions and intersections as possibly containing type parameters when they in fact cannot.
@typescript-bot perf test this |
Heya @weswigham, I've started to run the perf test suite on this PR at 7698260. You can monitor the build here. Update: The results are in! |
@weswigham Here they are:
CompilerComparison Report - main..54538
System
Hosts
Scenarios
TSServerComparison Report - main..54538
System
Hosts
Scenarios
StartupComparison Report - main..54538
System
Hosts
Scenarios
Developer Information: |
Yep no change to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
Should we backport this fix instead of #54507 to 5.1? Or does that feel too unsafe for a bugfix? (Mainly, are there actually performance regressions in 5.1 due to the previous optimization sometimes being too conservative?)
@typescript-bot test this Juuuust in case |
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 7698260. 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 7698260. You can monitor the build here. |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 7698260. You can monitor the build here. Update: The results are in! |
Probably, but the effect is small, as you can see. |
Heya @weswigham, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
Hey @weswigham, it looks like the DT test run failed. Please check the log for more details. |
@weswigham @jakebailey I wanted to mention here that it could be beneficial to backport this to 5.1, see for more details the following issue #54549 (comment) I'm trying to connect the dots, I hope this helps |
Basically, this conditionalizes the carrying-forward of type variable containment checks within
getObjectTypeInstantiation
more. The existing check pulls the flags forward onto primitives/unions/intersections which may result frominstantiateMappedType
unconditionally. This means we may mark unions and intersections as possibly containing type parameters when they in fact cannot.Noticed this while rereviewing #53246 while reviewing #54507