-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Can the typings be simplified to improve performance? #19113
Comments
I'm not familiar with material-ui types but try to answer these questions.
It might be worth looking at the implementation of types in version 3.9.3, because in this version, type checking was fast enough and typing was good. |
First of I'd like to thank you for reaching out and looking into this. It's incredibly helpful to have someone benchmark which parts of the types are slow or not.
It's impossible to be unbiased here. I don't think we can give up on it though looking at the wider ecosystem. Chrome devtools have this feature, react itself types the
I would need to check the CSS-in-JS solution we are using. Technically media queries in CSS can be recursive. I'd be willing to cut this recursiveness and see if we get reports. Technically nested media queries can be flattened with the const styles = {
root: {
'@media (max-width: 12cm)': {
':hover': {}
}
}
} Is this something you see yourself writing @oliviertassinari?
If the argument itself is not a property bag but a function then it requires a theme. The styles can depend on two different kind of property bags: The theme (available via React's context API) or the props (directly passed to the component): makeStyles({ root: { color: 'blue' }}); // A
makeStyles(theme => ({ root: { color: theme.color } })); // B
makeStyles({ root: props => ({ color: props.color})}); // C
makeStyles({ root: { color: props => props.color } }); // D: same as C, only exists for dev ergonomics
makeStyles(theme => ({ root: props => ({ color: props.color || theme.color }) })); // E: what you called "double-lazy" It's less about lazy evaluation to improve perf but more about having to wait for context and props being available.
There is one case where this is used: Consider const useStaticStyles = makeStyles({ root: { color: 'blue' } });
const useDynamicStyles= makeStyles({ root: { color: props => props.color } })
function Component() {
const staticClasses = useStaticStyles(); // No error
const throwingClasses = useDynamicStyles(); // $ExpectError
const dynamicClasses = useDynamicStyles({ color: 'blue' });
} To infer the call signature of the function returned from In short: We avoid having to write The problem with this "feature" is that advanced users don't have a problem using It seems like most of the time the type checker is occupied with the styling solution? I would've expected the call signature for our components i.e. what props are possible to take most of the time. I can crawl more repositories to look specifically at usage of |
@eps1lon We have a couple of occurrences of this nesting in the codebase (e.g. with |
A known-good baseline would be very helpful. Do you happen to have a link? |
@eps1lon Happy to be part of the conversation. Fast, correct types are good for everyone. 😄
Regarding What are your thoughts on 2(b)? It seems like that type isn't providing much value since it basically says that any property name will be accepted and the return type can be one of a large number of things. |
I'd like to experiment with overloading the call signature first and see if this has any impact. Then we'll try making it less sound by making it always optional. It seems safer than usual to make it less sound since almost all use cases only call it once so you'll likely only make the mistake once and it surfaces pretty quickly. It's going to be tough to sell it though if it does not gain us much performance. I'll use the repository created for the original issue (microsoft/TypeScript#34801 (comment)).
Skipped that accidentally. I'll look at it after the IsEmptyInterface experiment. |
@eps1lon Totally agree - keep |
With #19320 we got rid of some complex conditional types where function overload did achieve the same thing (removing I want to add that I'm currently switching back and forth between TS 3.2.4 and 3.7.4. Our type test suite runs 50% slower in 3.7.4 compared to 3.2.4 (~90s vs 50s). I'll continue investigating if we can limit the depth of CSSProperties and removing support for media queries and pseudo selectors entirely. Having those not typed is not really an option though. The type checker should be able to check those in a reasonable time. It might be that the type checker is the actual bottleneck. Maybe we can investigate in which version perf got hit. |
If you send me the commands you're running in 3.2.4 and 3.7.4, I can profile locally. However, experience suggests that the cause will probably turn out to be additional, desirable checking added since 3.2.4. (And I assume "0s" is a typo - probably "40s" or "50s"?) Regarding CSSProperties, I agree that keeping the property names and types is extremely valuable. However, I think there's more to it than checking them in a reasonable amount of time - the first problem is that the type system can't actually express the shape you want. I think we're probably going to be spending some time working on a general solution to the problem - can I assume you'd be interested in participating in that discussion? We have scripts for comparing multiple versions of the compiler so, if you can identify a fairly stable test you'd like us to run, we can figure chart the slowdown over time. |
Sorry, it is 50s.
It's just
I suspected as much. It looks like the way we mix a "concrete" shape with an object with an index signature is merely a workaround.
Definitely. I'll spend a little bit more time with non-recursive CSSProperties and then write a bit more tests to illustrate what we're looking for in those types. I would suspect that other css-in-js styling solutions hit similar performance bottlenecks. |
I'll try out those commands tomorrow (I suppose I should mention I'm on pacific time and observe US holidays).
Thanks, I've been struggling with how best to express that. Yes, you're correct that the index signature is not doing what you want. We have some thoughts on a variant that might, but we need to explore the performance implications.
Very much so. We're hoping that anything we do for you can be generalized to improve the whole ecosystem. |
I feel like I'm missing something obvious, but I'm currently stuck. First, I gave up on Windows - things seem to work better on Linux. Let me know if you'd like to dig into that. Second, I can get |
@amcasey I think the best "real-world" test case is using --- a/docs/tsconfig.json
+++ b/docs/tsconfig.json
@@ -3,6 +3,8 @@
"include": ["types", "src/pages/**/*"],
"compilerOptions": {
"allowJs": false,
- "noUnusedLocals": true
+ "noUnusedLocals": true,
+ "noEmit": true,
+ "skipLibCheck": true
}
} |
@eps1lon Thanks for clarifying. I'm not thrilled that tslint got slower, but I'd like to focus on one variable at a time. I'll run the docs build you suggested with various versions of typescript and see if anything jumps out. Thanks! |
This setup is perfect. I'm seeing the check time double between 3.3 and 3.4.
|
I'll dig a bit more, but I'm told that the 3.3 implementation of conditional types was incomplete, the 3.4 implementation was slow, and the 3.5 implementation is good. So, unfortunately, this is probably expected. Specifically, I suspect this change introduced the slowdown as described in this bug. |
I find it concerning that between 3.5 and 3.7 there was a 6-second increase in the time it takes to run the check. That looks pretty substantial. |
@embeddedt Those numbers are from single runs with each version, so there's probably quite a bit of noise. However, I'll dig in and see if I can find anything. |
I redid it on a Linux VM and 3.7 was consistently 20-25% slower than 3.5. |
This has proven to be quite difficult to bisect because consecutive runs of the same build vary by ~5% and the difference between 3.5 and 3.6 or between 3.6 and 3.7 is only ~10%. One suspicious thing I have noticed is that |
Much to my surprise, the new |
I couldn't think of a clever solution, so I'm going to plot compilation time merge by merge and look for spikes. I hope to have numbers tomorrow. |
@amcasey Thanks for your efforts in looking into this! Really neat to see members of the TS team and Material UI working together. I stumbled onto this Github issue trying to figure out why our editor experience with Material UI is so slow (we're using it in two projects at work). Can definitely confirm that we're seeing a pretty significant impact in intellisense and usability inside VsCode. At this point we'd be happy to trade a little bit of type safety in our JSS for snappy feedback for the rest of the library. Sometimes it takes 8-10 seconds of waiting before the Typescript server catches up to code changes |
Even with three-run averages, the data is very noisy. However, there appears to be a noticeable drop in runtime at microsoft/TypeScript@ad322a5 a noticeable increase (back to roughly the previous level) at microsoft/TypeScript@480b739 and a general upward trend after that (though it looks too uniform to me and I suspect environmental factors) including a particular spike at microsoft/TypeScript@c5e6d95. I'm going to focus on the first two until I run some more tests to confirm the general upward trend (how could it get uniformly worse every commit?). |
Is this why vscode is unbearably slow with when on projects with MUI? Intellisense takes seconds to pop up, and type errors sometimes take 10+ seconds to go away. It's pretty ridiculous. |
@douglasg14b Yes, TypeScript seems to be the main issue, sadly. If you don't care about typings, the hack I found was to just remove the typing files for MUI entirely from |
We plan to spend some time on TypeScript performance issues in the upcoming months. I've started this PR - #32571 for learning how we can more systematically test the performance improvements we make. If you can share some examples with specific code (or repository) that you see problematic I can add it as a test case for us. I don't think there is a silver bullet to solving this issue, so every example that you share will be valuable. This will help us prioritize bottlenecks that we can tackle first. From the discussions and the simple example app I've created, looks like some of the bottlenecks could be:
but I would like to verify this by having a good real-life app example where this is visible so that we can test our potential perf improvements over the same code. |
Since you're asking for examples: This learning project seems to have terrible TS performance once in and using it for a bit: https://github.com/douglasg14b/partytyme-karaoke-songs |
The biggest bottleneck in my experience is I've been using a module override for styled and it's basically fixed everything for me. Obviously, it's not perfectly accurate but it's made the DX way more bearable. // ## Curried function to put styles in
type NestedSelector = React.CSSProperties | { [key: string]: NestedSelector | undefined }
type ExtendReactComponentProps<Component extends (...args: any) => JSX.Element, Additional> = (props:Parameters<Component>[0] & Additional) => ReturnType<Component>
interface FastCreateStyled<C> {
(selectors:NestedSelector):C;
<AdditionalProps extends {} = {}>(cb:(props:{theme:Theme} & AdditionalProps)=>NestedSelector): ExtendReactComponentProps<C, AdditionalProps>
}
// ## Function that creates the styling curry
interface Options {
shouldForwardProp: (propKey: string) => boolean
}
interface MyStyled {
<C extends keyof JSX.IntrinsicElements>(tag:C, options: Options = {}) : FastCreateStyled<(props:JSX.IntrinsicElements[C])=>JSX.Element>
<C extends React.JSXElementConstructor<React.ComponentProps<C>>>(component:C, options: Options = {}) : FastCreateStyled<C>
<C extends React.ComponentClass<React.ComponentProps<C>>>(component:C, options: Options = {}) : FastCreateStyled<C>
}
// ## Can't export interfaces so assign it
let MyStyledFunction : MyStyled
declare module "@mui/system/styled" {
export = MyStyledFunction
} |
Thanks for starting to share some of the benchmarks. After initial view:
@douglasg14b from this project I could confirm that the
I could see this too as a problem in the simple app I've created for benchmarking. Ok, let's start with these two then. My first proposal would be trying to not use recursion for the CSS nested objects, but limit them to 2 or 3 levels, as it was initially proposed in #19113 (comment) .I will report back the results of this. |
Can someone help me test a potential perf improvement on the
These two screenshots should indicate some differences in the time required for checking the types (maybe something similar to #32735 (comment) (note that this is a very simple project)). In addition, would be great if you could share whether the overall experience for the autocompletion is better. Lastly, please share some type checking problems that you may encounter, just to track what type changes this may have introduced, as there is a change on the linked PR related to the refs. Thanks in advance! |
v5.6.0 Application of ~1000loc with the DataGrid and some small forms. 3700ms to 3080ms ~ ⬇️16.7% |
Here's a workaround if you don't care about rm -rf node_modules/@mui/system/styleFunctionSx; find node_modules/@mui/styled-engine -name '*.d.ts' -delete; sed -i 's/ComponentPropsWithRef/ComponentProps/' node_modules/@mui/material/OverridableComponent.d.ts You'll also need to enable For convenience, put the above script as the |
Thank you very much. It's of some use. 👍 |
As #32735 was merged, for everyone that would like to try an updated relaxed typings for import type {} from '@mui/material/OverridableComponentAugmentation';
// and if you use @mui/base
import type {} from '@mui/types/OverridableComponentAugmentation'; You can see more details on the changes in the PR #32735 itself. The reason why we decided not to update the current types of the As most of the reproductions shared in the issue are no longer applicable, I am going to close it and ask everyone who has a problem to create a new issue using the labels: "typescript", "performance" and to provide a simple repository with an output you would get using Also, I would ask anyone if they are aware of better way for measuring TypeScript check performances to let us know 🙏 Thanks a lot for the input that everyone had so far. |
@mnajdova Thanks. How exactly is this used? Do we only need to import it once, e. g. in app.tsx |
Sorry for I does not do anything for a while on this.
I revisited for OverridableProps performance just a little bit and found that recent TypeScript maybe became more lazy at least for completion. For For another example, for IIRC, for older TypeScript versions, all props of all possible (input, button, div, ...) element types showed up because It maybe means TypeScript has become more efficient that it does not eagerly instantiate huge base constraint ( (I only checked for completions, so performance for compile or semantic highlight can be vary.) |
Yes, you need to import it only once, for example in your |
@ypresto thanks for the response.
I've seen this reported already in #33799 I suppose you are trying the default
This means that the perf would be even worse. |
Thank you so much for the effort on performance @mnajdova! Good performance is super critical for productive development. Regarding
I think this is an interesting metric, but what I think what people are complaining about is autocompletion time in VSCode, which uses tssserver: https://shopify.engineering/migrating-large-typescript-codebases-project-references I am not aware if the TypeSense project referenced ^ here is open-source, or if there is a comparable version. Thanks again 🙏 |
Thanks for sharing this blogpost @markhalonen I will read trough it now. From scanning it, seems like https://github.com/microsoft/TypeScript/wiki/Performance is already linked there (which is the reference I used), but if I find out more about this, will test it out and provide more information here. |
As suggested by @eps1lon in #18128, I'm creating this issue as a place to discuss the Material-UI typings and whether they can be simplified to reduce the amount of time spent checking them, especially during editing.
There's always a tension between having the most exact types (which provide the best errors and editor completions) and having the fast type checking (the far end of the spectrum being
any
).Issues like microsoft/TypeScript#34801 suggest that Material-UI might benefit from relaxing the exactness in order to gain back some perf.
From the repros I've investigated so far, a lot of the slowness seems to come from the large number of CSS property names (see https://github.com/mui-org/material-ui/blob/master/packages/material-ui-styles/src/withStyles/withStyles.d.ts). Not being an active CSS user myself, I have some naive questions:
CSSProperties
type appears to exist to support "pseudo selectors and media queries", which - according to my limited reading - seem to be named bags of additional CSS properties.a) Are these bags themselves recursive or is there only a single additional layer? That is, do you go from
width
tofoo.width
or tofoo.bar.width
, etc? If it's just one level, simplifying the types cuts my local repro from 4.6 seconds down to 3.6 seconds (i.e. big win).b) I played around with the types myself and couldn't come up with anything better than
BaseCSSProperties[keyof BaseCSSProperties]
, but - as I'm guessing you're aware - that's not a very useful type. It basically says that any CSS property can have the type of any (other) CSS property - that's only slightly better thanany
.StyleRules
, if there are no properties, you get eitherCSSProperties
or() => CSSProperties
(which I will sloppily call "thunked CSSProperties"), which makes sense - theCSSProperties
might be lazy. If there are properties, you get eitherCreateCSSProperties<Props>
, which makes sense - theProps
might be required to compute theCSSProperties
- or(props: Props) => CreateCSSProperties<Props>
, which I didn't understand because it's effectively double-lazy - you have pass in theProps
once to get theCreateCSSProperties
and then again to get individual properties. Why is it "double thunked"?Separately, I suspect, but have yet to demonstrate that
IsEmptyInterface
is too expensive for the benefit it provides. However, it's quite possible that I don't fully understand the benefits, so it would helpful to hear more.Can we work together to find the right balance between accuracy and perf? (Note: "just make the compiler faster" is obviously a viable strategy, but I'd like to get the typings to a good place before we optimize for them.) Thanks!
Pains
The text was updated successfully, but these errors were encountered: