Skip to content
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

Closed
2 tasks done
alexd16 opened this issue Jul 3, 2020 · 33 comments
Closed
2 tasks done

Bad material-ui Box performance #21657

alexd16 opened this issue Jul 3, 2020 · 33 comments
Assignees
Labels
component: Box The React component. package: system Specific to @mui/system performance priority: important This change can make a difference v4.x

Comments

@alexd16
Copy link

alexd16 commented Jul 3, 2020

Seeing significant performance degradation when using Box components to style list item when iterating over arrays

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

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:

  1. Click 'Show Div List'
  2. Data will show without any delay
  3. Click 'clear'
  4. Click 'Show Box List'
  5. After a small delay the data shows

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 🌎

Tech Version
Material-UI v4.8.3
React v16.13.1
@alexd16 alexd16 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 3, 2020
@oliviertassinari oliviertassinari added duplicate This issue or pull request already exists package: system Specific to @mui/system performance and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer duplicate This issue or pull request already exists labels Jul 3, 2020
@eps1lon
Copy link
Member

eps1lon commented Jul 3, 2020

Thanks for the report. A more useful comparison would be an existing Material-UI component e.g. Typography. Comparing against a simple <div> will obviously show worse performance.

@alexd16
Copy link
Author

alexd16 commented Jul 3, 2020

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.
I was expecting some overhead from using the Box component, just not that much.

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:
Using Typgraphy: https://drive.google.com/file/d/14szDrfvs1VBHqnAXMb22gZjL9cxj7z4a/view?usp=sharing
Using Box: https://drive.google.com/file/d/1EOwf-mhBLDumlLpTJ9rAQyNTW0d1QB3P/view?usp=sharing

Those profiles represent clicking for the respective button 3 consecutive times. Let me know if you prefer a screenshot instead of the json files.

@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Jul 3, 2020
@eps1lon
Copy link
Member

eps1lon commented Jul 3, 2020

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.

@alexd16
Copy link
Author

alexd16 commented Jul 3, 2020

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.

@oliviertassinari
Copy link
Member

This thread makes me think of this tweet: https://twitter.com/jaredpalmer/status/1271482711132254210.

@ypresto
Copy link
Contributor

ypresto commented Jul 4, 2020

Duplicate of #16704 ?

@alexd16
Copy link
Author

alexd16 commented Jul 12, 2020

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:

import {
  spacing,
  shadows,
  borders,
  palette,
  sizing,
  display,
  flexbox,
  typography,
  positions
} from "@material-ui/system";
import styled from "styled-components";

export const Box = styled.div`${palette}${spacing}${typography}${shadows}${borders}${sizing}${display}${flexbox}${positions}`;

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

@ypresto
Copy link
Contributor

ypresto commented Jul 12, 2020

@alexd16

You can use material-ui theme in styled-components by passing mui's theme object to ThemeProvider of styled-components.
https://material-ui.com/guides/interoperability/#theme

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}`

@alexd16
Copy link
Author

alexd16 commented Jul 12, 2020

@ypresto I think I didn't explain very well want I meant.

with mui Box I can do this:

<Box bgcolor="background.primary" fontsize="h3.fontSize">Text</Box>

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:
https://codesandbox.io/s/hungry-elion-ogpzl?file=/src/Demo.js

@gandhirahul
Copy link

Heya! Is there any update on the same?

btw @eps1lon, The issue still exists even if we provide the props.

@eps1lon
Copy link
Member

eps1lon commented Jul 20, 2020

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?

@oliviertassinari
Copy link
Member

@ypresto
Copy link
Contributor

ypresto commented Jul 29, 2020

@alexd16

The missing piece is ThemeProvider of styled-components.

image

Please read this link:
https://material-ui.com/guides/interoperability/#theme

@alexd16
Copy link
Author

alexd16 commented Jul 29, 2020

@ypresto Oh, that works! Completely missed the ThemeProvider from styled-components, should have read the docs more closely.

Thank you!

@darkowic
Copy link
Contributor

@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.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 16, 2020

@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.

@kof
Copy link
Contributor

kof commented Aug 24, 2020

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
Copy link
Member

@kof I think that we would get better performance if we were using react-jss rather than a custom integration. @mnajdova maybe we should have included react-jss in the performance benchmark?

@kof
Copy link
Contributor

kof commented Aug 24, 2020

@oliviertassinari react-jss doesn't have yet a styled interface (officially), maybe it's time to make an official one and try it on this bench.

We can for sure add a hooks based interface, but I can see that makeStyles (hooks based) in mui has great performance already:

Styled Components
Duration: 28ms
MUI makeStyles
Duration: 12.18ms

I wonder though if the difference between MUI makeStyles and MUI styled (5x in perf) has to do with function values or just the component related overhead?

@mnajdova
Copy link
Member

I just run again locally the benchmarks (mnajdova/react-native-web#1) react-jss is around 2x slower than emotion, which is above the threshold. Supporting styled functionality would be great, for potentally easier replacement (or adding adapters in the future) @kof the bad performance of Mui's makeStyles comes into play when there are dynamic styles, for example using props inside it. We tested both that scenario, as well as the Box scenario which basically uses dynamic styles calculations internally. It doesn't have much to do with the overhead of the components themselves. Does that makes sense?

@kof
Copy link
Contributor

kof commented Aug 24, 2020

@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})

@mnajdova
Copy link
Member

@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.

image

@kof
Copy link
Contributor

kof commented Aug 24, 2020

@mnajdova did you remove the function alltogether? How do styles look rn?

@mnajdova
Copy link
Member

Here it is:

const useStyles = createUseStyles({
  root: (props) => ({
    ...(props.outer && {
      alignSelf: 'flex-start',
      padding: 4
    }),
    ...(props.layout === 'row' && {
      flexDirection: 'row'
    }),
    ...(props.color === 0 && {
      backgroundColor: '#14171A'
    }),
    ...(props.color === 1 && {
      backgroundColor: '#AAB8C2'
    }),
    ...(props.color === 2 && {
      backgroundColor: '#E6ECF0'
    }),
    ...(props.color === 3 && {
      backgroundColor: '#FFAD1F'
    }),
    ...(props.color === 4 && {
      backgroundColor: '#F45D22'
    }),
    ...(props.color === 5 && {
      backgroundColor: '#E0245E'
    }),
    ...(props.fixed && {
      width: 6,
      height: 6
    }),
  })
});

@kof
Copy link
Contributor

kof commented Aug 24, 2020

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.

@kof
Copy link
Contributor

kof commented Aug 24, 2020

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
  }
});

@mnajdova
Copy link
Member

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 :)

@kof
Copy link
Contributor

kof commented Aug 24, 2020

I see, yeah direct perf comparison is impossible in this case unless you use the same type of api with emotion.

@mnajdova
Copy link
Member

I tried also styled from emotion, perf numbers are more or less the same

@mnajdova
Copy link
Member

mnajdova commented Oct 12, 2020

@alexd16 seems like after we migrated the Box to use our @material-ui/styled-engine (emotion by default, but can be configured to styled-components) we got out of the box around 3x perf improvement according to our benchmarks:

Previously:

Box @material-ui/styles:

484.97ms
503.47ms
487.66ms
488.58ms
480.11ms
466.57ms
464.07ms
495.40ms
497.15ms
483.59ms
-------------
Mean: 485.16ms
Median: 486.31ms

After the changes:

Box @material-ui/styled-engine (emotion):

167.18ms
149.94ms
151.12ms
170.33ms
168.17ms
157.04ms
156.22ms
168.81ms
170.78ms
155.42ms
-------------
Mean: 161.50ms
Median: 162.11ms

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

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 12, 2020

Should we consider this resolved?

@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

Screenshot 2023-02-03 at 23 43 28

v5.0.0-alpha.20 https://codesandbox.io/s/benchmark-v5-alpha-45ilh

Screenshot 2023-02-03 at 23 50 53

v5.11.0 https://codesandbox.io/s/benchmark-v5-y2jlm1

Screenshot 2023-02-03 at 23 45 08


Now, I think that we should still be able to be x3, x10 faster, which is what #34826 is about.

@alexd16
Copy link
Author

alexd16 commented Oct 12, 2020

@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?
I am wondering if the migration from v4 to v5 will be too painful

Edit: and thank you all involved for looking into this issue. really appreciate it

@alexd16 alexd16 closed this as completed Oct 12, 2020
@oliviertassinari
Copy link
Member

@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

@mnajdova mnajdova mentioned this issue Oct 16, 2020
2 tasks
@oliviertassinari oliviertassinari added the component: Box The React component. label Dec 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Box The React component. package: system Specific to @mui/system performance priority: important This change can make a difference v4.x
Projects
None yet
Development

No branches or pull requests

8 participants