-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Cache discriminated contextual types #58372
Conversation
@typescript-bot test it |
@typescript-bot test it Sorry, the new function app we had to deploy has some scaling issues being worked out. |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests comparing Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Wow, that's actually significant., 3% on our self test (expected), but also 1.5% in mui-docs. |
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.
Honestly surprised we call this with the same arguments so many times - but I guess at least twice with the same args is basically guaranteed, since signature inference is 2-pass.
@typescript-bot test it |
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hmm, removing |
@jakebailey Here are the results of running the top 400 repos comparing Everything looks good! |
I bisected a perf regression typechecking https://github.com/arktypeio/arktype and found the following: {
"checkTime": 9.68,
"types": 483415,
"instantiations": 2629152
}
devDependencies:
- typescript 5.5.0-dev.20240429
+ typescript 5.5.0-dev.20240430
{
"checkTime": 12.2,
"types": 514846,
"instantiations": 2819487
} It seems likely this change was somehow involved? |
Guided by This PR isn't part of that. |
My mistake, I likely jumped to conclusions when I saw this in the release notes as a likely candidate since I use quite a lot of unions internally. @Andarist and I have been looking at which of the changes from that day could have caused such a large increase in type instantiations and check time and haven't been able to identify it yet. |
While researching another issue, @jakebailey noticed this statement takes an exceedingly long time to type check. With the caching added in this PR, the check time for the statement drops from 250ms to about zero on my machine.