-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
Bad material-ui Box performance #21657
Comments
Thanks for the report. A more useful comparison would be an existing Material-UI component e.g. |
I guess that's fair, I'll make a comparison using Typography to see If I find the same issues. I was using div directly just because that's exactly what the output of using the Box component would be. Using a library like tailwindcss or tachyons I believe the performance would be similar to the one found in the div example. I understand these are completely different libraries on how they work, but in terms of the usage of the Box component, I see them more or less the same way. I have updated the original codesandbox sample to show how the typography behaves. As far as I can tell, it also shows a noticeable performance degradation, but not as much as the Box version. Performance profile from the performance tab: Those profiles represent clicking for the respective button 3 consecutive times. Let me know if you prefer a screenshot instead of the json files. |
My point is that the benchmark isn't all that useful since you aren't using any features of the Box. I agree that this shouldn't create that much overhead but you're benchmarking the least used case. That isn't a fair comparison. It's entirely possible that the Box is just as slow with actual props. But it would help us identifying the bottleneck in a real-world scenario. Then we can compare it to components that implement a similar interface. From a quick glance I can already see that there's a big difference in style calculation. Something that shouldn't be there if these would actually produce the same output. Currently you're comparing a 2020 smartphone to a 2000 mobile phone with regards to their battery lifetime. It's frustrating to read issues about "bad performance" if these attack the weakest argument for using a component. |
Ok, that's fair. I am sorry, I am not too used to looking into these issues. I thought it would be more helpful to give the simplest example that showed the issue. In any case this is similar to what we are doing in the application currently: https://codesandbox.io/s/wonderful-http-x5l05?file=/src/Demo.js Hope this is a better example to demonstrate the issue. As far as I can tell I am not doing anything special there, but there is a very noticeable delay when switching pages. |
This thread makes me think of this tweet: https://twitter.com/jaredpalmer/status/1271482711132254210. |
Duplicate of #16704 ? |
I have been away over the last week so I didn't have the chance to look into this any further. I did take a look at the issue referenced by @ypresto and it seems using styled-components could improve the performance I tried to replace the Box implementation from @material-ui/core with this:
And in fact, just by replacing in a couple of components, the difference in performance was very noticeable. The big downside that I can see from doing this is that I can no longer reference the theme keys directly. I have to access the theme object with useTheme. Is there any way to get a round that issue? Other than this is there other concerns that I might need to be aware using this version of the Box component? I read something on the docs about preferring to use jss to minimize bundle size. Should I expect a big impact on bundle size? Thank you |
You can use material-ui theme in styled-components by passing mui's theme object to ThemeProvider of styled-components. To define theme type of styled-components in TypeScript use this: import { Theme } from '@material-ui/core'
declare module 'styled-components' {
export interface DefaultTheme extends Theme {}
} Note: In TypeScript styled-components-based Box components will look like this: import {
borders,
display,
flexbox,
grid,
palette,
positions,
shadows,
sizing,
spacing,
typography,
PropsFor,
ComposedStyleFunction,
} from '@material-ui/system'
import styled from 'styled-components'
// Borrowed from Box.d.ts
type BoxStyleFunction = ComposedStyleFunction<
[
typeof borders,
typeof display,
typeof flexbox,
typeof grid,
typeof palette,
typeof positions,
typeof shadows,
typeof sizing,
typeof spacing,
typeof typography
]
>
type SystemProps = PropsFor<BoxStyleFunction>
export const Box = styled.div<
SystemProps
>`${borders}${display}${flexbox}${grid}${palette}${positions}${shadows}${sizing}${spacing}${typography}` |
@ypresto I think I didn't explain very well want I meant. with mui Box I can do this:
where background.primary is resolved from the palette key on the theme object and fontSize from the typography key. As far as I can tell I cannot do that with a style-components version of a Box, or am I missing something? The code you pasted seems the same I had but with typescript typings. Here is an example of what I mean: |
Heya! Is there any update on the same? btw @eps1lon, The issue still exists even if we provide the props. |
Do you have a benchmark that compares it to a component with a similar feature-set? |
We have the beginning of a benchmark in https://github.com/mui-org/material-ui/tree/next/packages/material-ui-benchmark#material-uistyles |
The missing piece is ThemeProvider of styled-components. Please read this link: |
@ypresto Oh, that works! Completely missed the ThemeProvider from styled-components, should have read the docs more closely. Thank you! |
@Nvveen has posted Box comparison with other styling solutions in another issue: #6115 (comment) - the demo is here https://codesandbox.io/s/css-in-js-comparison-forked-mg3gx?file=/src/App.js Now I'm confused. Should we even use the Box component? I started my app POC using it, but now I have a lot of doubts about it. |
@darkowic it depends on "when" you need to ship your app to production and how many Box you want to use in your application. The main blocker behind #15561 is the performance of dynamic styles, a problem we plan to solve by moving to a different styling solution @mnajdova is leading this effort for v5. The performance of the Box component can be a problem with long list, but even makeStyles can be an issue compared to using primitives with a nested CSS selector. |
I came here to see the perf problem, but the difference in numbers between themed and non-themed solution seems too big (I get themed 1488.35ms vs unthemed 71.82ms) which means something is off with the theming implementation in MUI specifically. |
@oliviertassinari react-jss doesn't have yet a We can for sure add a hooks based interface, but I can see that
I wonder though if the difference between MUI |
I just run again locally the benchmarks (mnajdova/react-native-web#1) |
@mnajdova yeah, I wonder how it looks for you if you try passing the theme differently, instead of const useStyles = createUseStyles(theme => ({
root: (props) => ({ try const useStyles = createUseStyles({
root: (({theme)) => ({ then inside the component: const theme = useTheme()
const classes = useStyles({...props, theme}) |
@kof I've just tried it locally. Seems like the theme was not even used so I removed that argument. Still the perf is around 2x slower than emotion. |
@mnajdova did you remove the function alltogether? How do styles look rn? |
Here it is:
|
Do I understand correctly that you are comparing to this implementation from emotion? If yes, this is comparable to the version that doesn't use function rules/values, it is using a set of predefined rules. |
I would be also curious to see the difference on performance if usage of spread operators is removed, because it results in a number of function calls because of babel: const useStyles = createUseStyles({
root: (props) => {
let style = {}
if (props.outer) {
style.alignSelf = 'flex-start'
style.padding = 4
}
if (props.layout === 'row') {
style.flexDirection = 'row'
}
style.backgroundColor = ({
0: '#14171A',
1: '#AAB8C2',
2: '#FFAD1F',
3: '#FFAD1F',
4: '#F45D22',
5: '#E0245E',
})[props.color]
if (props.fixed) {
style.width = 6
style.height = 6
}
return style
}
}); |
Yep, I am running the tests on that branch, you may fetch it locally if you are more interested in the tests. I run again it's similar results. BTW I've updated your comment there was typo in the code :) |
I see, yeah direct perf comparison is impossible in this case unless you use the same type of api with emotion. |
I tried also |
@alexd16 seems like after we migrated the Box to use our Previously:
After the changes:
I've also updated your codesandbox with the latest version that contains these changes, and it looks much better: https://codesandbox.io/s/restless-smoke-wmnbv?file=/src/BoxListItem.js Should we consider this resolved? Should we test upon some more advanced scenarios? For our benchmarks, you can take a look on the scenarios here - https://github.com/mui-org/material-ui/tree/2df44b790c739b85eb7baa0a342e071da9c0053d/benchmark/scenarios |
@mnajdova I think that we can close. This sounds a lot faster, it's not every day we can get an improvement in performance as significant. See the following benchmark. Updated from Feb 2023. On an Apple M1 Pro chip, we go from 48ms to 15ms, an x3.2 speed-up. v4.12.4 https://codesandbox.io/s/benchmark-v4-dhvjg v5.0.0-alpha.20 https://codesandbox.io/s/benchmark-v5-alpha-45ilh v5.11.0 https://codesandbox.io/s/benchmark-v5-y2jlm1 Now, I think that we should still be able to be x3, x10 faster, which is what #34826 is about. |
@mnajdova, @oliviertassinari Yup, I think this can be closed. Do you have an estimation when v5 final will land? Just to make sure I understood, to take advantage of the performance improves it requires an update to the next major version correct? Edit: and thank you all involved for looking into this issue. really appreciate it |
@alexd16 We are trying to make the migration as smooth as possible. There are deprecations warnings coming in v4 anytime possible, there is a detailed upgrade guide. There is a theme adapter in v5. There are modules we keep exported in v5.0.0 |
Seeing significant performance degradation when using Box components to style list item when iterating over arrays
Current Behavior 😯
When rendering components that use a lot of Box components, I see a visible slowdown in the UI and stops interaction across other elements in the application, which doesn't happen with native components.
Expected Behavior 🤔
I should not see that much of a performance degradation for using the Box component
Steps to Reproduce 🕹
Here is a codesandbox sample that shows the issue: https://codesandbox.io/s/hidden-smoke-hdhe6
Steps:
Looking at the performance tab on chrome, this is what I see:
Box List
Div List
To be honest I am not too familiar with the performance tab, so I am not sure if what I measured is relevant. But if I am looking at this correctly it seems to be more than 10x performance degradation.
Context 🔦
I have been having some performance issues in the project i've been working on, yesterday I tried to investigate why those issue where happening, and after digging a bit, it seems the reason why I am seeing bad performance across the application is because of our usage of the Box component
Given I didn't saw any warning in the docs I freely used the Box component across the application without expecting performance issues. A lot of styling in the application is done this way.
If this is something that can't be solved, and it's just the overhead that the Box component introduces, I think there should be a notice on the Box component component page so that people avoid this type of usage.
Your Environment 🌎
The text was updated successfully, but these errors were encountered: