mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

Improve Material-UI performance #10778

Closed Bessonov closed 4 years ago

Bessonov commented 6 years ago

First, thank you very much for this awesome component library! It's great!

I added a drawer in my new app. Mostly, I copypasted drawer example. Just for PoC I multiplied

        <Divider />
        <List>{mailFolderListItems}</List>

section.

After that it feels very slow, especially on mobile device (nexus 4, cordova with crosswalk 20). I use dev mode, but prod mode doesn't speed up much.

Through react dev tools I noticed that components in mailFolderListItems rendered on every link click in react-router or even menu open. It takes sometime up to 50-60ms to rerender ONE {mailFolderListItems}. I use

const modalProps = {
    keepMounted: true, // Better open performance on mobile.
};

To eliminate uncertainty with other app components, I converted mailFolderListItems to Component and disable rerendering:

class MailFolderListItems extends React.Component<{}, {}> {

    shouldComponentUpdate() {
        return false;
    }

    render() {
        return (
            <List>
                <Link to={Routes.Startpage.path}>
                    <ListItem>
                        <ListItemIcon>
                            <InboxIcon />
                        </ListItemIcon>
[...]

                <Divider />
                <MailFolderListItems />

After that this part feels OK.

I found https://github.com/mui-org/material-ui/issues/5628 . I suggest to revisit it. Optimizing shouldComponentUpdate is fundamental and most important step to gain performance. PureComponent is just most common shortcut.

Furthermore, I noticed that very much time (1-2ms and more for EVERY material-ui component) is spended in WithStyles.

Expected Behavior

I'm expecting to get most of possible react performance for this great library.

Current Behavior

The app get slower with every material-ui component.

Steps to Reproduce (for bugs)

I don't provide reproducing example yet, because I just copypasted from component demo page, but if needed I can provide codesandbox demo. For browser it's noticeable, if browser slowed down by factor >=5x in performance setting.

Your Environment

Tech Version
Material-UI 1.0.0-beta.38
Material-UI Icons 1.0.0-beta.36
React 16.2.0
browser cordova crosswalk 20 (equals android chrome 50)
oliviertassinari commented 6 years ago

I'm expecting to get most of possible react performance for this great library.

@Bessonov Performance will be a focus post v1 release. We have tried to keep the core of the library as fast as possible. It should be good enough for +90% of the cases. At least, it's my experience using the library so far. You haven't provided any external reference, aside from raising our attention around the fact that performance is important, we can't make this issue actionable. If you are able to identify a performance root limitation of Material-UI that we can investigate (with a reproduction codesandbox or repository), please raise it. Until then, I'm closing the issue.

Bessonov commented 6 years ago

@oliviertassinari thank you for your quick response! I'm very happy to hear, that performance will be focus after release.

It should be good enough for +90% of the cases. At least, it's my experience using the library so far.

On desktop - yes, it is. But on mobile it is really slow. Just Drawer and some buttons makes app laggy. It's not responsive and consumes more power as needed.

You haven't provided any external reference, aside from raising our attention around the fact that performance is important, we can't make this issue actionable.

I provided a reference to already raised issue and a reference to react documentation.

If you are able to identify a performance root limitation of Material-UI that we can investigate (with a reproduction codesandbox or repository)

As I said, I can do it. Here is a comparison of pure and non pure component. Steps to reproduce are:

  1. Use chrome and go to https://codesandbox.io/s/r1ov818nwm
  2. Click "Open in a new window" in preview window
  3. Open dev tools and go to "performance" tab
  4. Throttle your CPU by at least 5x (depending on your PC - may be 10x) to reflect mobile device
  5. Click on burger and see how it lags.

And now:

  1. Go to index.js
  2. change pure to true
  3. Save
  4. Repeat steps from above
  5. Enjoy it!

This example is a litte bit unrealistic because I've inserted too much List elements. But as I said, on mobile it get very quickly to point where you "feel" every action.

Bessonov commented 6 years ago

Here is my issue with WithStyles. It's on Desktop with CPU throttling. I would like to post screenshots from real device on Monday.

Time for WithStyles(ListItem): image

Time for ListItem: image

The difference is 1.26 ms just for a ListItem component. With 13 components like this you can't render with 60fps anymore. But I noticed on Desktop, that this duration is not always here.

Bessonov commented 6 years ago

Here is a comparison of pure and non pure component

Btw I don't say that PureComponent is THE solution for all performance issues. I just say, that it can be one of the helpers to make material-ui usable on mobile.

oliviertassinari commented 6 years ago

The difference is 1.26 ms just for a ListItem component.

@Bessonov The WithStyles component should be very cheap for repetitive instance of the same component. We only inject the CSS once. Maybe you are reaching React limitations and the cost of higher order components. There is a range of solution to mitigate performance issue in React. For instance, you can use element memoization and virtualization. I'm keeping this issue open as the start point for future performance investigation. I don't think there is much we can do about it right now. Thanks for raising the concern.

Bessonov commented 6 years ago

Ok, that's only one part. What do you think about more important part - optimizing shouldComponentUpdate?

EDIT: I want to split this issue in two parts. This part is about shouldComponentUpdate and the other part for WithStyles, if I can show more information. Is that OK for you?

oliviertassinari commented 6 years ago

optimizing shouldComponentUpdate

@Bessonov We don't implement such logic on purpose on Material-UI side for two reasons:

  1. Most of our components accept react elements, a react element reference change at each render, making the logic systematically waste CPU cycles.
  2. The closer the shouldComponentUpdate logic is from the root of the React tree, the more efficient it's. I mean, the more pruning you can do on the tree, the better. Material-UI components are low level. There low leverage opportunity.

The only opportunity we have found is with the icons. Let us know if you can identify new ones.

the other part for WithStyles

+90% of the time spent in WithStyles is actually spent in JSS, there very few we can do about it on Material-UI side. At least, I have been identifying a caching opportunity for server-side rendering lately to greatly improve the performance on repetitive rendering. But it's server-side only.

Bessonov commented 6 years ago

Most of our components accept react elements, a react element reference change at each render, making the logic systematically waste CPU cycles.

That's depending on usage and can be improved. I don't benchmark myself, but I'm sure that right component usage and optimized shouldComponentUpdate (with shallow comparison) is less costly than not optimized component, especially for immutable structures (and immutable.js isn't required btw). Related ticket: https://github.com/mui-org/material-ui/issues/4305

For example in https://github.com/mui-org/material-ui/blob/v1-beta/docs/src/pages/demos/drawers/PermanentDrawer.js#L68 this leads to new objects and makes PureComponent senseless:

        classes={{
          paper: classes.drawerPaper,
        }}

because it returning every time a new object. But not:

const drawerClasses = {
    paper: classes.drawerPaper,
};
[...]
        classes={drawerClasses}

Same for components.

The closer the shouldComponentUpdate logic is from the root of the React tree, the more efficient it's. I mean, the more pruning you can do on the tree, the better.

Yes, that's true.

Material-UI components are low level. There low leverage opportunity.

That's partially true. As you can see in https://codesandbox.io/s/r1ov818nwm I just wrap List in PureComponent. Nothing else and it speed up Drawer by significant factor. I would claim that this is true for every component, at least which use {this.props.children}. I created another demo: https://codesandbox.io/s/my7rmo2m4y

There is just 101 Buttons (pure = false) and Buttons wrapped in PureComponent (pure = true, see where Button import from). Same game result, if I click the "Click"-Button:

Normal Button: image

Wrapped Button (with overhead and so on): image

As you can see, there is 637ms vs. 47ms for just normal Buttons! Do you still think that optimize shouldComponentUpdate (or just PureComponent) not worth it? :)

EDIT: fix wording at beginning

Bessonov commented 6 years ago

@oliviertassinari @oreqizer I noticed interesting thing. It seems like extends PureComponent performs better as Component with

  shouldComponentUpdate() {
    return false;
  }

image

EDIT: I think this is one of react internal optimization.

oliviertassinari commented 6 years ago

As you can see, there is 637ms vs. 47ms for just normal Buttons! Do you still think that optimize shouldComponentUpdate (or just PureComponent) not worth it? :)

@Bessonov It's demonstrating the potential of the pure logic. Yes, it can be very useful! It's a x13 improvement. But I don't think it's close to real-life conditions:

Bessonov commented 6 years ago

@oliviertassinari as a great developer, how you can write things like this? Why you use your personal assumptions as arguments and no facts? I think, I presented enough facts above. I did everything to show it, because I like this project and want to contribute. It's not enough? Ok, then more facts and no assumptions.

Just for you I reduce to 10 Buttons. 10! It's mean any 10 components (worse: containers) from material-ui slows down entire app until the app is not usable! Tested on real device with crosswalk 21/chrome 51 without any throttle:

Normal button: image

PureButton: image

This still an 8x improvement! It's huge! Can you imagine how important this on mobile device is?

Instead of 100 buttons, you will find 10 buttons at most on a page. Still, you will find 10 grids, 10 X, etc.

I used Button because it is a one of simplest component! It shows that material-ui is broken from performance point of view. Now, imagine a container components like a AppBar, Toolbar, List, Drawer! This is even worse! You get very quickly to 20 components/containers on every page. And because you don't expire any slowness on your powerful desktop/mac it's not mean that material-ui isn't incredible slow.

react-intl, the check between the previous and new props will always be false. You will waste CPU cycles. So x13 -> x0.8

Show me an example on codesandbox. I don't see why this should be happen. I'm sure, this happens only on wrong usage of react components. And official example shows wrong usage, because it seem like react-intl not applied context subscribers. But there is a lot of other components which are aligned with react guidelines and best practices to stay performant.

BTW: WithStyles consume up to 2,27ms for the Button on mobile device. 8 components and you under 60fps.

oliviertassinari commented 6 years ago

Why you use your personal assumptions as arguments and no facts?

What makes you think it's a personal assumption? I was trying to perform critical thinking. Conceptually speaking, the extra prop traversal will slow down the pure version compared to the non pure version, it has to prune something to worth it. Same reasoning as #5628 or https://github.com/react-bootstrap/react-bootstrap/issues/633#issuecomment-234749417 or https://github.com/reactstrap/reactstrap/pull/771#issuecomment-375765577

With pure: capture d ecran 2018-03-27 a 11 43 41

Without pure: capture d ecran 2018-03-27 a 11 44 15

The reproduction is the following.

Bessonov commented 6 years ago

@oliviertassinari are you sure that codesandbox makes everything right for your test? Because my results are very different:

Without pure (even without throttle it's slow): image

Pure (after change to true and save I get new url for codesandbox): image

Bessonov commented 6 years ago

Since it doesn't check context changes and also forces users to ensure all of the children components are also "pure", I don't believe this to be a good fit for this library. This library needs to be as flexible as possible and I believe this will make it more difficult to use.

I see the point. But it's a very interesting tradeoff. On one side even material-ui should be "as flexible as possible", but on the other side current performance makes it unusable at all.

May be you should think about providing pure and non-pure version of components. I do it now for my app to get some usable perfomance even on desktop.

oliviertassinari commented 6 years ago

@Bessonov The codesandbox wasn't saved correctly. Sorry. It was missing the following diff. I have updated the link:

<Button>
+ <i>
    Button
+ </i>
</Button>
Bessonov commented 6 years ago

I don't understand why it should produce different results? I get better chart, but non-pure version is significantly slower.

EDIT: ok, I see. Try to figure out what going on...

Bessonov commented 6 years ago

OK, I understand now. Just the same "new object on every render"-thing. I didn't noticed it before. For some cases it can be improved with constants trough babel plugin automatically.

oliviertassinari commented 6 years ago

@Bessonov https://medium.com/doctolib-engineering/improve-react-performance-with-babel-16f1becfaa25

Bessonov commented 6 years ago

Well, then you know it already! :D Even there is not much benefit on it's own (you showed ~7%), it's still profitable in conclusion with pure components to avoid some flaws you mention above. I tested it now with Pure Wrapper + babel plugin + production build and it's impressive fast on the same mobile device!

As I said, I think it's best to offer both - non pure components for flexibility and pure components (wrappers are enough to keep it simple and maintable) for performance. But for me, I can live with just pure components, because overall performance improvement is much greater that performance drawbacks. Or better: I can't use material-ui without pure components.

Ok, for now I'm waiting for further input on this topic and create own wrappers in my app )

Thanks for insights!

dantman commented 6 years ago

I've never heard of transform-react-constant-elements actually being used and being really useful. It's ok as as random micro-optimization to throw in, but in practice you rarely write code simple enough to get much from it. Although, I suppose it wouldn't be a bad optimization for all the SVG-style icon components like <Add />.

Take a look at this example (click on Plugins in the side, search for "react-constant" and click the checkbox on "transform-react-constant-elements") and you'll see that barely anything has been optimized:

I personally like PureComponent and try to use it absolutely everywhere and optimize things as much as I can so that it works. But it doesn't look like MUI is at all made in a way that PureComponent would work.

If we want to optimize anything, these fundamental issues need to be dealt with, otherwise simply letting people enable a pure mode MUI won't actually optimize much at all. I can think of two possibilities.

We just allow pure mode to be enabled, consumers have to memoize or hoist objects manually to actually get optimizations

  1. One way I can think of to do this would be some short of shallowMemoize which using the local this and a key (so you can use it on different bits of data) that memoizes the data as long as it's shallow equal
  2. Another I can think of is using something like reselect to create selectors that will memoize data.
  3. Another way of doing the shallowMemoize in 1. would be to pass it to render() with a decorator. This way we could pass in a new one each render and instead of needing key we can just check if any of the memoized objects from the last render should be re-used, then discard all the old values.

The problem of course is this makes consumers a lot larger and messier and requires logic to be manually hoisted up to places where it's far away from the code using it.

import {createSelector} from 'reselect';

class FormPage extends PureComponent {
  state = { example: '' };

  change = e => this.setState({example: e.target.value});
  submit = () => {
    console.log('Submit: ', this.state.example);
  };

  runButtonClasses = createSelector(
    props => props.classes.runButtonLabel,
    runButtonLabel => ({runButtonLabel}));

  render() {
    const {title} = this.props;
    const {example} = this.state;

    return (
      <form>
        {title}
        <TextField
          InputProps={this.shallowMemoize('a', {
            // This example assumes use of transform-react-constant-elements to make this object always the same
            startAdornment: <InputAdornment position="start">Kg</InputAdornment>,
          }}}
          onChange={example}
          value={example} />
        <Button classes={this.runButtonClasses(classes)}>Run</Button>
        <Button onClick={this.submit}>Submit</Button>
      </form>
    );
  }
}
// ...
  @withShallowMemoize
  render(memo) {
    const {title} = this.props;
    const {example} = this.state;

    return (
      <form>
        {title}
        <TextField
          InputProps={memo({
            startAdornment: <InputAdornment position="start">Kg</InputAdornment>,
          }}}
          onChange={example}
          value={example} />
        <Button classes={memo(classes)}>Run</Button>
        <Button onClick={this.submit}>Submit</Button>
      </form>
    );
  }

Instead of trying to opimize InputProps, classes, etc... we encourage people to make micro-components for all of their use casses

If this is the recommended way to use MUI, we may not even need a pure mode. As you can see, as soon as you start making small helper components for your common use cases those components themselves can easily be pure components. In the example WeightTextField now never get's re-rendered as long as value is still the same, completely skipping withStyles, any memoization work necessary for InputProps, or the InputAdornment setup. And when value does change, we have to re-render TextField anyways so the non-pure InputProps={{...}} doesn't matter.

I'm fine with this path. I like micro-components in theory; though I hate every currently valid syntax/pattern for writing them that I can think of. I don't want to MyComponent = enhance(MyComponent), I want to decorate them, but you can't decorate any of the short ways of writing a tiny component. I also dislike turning import {TextField} from 'material-ui'; into import WeightTextField from '../../../ui/WeightTextField;`.

let WeightTextField = ({unit, InputProps, ...props}) => (
  <TextField
    {...props}
    InputProps={{
      startAdornment: <InputAdornment position="start">{unit}</InputAdornment>,
      ...InputProps
    }}
    onChange={example}
    value={example} />
);
WeightTextField = pure(WeightTextField);

RunButton = compose(
  withStyles(theme => ({
    label: {
      fontWeight: '800',
    },
  })),
  pure
)(Button);

const SubmitButton = pure(Button);

class FormPage extends Component {
    state = { example: '' };

  change = e => this.setState({example: e.target.value});
  submit = () => {
      console.log('Submit: ', this.state.example);
    };

  render() {
    const {title} = this.props;
    const {example} = this.state;

    return (
      <form>
        {title}
        <WeightTextField
          unit='Kg'
          onChange={example}
          value={example} />
        <RunButton>Run</RunButton>
        <SubmitButton onClick={this.submit}>Submit</SubmitButton>
      </form>
    );
  }
}
wilsonjackson commented 6 years ago

I've got a use case where I need display 500–2000 checkboxes on a page in a big list. Using native browser checkboxes, performance is fine, but using the <Checkbox> component, performance is very poor and scales linearly with the number of checkboxes on the page. Example: https://codesandbox.io/s/5x596w5lwn

I'm using mui@next — is there some strategy I can employ now to make this workable?

dantman commented 6 years ago

@wilsonjackson First, don't do the following. This will create a new handler every checkbox every render, which will deoptimize any PureComponent optimizations you try to do.

  handleChange = index => event => {
    this.setState({

Secondly, make your own small component to wrap Checkbox and make that component pure. This has the extra benefit that you can add in any properties that are common to all your checkboxes. And, since you have the common issue of needing a different change event handler for each item we can use a class component and do that in the component instead of in the list container.

https://codesandbox.io/s/r7l64j6v5n

oliviertassinari commented 6 years ago

If we want to optimize anything, these fundamental issues need to be dealt with, otherwise simply letting people enable a pure mode MUI won't actually optimize much at all. I can think of two possibilities.

@dantman These API choices have been made in order to improve the DX as much as possible while trying to be fast enough.

Instead of trying to opimize InputProps, classes, etc... we encourage people to make micro-components for all of their use casses

Yes, we do. The wrapping pattern is definitely the encouraged way to customize the library. It can be extended to apply performance optimizations. It's easier on userland where they variability of the usage of the components is much lower. We could even add an FAQ question or guide section about this point in the documentation.

dantman commented 6 years ago

Yes, we do. The wrapping pattern is definitely the encouraged way to customize the library. It can be extended to apply performance optimizations. It's easier on userland where they variability of the usage of the components is much lower. We could even add an FAQ question or guide section about this point in the documentation.

Ok. In that case:

wilsonjackson commented 6 years ago

@dantman fantastic, thank you.

Pajn commented 6 years ago

I have multiple times needed to apply sCU due to withStyles (or rather JSS) being very slow. I don't know the code of JSS but it feels like it could be optimized quite a lot. I usually use styled-components or glamorous and thus end up with JSS and one of the other in the app and both of them outperform JSS.

While these cases might be a bit annoying they are easy to work around with app level sCU or smarter state updates. I have yet to see a single MUI component be slow enough to be problematic and I have also yet to code actually inside MUI to take significant time.

Not to say that it couldn't be faster and sure it would be nicer if less oversight was needed but at least to what I see it would be better to spend time optimizing JSS directly than MUI in that case.

oliviertassinari commented 6 years ago

@Pajn Thanks for the feedback. It would be really great to see some situation where withStyles performance is problematic or where it's outperformed by styled-components.

nimaa77 commented 6 years ago

did any one checked this repo https://github.com/reactopt/reactopt ?

$ click - button (text: مقالات ) => CssBaseline,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,ScrollbarSize,TransitionGroup,TouchRipple,Ripple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,Main,ScrollbarSize,TransitionGroup,TouchRipple,Ripple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple,TransitionGroup,TouchRipple

these components do unnecessary re-rendering just for a simple click, why just don't give a try to Should Component Update life-cycle ?

Bessonov commented 6 years ago

@nimaa77

did any one checked this repo https://github.com/reactopt/reactopt ? No, I use plain why-did-you-update and chrome/react dev tools capabilities.

these components do unnecessary re-rendering just for a simple click, why just don't give a try to Should Component Update life-cycle ?

See discussion above. This doesn't fit for every use case. For me, it seems like material-ui should offer both - pure and non pure versions of components. I see huge performance improvement with pure components.

@dantman

Take a look at this example (click on Plugins in the side, search for "react-constant" and click the checkbox on "transform-react-constant-elements") and you'll see that barely anything has been optimized:

This is just one way to address this issue. There are also other options, plugins and other hand crafted optimizations. Don't get me wrong, but I'm not interested in theoretical discussions which optimizations are good or bad. I'm interested in hands on optimization which are WORKS. At least in my setup. Everything I wrote above is WORKING for me and make my app at least usable on low-mid mobile devices. Although I'm forced to do more optimizations like reordering component trees, achieved performance wouldn't be possible without pure components and other automatic optimizations. And yes, I profile and optimize very much to get this done.

hexetia commented 6 years ago

@Bessonov maybe we can use a prop to make the shouldComponentUpdate method to do shallow compare (https://reactjs.org/docs/shallow-compare.html) OR always return false, this will not increase the bundle size with two version of components (pure and normal)

Bessonov commented 6 years ago

@lucasljj I don't expect significant increase of bundle size if this done as a wrapper for stateful component like mention above. See also profiles above: just pure components are faster than return false; in sCU.

The problem with pure components or components implementing sCU is, if you use non pure component inside it or children. See statements above. Another issue to address is theme switching. Not tested, but I think this can be overcome at least with Context API.

mschipperheyn commented 6 years ago

@bossonov Pure Components are considered the should-have-been-default of react that are not because they came late to the party. But I agree that most people will create redux form style wrappers mitigating the lack of it on the subtree

the problem you reference regarding pure components and children, only occurs if the top level props don't propagate to the children. Every prop or state change will trigger a rerender through the child tree up to the level that the prop doesn't propagate. Because the idea w pure components is that it rerenders on every prop or state change. I don't know the internals intimately but I was wondering In you couldn't use a per component change property that you pass through to every child. You might even use the new context api for this purpose so as not to have to pass down a prop through the entire tree.

Bessonov commented 6 years ago

@oliviertassinari

Performance will be a focus post v1 release.

Great, v1 is released :) Is there any idea when performance should be addressed? I've noticed that this issue isn't part of post v1 milestone.

oliviertassinari commented 6 years ago

@Bessonov I think that it would be great to spend some time to update the ROADMAP. Regarding the performance. I have two things in mind actionable, but I hope to discover more:

  1. We can't improve something we can't see. We need a benchmark. I have seen two interesting libraries so far:
  2. Rendering the CSS server side requires ~30% of what React needs to render. Because we don't support style = f(props) (well, we don't support it yet: #7633). We can implement a very efficient caching capability, cutting the cost near to 0% for repeated requests. I might work on that for the office if the SSR performance harms our business metrics. But it's not the only options, we can also think of Babel plugins to apply the costly JSS presets at the compilation time instead of at the runtime (to balance with bundle size implications) cc @kof.
Bessonov commented 6 years ago

Thanks for the urls.

Fully agree with 1 and linked #4305 above.

  1. SSR can help with first page load, but doesn't help with slow rerendering or cordova. While personally I can ignore first time loading on desktop or even mobile, slow rerendering still affects mobile devices after first loading.

Compile time optimizations would be great and from my perspective should be preferred, because it can improve first time loading and rerender.

Another thing is sort of documentation or example project how to use mui (and babel etc.) to get better performance.

I don't know if jss-cache can be used.

kof commented 6 years ago

ISTF + preprocessing pipeline will be able to shave some ms.

On Sat, May 19, 2018, 19:06 Anton Bessonov notifications@github.com wrote:

Thanks for the urls.

Fully agree with 1 and linked #4305 https://github.com/mui-org/material-ui/issues/4305 above.

  1. SSR can help with first page load, but doesn't help with slow rerendering or cordova. While personally I can ignore first time loading on desktop or even mobile, slow rerendering still affects mobile devices after first loading.

Compile time optimizations would be great and from my perspective should be preferred, because it can improve first time loading and rerender.

Another thing is sort of documentation or example project how to use mui (and babel etc.) to get better performance.

I don't know if jss-cache http://cssinjs.org/jss-cache?v=v3.0.0 can be used.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mui-org/material-ui/issues/10778#issuecomment-390418709, or mute the thread https://github.com/notifications/unsubscribe-auth/AADOWGrCxNGqrT4MijiX8r9Ad32z6RsJks5t0FEtgaJpZM4S4woq .

Bessonov commented 5 years ago

May be helpful to setup benchmarks: https://github.com/A-gambit/CSS-IN-JS-Benchmarks/blob/master/RESULT.md

janhoeck commented 5 years ago

Oh react-jss (Which is used by material-ui) seems to be pretty slow

kof commented 5 years ago

@janhoeck it isn't, and you won't be able to prove otherwise, I am sure.

Bessonov commented 5 years ago

@kof I'm pretty understand that micro-benchmarks doesn't say much, but 4x-6x in comparison to aphrodite is really slow. In fact, overall performance of material-ui isn't shiny.

oliviertassinari commented 5 years ago

4x-6x in comparison to aphrodite is really slow

@Bessonov The benchmark methodology is key. For instance, you could try it in your browser with: https://twitter.com/necolas/status/954024318308007937?lang=fr

The results in my browser: ⚠️ with the incertitude

capture d ecran 2018-06-12 a 17 58 54

overall performance of material-ui isn't shiny

We might very well be bounded by React itself and the cost of components.

kof commented 5 years ago

@Bessonov whatever you used to check the performance, when it comes to aphrodite its not true, because it uses asap and delays the rendering, so most benchmarks measure cpu time but not final rendering performance. That being said the bench @oliviertassinari posted is most realistic.

kof commented 5 years ago

Regarding MUI performance, you can get faster when you don't use any abstractions at all, but thats not the point. MUI next is pretty fast. It is so fast that you should never worry about its performance unless YOU did something entirely wrong or misused the library.

Bessonov commented 5 years ago

@kof no, MUI isn't fast by default. We are forced to implement workarounds to get acceptable performance. Just for sure: did't you see that it's about mobile devices and not for high end macs?

never worry about its performance unless YOU did something entirely wrong or misused the library

Well, above are some codesanbox examples. If we just use MUI components in a container and save input values in container state, like it showed in component demos, then it get unusable very fast. Wrap in PureComponent, use uncontrolled components, tree reordering, constant elements, memoize and so on helping us to maintain some acceptable performance. You are welcome to show how to use MUI right, to get better results, for example in (atm we don't use drawer anymore): https://codesandbox.io/s/r1ov818nwm

@oliviertassinari thanks for the link. Here are results for Chrome 66 on nexus 4. As you can see the results are 10x poorer. I think in crosswalk 21/chrome 51 it can be a little bit slower:

screenshot_2018-06-12-18-20-19

We might very well be bounded by React itself and the cost of components.

IMHO not necessarily, as mentioned by others too. Every avoided rerender is a big win for performance and battery. Don't forget, performance is the third point on your roadmap :+1:

kof commented 5 years ago

See it that way: if what you do with cssinjs is too slow on a mobile device, you shouldn't be using for this react and most other stuff either. If cssinjs slows you down, react component slows you down way more. You need to rethink the level ob abstraction or apply optimizations.

kof commented 5 years ago

@oliviertassinari is that evtl. possible that mui wrapper rerenders jss styles on update? There might be some leak potentially that does unneeded work.

Bessonov commented 5 years ago

@kof

See it that way [...]

I see your point. But react and react (pure)components aren't bottleneck on it's own and performs very well. For example, MUI doesn't use PureComponent (with pros and cons described above), but they are our life safer. @oliviertassinari mentioned that there is a chance to gain better performance by using caches or pre-compiler.

Don't get me wrong, you are awesome guys and I'm really happy about every MUI release :clap: But you need consider performance too, because not every user visit websites trough high performance desktop.

kof commented 5 years ago

if react is not performance bottleneck and you are sure about it, JSS won't be either. The only thing I could imagine which needs to be validated if there is some unnecessary work going on and css gets regenerated on update.

Pure component is something YOU should be using in your application level code, if you need an optimization. MUI doesn't have to do it, it is a generic library and shouldn't make any assumptions. PureComponent is not always good.

JSS has considered performance from the day one and I spent endless hours in micro optimizations.

danielo515 commented 5 years ago

This is ridiculous. I can't stop material ui components be re-rendered on every single interaction. It does not matter what I do, eve something as simple as this:

class RowText extends Component {
  shouldComponentUpdate = (nextProps, nextState) => {
    return false;
  };

  render() {
    const { title, artist } = this.props;
    return <ListItemText primary={title} secondary={artist} />;
  }
}

Triggers re renders of the underlying Typography elements. How is this even possible ?

danielo515 commented 5 years ago

Hey, this is only happens with list and list item. Why ? Is there something that I can do ?