Improve Material-UI performance #10778
Comments
@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. |
|
@oliviertassinari thank you for your quick response! I'm very happy to hear, that performance will be focus after release.
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.
I provided a reference to already raised issue and a reference to react documentation.
As I said, I can do it. Here is a comparison of pure and non pure component. Steps to reproduce are:
And now:
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. |
|
Here is my issue with Time for Time for The difference is 1.26 ms just for a |
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
added
performance
discussion
and removed
waiting for user information
labels
@Bessonov The WithStyles component should be very cheap for repetitive instance of the same component. We only inject the CSS once. |
|
Ok, that's only one part. What do you think about more important part - optimizing EDIT: I want to split this issue in two parts. This part is about |
@Bessonov We don't implement such logic on purpose on Material-UI side for two reasons:
The only opportunity we have found is with the icons. Let us know if you can identify new ones.
+90% of the time spent in WithStyles is actually spent in JSS, there very few we can do about it on Material-UI side. |
oliviertassinari
changed the title
That's depending on usage and can be improved. I don't benchmark myself, but I'm sure that right component usage and optimized 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 because it returning every time a new object. But not: Same for components.
Yes, that's true.
That's partially true. As you can see in https://codesandbox.io/s/r1ov818nwm I just wrap 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: Wrapped Button (with overhead and so on): As you can see, there is 637ms vs. 47ms for just normal Buttons! Do you still think that optimize EDIT: fix wording at beginning |
|
@oliviertassinari @oreqizer I noticed interesting thing. It seems like extends
EDIT: I think this is one of react internal optimization. |
@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:
|
|
@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: PureButton: This still an 8x improvement! It's huge! Can you imagine how important this on mobile device is?
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.
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. |
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 react-bootstrap/react-bootstrap#633 (comment) or reactstrap/reactstrap#771 (comment) With pure: Without pure: The reproduction is the following.
|
|
@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): Pure (after change to true and save I get new url for codesandbox): |
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. |
|
@Bessonov The codesandbox wasn't saved correctly. Sorry. It was missing the following diff. I have updated the link: <Button>
+ <i>
Button
+ </i>
</Button> |
|
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... |
|
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. |
|
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! |
|
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 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
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 cassesIf 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 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 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>
);
}
} |
|
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 I'm using mui@next — is there some strategy I can employ now to make this workable? |
|
@wilsonjackson 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. |
@dantman These API choices have been made in order to improve the DX as much as possible while trying to be fast enough.
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. |
|
Your Your I think MUIs implementation of List should change to not recreate the List context value each render So instead have List look something like this: That would simplify creating |
@Pajn No, look at my React profiler results. |
|
@oliviertassinari import React, { Component } from 'react';
import StylesProvider from '@material-ui/styles/StylesProvider';
import ThemeProvider from '@material-ui/styles/ThemeProvider';
import { createMuiTheme } from '@material-ui/core';
import List from '@material-ui/core/List';
import ListItem from '@material-ui/core/ListItem';
const theme = createMuiTheme({});
const MyListItem = React.memo(ListItem, (prev, next) => prev.selected === next.selected);
class App extends Component {
state = {
selected: null,
}
render() {
return (
<StylesProvider>
<ThemeProvider theme={theme}>
<List>
{[0, 1, 2, 3, 4].map(el => (
<MyListItem
button
selected={el === this.state.selected}
onClick={() => this.setState({ selected: el })}
>
{el}
</MyListItem>
))}
</List>
</ThemeProvider>
</StylesProvider>
);
}
}
export default App;React profiler results (after clicking on the 4th list item):
As you can see, it works as expected, i.e. there is no extra re-renders (except for I know you cannot tell me what is wrong with my code causing extra re-renders. But I ask you a question: What can cause a re-render in the |
|
@mkermani144 What do you think of this fix? --- a/packages/material-ui/src/List/List.js
+++ b/packages/material-ui/src/List/List.js
@@ -40,6 +40,13 @@ const List = React.forwardRef(function List(props, ref) {
...other
} = props;
+ const context = React.useMemo(
+ () => ({
+ dense,
+ }),
+ [dense],
+ );
+
return (
<Component
className={clsx(
@@ -54,7 +61,7 @@ const List = React.forwardRef(function List(props, ref) {
ref={ref}
{...other}
>
- <ListContext.Provider value={{ dense }}>
+ <ListContext.Provider value={context}>
{subheader}
{children}
</ListContext.Provider>Do you want to submit a pull request? :) We are using the same strategy with the Table component. It works great. Thank you for reporting the problem! |
|
@oliviertassinari Sure. This is exactly what @Pajn suggested earlier. I submitted a PR. |
|
#14934 is merged; the |
That'd be great 👍 |
|
Fwiw I built a chat app and the app needs to render a large list of contacts. I ran into the same issue |
|
@henrylearn2rock Have you considered using virtualization? We have added a demo for the list: https://next.material-ui.com/demos/lists/#virtualized-list. |
|
This also really tripped me up. I think most people (including me) assumed everything under a pure component was safe from a rerender, which evidently isn't the case for this library. I am going to try virtualization as you most recently suggested. Thanks! |
This is not how React.PureComponent or React.memo works. It only affects the the component itself. Children might still have to re-render if context changes. @pytyl Can you share the code where you used a PureComponent and expected it to prevent any re-render in its sub-tree? |
|
@eps1lon the following documentation makes it seem like returning false from shouldComponentUpdate automatically skips re-render in children components.
Maybe I am mistaken on this? The following is a snapshot of my profiler. Just for the sake of testing, I explicitly return false for shouldComponentUpdate in my Menu component:
This made all of my children components (Categories, Category, CategoryItems, CategoryItem) not re-render. Many MUI related things appear to be re-rendering at the bottom which seems to be causing a lot of delay. Stuff like withStyles, Typography, ButtonBase. Still a bit new to React so please excuse my ignorance. Below is my code for Menu component (where I am returning false for shouldComponentUpdate): |
|
I would need a full codesandbox to understand the issue. |
|
@eps1lon I will try to produce one for tomorrow. Thank you. |
|
@eps1lon here is the codepen: Short description System Case 1 (user clicks on a menu item)
Case 2 (user clicks add to order button in dialog)
I'm sure I'm making some novice mistake here so any guidance is greatly appreciated. |
|
As WithStyles(ButtonBase) is rerendered I assume that WithStyles uses a context that is recreated even though it doesn't need to. I managed to find this https://github.com/mui-org/material-ui/blob/048c9ced0258f38aa38d95d9f1cfa4c7b993a6a5/packages/material-ui-styles/src/StylesProvider/StylesProvider.js#L38 but can't find a place where StylesProvider is used in actual code (GitHubs search isn't very good) but it might be the reason. Does @eps1lon know if this might be the cause? If it is, a useMemo on the context object would probably fix this. Though I don't know if the localOptions are stable or if useMemo needs to be propagated even further. |
|
Maybe. But you should first investigate why your component using a StylesProvider rerenders. This is either something at the top of your tree or should be at some UI boundary. In either case those should rarely rerender. Remember that react context isn't optimized for frequent updates anyway. We shouldn't prematurely optimize these things just because some object is re-created during render. Memoization is not a silver bullet. So without a concrete example I can't do much. Yes things re-render. Sometimes more often then they need to. But a wasted re-render doesn't mean it's the cause of a performance bottleneck. |
|
@pytyl I have looked at your codesandbox, there is a problem with the rendering architecture. You rerender everything when the menu item is clicked. Your GlobalContext jumps the pure logic. @eps1lon I think that we should close this issue. It would be better to focus on specifically identified issues. |
|
TL;DR: Create context slices, memoize context values, no issue with material-ui specifically: https://codesandbox.io/s/8lx6vk2978 Did some digging and the issue is that you have this big global context that is re-created during render. You re-render your App when you click at which point the global context is re-created. Your CategoryItem is listening to it which appears 100 times in your App. Since you have 100 material-ui MenuItems you encounter the classic death by a thousand cuts. So ironically part of the solution is memoizing a context value but the important part is identifyng separate context slices. It seems like a state and dispatch context is appropriate. This is recommended when using useContext with useReducer and seems to fit here to. This can create quite a big tree and render props hell the more contexts you have. I encourage you to have a look at @oliviertassinari It's good issue to collect common pitfalls with solutions. We can decide if we want to create separate issues from it. |
|
@oliviertassinari @eps1lon thanks for revising! Performance seems great. |
|
I just had a problem with slow rendering performance. I solved it entirely by replacing all instances of the With Without |
|
@mankittens You could keep the Box component, using styled-components as the style engine. The performance would be much better. It should get better with JSS in the near future #16858. I'm closing this issue. We need a dedicated performance report for each potential area of improvement, not an umbrella thread. |
First of all Thanks for the community.
|
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
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 useTo eliminate uncertainty with other app components, I converted
mailFolderListItemsto Component and disable rerendering:After that this part feels OK.
I found #5628 . I suggest to revisit it. Optimizing
shouldComponentUpdateis fundamental and most important step to gain performance.PureComponentis 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
The text was updated successfully, but these errors were encountered: