-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Performance: variance check balloons number of instantations and degrades performance #45249
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
Comments
Tagging @amcasey as per request in a comment in a similar thread about editor performance issues: #34801 (comment) |
I stumbled upon https://www.npmjs.com/package/@amcasey/ts-analyze-trace which seems super useful. It should really be mentioned in the wiki page here: https://github.com/microsoft/TypeScript/wiki/Performance-Tracing Here's the output on the repro above:
Seems that the hot spot is: {"id":103,"symbolName":"Model","recursionId":77,"instantiatedType":91,"typeArguments":[41,93,94],"firstDeclaration":{"path":"/users/andreialecu/work/contrib/tsc-perf/types/mongoose.d.ts","start":{"line":757,"character":15},"end":{"line":757,"character":33}},"flags":["524288"]},
{"id":104,"symbolName":"Model","recursionId":77,"instantiatedType":91,"typeArguments":[42,93,94],"firstDeclaration":{"path":"/users/andreialecu/work/contrib/tsc-perf/types/mongoose.d.ts","start":{"line":757,"character":15},"end":{"line":757,"character":33}},"flags":["524288"]}, From what I can see the only difference is in the {"id":41,"flags":["262144"]},
{"id":42,"flags":["262144"]},
{"id":43,"flags":["262144"]}, |
An even simpler way to trigger this seems to be (grab import { Model } from "mongoose";
class Foo<T extends Model<any>> {} // add/remove the `extends` check and run `yarn tsc --extendedDiagnostics`
export const UserSchema = new Foo<Model<{}>>(); // with: class Foo<T> {}
Types: 92
Instantiations: 2
// with: class Foo<T extends Model<any>> {}
Types: 42259
Instantiations: 291012 |
extends
check balloons number of instantations and degrades performance
As we discussed on discord, I think the problem is that structural assignability check is expensive, and the variance check is what triggers the structural assignability. It might be possible to skip variance checking in your specific case if the types were treated more nominally. I think the interesting question is whether production code would be able to avoid a variance check. |
Thank you, @sandersn, for a very insightful discord discussion! 🙂 For reference, here's how the types would be used, idiomatically, using mongoose, as per the documentation: https://mongoosejs.com/docs/typescript.html It's possible I'm overlooking something but I'm not sure if variance checking is necessary in this case, since the types are nominal here and a base interface for the "schema" would always be defined and used. |
Hope you don't mind the ping @amcasey (and/or @weswigham) I noticed this was tagged as planned for TD 4.5 but I was wondering if there's anything you could point us to in the mean time. (This has been an ongoing problem in mongoose and it has been making life painful for everyone since the types got moved off Definitely Typed.) Improving typescript performance has been a priority for mongoose maintainers but seems that the root cause still can't be found. Any pointers would be hugely appreciated. Thank you! |
It seems like the upcoming variance annotations in the 4.7 would allow mitigating this problem, right? |
One would think. I did some quick experimentation and saw some very slight improvement. When I come back to it I’ll do it a bit more scientifically with some tracing. Though if someone on the mongoose side beats me to it, even better. |
@andreialecu do you get similarly bad performance with mongoose v6? |
@andrewbranch, yes, it's still noticeably slow, but I learned to deal with it. I'm on a Macbook Pro M1 Max FWIW, which should be a powerhouse. Here's my VSCode experience with mongoose 6.3.9 - notice how it takes a couple of seconds for the errors to update: Screen.Recording.2022-06-23.at.10.27.38.movThis capture above was pretty fast, but it can take up to 10 seconds to update at times. |
When compiling this file: https://github.com/gchq/Bailo/blob/main/server/models/Deployment.ts It seems that a fairly simple model with ~10 attributes still seems to take nearly 2 seconds to type check. |
I’m not sure there’s anything currently actionable for us here. Automattic/mongoose#10349 has gotten some renewed attention and mongoose maintainers have a PR up with some improvements. Further up in that thread, I described getting some decent improvements by adding variance annotations to a few of the variance hot spots. However, it’s up to the mongoose folks whether they want to split their types with |
Bug Report
The Mongoose project recently introduced their own typescript types, therefore deprecating the DefinitelyTyped types.
However, the new types suffer from some big performance issues, greatly slowing down the VS Code developer experience. I've been investigating the problem and noticed that the issue seems to occur from some
extends
checks.Related issues in mongoose repo:
Automattic/mongoose#10349
Automattic/mongoose#10487
Automattic/mongoose#10492
🔎 Search Terms
performance extends instantiations recursiveTypeRelatedTo
🕗 Version & Regression Information
This occurs on TS 4.3.5.
⏯ Playground Link
Unfortunately no playground link, but I made a very simple repro at: https://github.com/andreialecu/typescript-performance-extends
Take a look at the README there.
💻 Code
This diff in
mongoose
's types seems to completely eradicate the problem:The difference between before and after are:
Before:
After:
I realize the code after the diff isn't exactly equivalent, but the problem seems to be related to the
extends
check. Avoiding extends gets rid of the issue.Here's a cpu profile:
cpuprofile.zip
The 'before' version shows:
🙁 Actual behavior
Slow type checking performance, especially in VS code.
🙂 Expected behavior
Better performance.
I opened this in the hope that there is value in investigating the repro by the TS team, in order to check whether any performance improvements in cases such as this one are possible.
The text was updated successfully, but these errors were encountered: