-
Notifications
You must be signed in to change notification settings - Fork 13k
Move anonymous type instantiation cache from AST node to root type #41084
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
Conversation
@typescript-bot perf test this |
Heya @andrewbranch, I've started to run the perf test suite on this PR at 43f2cba. You can monitor the build here. Update: The results are in! |
@andrewbranch Here they are:Comparison Report - master..41084
System
Hosts
Scenarios
|
Hmm, the performance here looks a little worse than #41003 on material-ui and monaco, but that makes me wonder if there were previously more incorrect instantiations in material-ui that just went unnoticed 🤔. The difference is almost within the margin of error, so maybe not too bad... |
It's possible the slowdown is just a result of reduced monomorphism in object layouts, but the numbers are higher enough that likely there are more type objects being allocated. Would be interesting to know where those extra allocations are coming from. I have a sense it isn't just spreads in object literals. |
I'll see if I can find out—in my first attempts I had some code that used the cache both the old way and the new way and hit a |
I think I see what's causing the slowdown. The changes aren't quite right for deferred type references, we need to obtain the "root" deferred type reference (the one produced by |
@typescript-bot perf test this |
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 3b35e23. You can monitor the build here. Update: The results are in! |
@ahejlsberg Here they are:
Comparison Report - master..41084
System
Hosts
Scenarios
|
Much better. Looks like we're now in the margin of error. |
BTW, I have manually verified that this PR now doesn't affect the number of types created when compiling |
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.
Added the test case from #41003. Thanks for the help!
Fixes #40995.