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

[List] Rendering performance issue #3289

Closed
cgestes opened this issue Feb 10, 2016 · 46 comments
Closed

[List] Rendering performance issue #3289

cgestes opened this issue Feb 10, 2016 · 46 comments
Labels
component: list This is the name of the generic UI component, not the React module! performance

Comments

@cgestes
Copy link
Contributor

cgestes commented Feb 10, 2016

I ran the following tests in chrome:

Standard MUI:

class TestListMui extends React.Component {
  render() {
    let items = [];
    for (let i = 0; i < 10000; ++i)
      items.push(<mui.ListItem key={i} primaryText={'perf-' + i}/>);
    return <mui.List>
      {items}
    </mui.List>;
  }
}

Same as MUI but with inline style:

class TestListInline extends React.Component {
  render() {
    let items = [];
    for (let i = 0; i < 10000; ++i)
      items.push(
        <div key={i}>
          <span
            style={{
              border: '10px',
              boxSizing: 'border-box',
              fontFamily: 'Roboto, sans-serif',
              transform: 'translate3d(0px, 0px, 0px)',
              color: 'rgba(0, 0, 0, 0.870588)',
              fontSize: '16px',
              lineHeight: '16px',
              position: 'relative',
              transition: 'all 450ms cubic-bezier(0.23, 1, 0.32, 1) 0ms',
            }}
            tabIndex="0"
            type="button"
          >
            <div>
              <div style={{
                  marginLeft:0,
                  paddingLeft:16,
                  paddingRight:16,
                  paddingBottom:16,
                  paddingTop:16,
                  position:'relative',
                }}
              >
                <div>{'perf-' + i}</div>
              </div>
            </div>
          </span>
        </div>);

    return <mui.List>
      {items}
    </mui.List>;
  }
}

Same as MUI but with CSS:

.span-aa {
            border: 10px;
            box-sizing: border-box;
            font-family: Roboto, sans-serif;
            transform: translate3d(0px, 0px, 0px);
            color: rgba(0, 0, 0, 0.870588);
            font-size: 16px;
            line-height: 16px;
            position: relative;
            transition: all 450ms cubic-bezier(0.23, 1, 0.32, 1) 0ms;
}
.div-aa {
              margin-left:0px;
              padding-left:16px;
              padding-right:16px;
              padding-bottom:16px;
              padding-rop:16px;
              position: relative;
}
class TestListCss extends React.Component {
  render() {
    let items = [];
    for (let i = 0; i < 10000; ++i)
      items.push(
        <div key={i}>
          <span
            className="span-aa"
            tabIndex="0"
            type="button"
          >
            <div>
              <div className="div-aa"
              >
                <div>{'perf-' + i}</div>
              </div>
            </div>
          </span>
        </div>);

    return <mui.List>
      {items}
    </mui.List>;
  }
}

Results:
MUI: 19sec
Inline: 4sec
CSS: 3sec

@oliviertassinari
Copy link
Member

Yeap, I can confirm this. The <List /> is really slow. We even have an issue for this (#1501).

@cgestes
Copy link
Contributor Author

cgestes commented Feb 10, 2016

Thanks for the link.

I'll try to see why it is so slow.

@alitaheri
Copy link
Member

@cgestes It's not just the initial render that's slow. Consecutive renders will also be slow since we don't implement shouldComponentUpdate but we're working on that. If the initial render can be optimized for now, it would be a great help thanks 👍

@edgesoft
Copy link

@oliviertassinari @cgestes @alitaheri Any news on this. I have a list with 300 MenuItems and it is impossible to use this right now. Any workarounds?

@mbrookes
Copy link
Member

I have a list with 300 MenuItems and it is impossible to use this right now.

Then you know what to do! 😄

@nathanmarks
Copy link
Member

@newoga

After reading through issues such as this I am wondering if we should try to add some performance checks to the test suite. Not 100% sure right this second how it would be implemented. Just food for thought.

(Actual performance benchmarks -- not just checking for rerenders under conditions we expect to prevent it in shouldComponentUpdate).

@newoga
Copy link
Contributor

newoga commented Feb 13, 2016

Yeah, I think it's a great idea. Let me know if you develop any thoughts on how we can best prepare for implementing that.

@nathanmarks
Copy link
Member

@newoga My initial thought is setting up a test bed component to help identify areas we need either pure components or classes implementing shouldComponentUpdate with the ability to record results and somehow identify regressions in the future.

  • Component(s) being tested are passed in as children
  • The configuration for each test should be consistent so regressions can be found, so some sort of configuration object should be implemented
  • Props are passed down from the internal state of the test bed component
  • After mounting, the test bed component initiates react perf,
  • State is rapidly changed in a loop to trigger consecutive re-renders. (For a set amount of loops, say 100).
  • After the loop is finished, react perf is stopped and wasted time is measured
  • Wasted time is then compared to previous results

We can't depend on the CI server always spitting back the same results, so we need some sort of a control to run on every test as a baseline for the system that the benchmarks are currently running on. We can then use that control to assess the benchmark output.

@nathanmarks
Copy link
Member

@newoga Just to add to my last reply -- I think this is something that we can work into the browser test suite.

@newoga
Copy link
Contributor

newoga commented Feb 21, 2016

@nathanmarks These are good ideas.

We can't depend on the CI server always spitting back the same results, so we need some sort of a control to run on every test as a baseline for the system that the benchmarks are currently running on.

That's a good point. I suppose one option is to set up a basic dummy component that implements shouldComponetUpdate and before the other ones. I suppose for wastedTime, it might not matter if the numbers are changing with each run because ideally we can optimize to completely remove wastedTime. But for other measurements it would be beneficial to have some consistency.

Lots to think about but I think getting this component setup is a good start. Even if we can run it on CI yet, it'd be helpful to run on our development machines since the results should be consistent there.

@nathanmarks
Copy link
Member

@newoga The wasted time is good to check for to prevent PRs introducing performance regressions. (and also helps highlight a number of existing performance issues)

@nathanmarks
Copy link
Member

Obviously we will want to test the performance impact outside of wasted time too. But wasted time is the first thing to go on the chopping block ;). It's one of the biggest issues I found in my own testing.

@newoga
Copy link
Contributor

newoga commented Feb 21, 2016

But wasted time is the first thing to go on the chopping block ;).

@nathanmarks Definitely, I agree! I think starting with the test component to assist with identifying wasted time across the codebase is a great start.

@nathanmarks
Copy link
Member

@newoga Check this PoC out:

Text field spec to reproduce the underline and label wasting time:
https://github.com/nathanmarks/material-ui/blob/textfield-performance/test/browser/TextField.spec.js

Snags on the first assertion, so spits this out:

FAILED TESTS:
  TextField
    ✖ underline and label should not waste rendering time
      PhantomJS 1.9.8 (Mac OS X 0.0.0)
    AssertionError: should not waste time: expected 'TextField > TextFieldUnderline wasted 35ms across 99 instances' to equal false

There are 100 loops by default, so the 99 instances represents the wasted # of renders in this case.

I set this up to help address #2860

@newoga
Copy link
Contributor

newoga commented Feb 21, 2016

@nathanmarks Awesome, that was quick!

I haven't tried it yet but I look at the PoC on Github and it looks like it makes sense. That's a really cool test. I could see this eventually becoming a shouldNotWasteTime helper maybe.

@newoga
Copy link
Contributor

newoga commented Feb 21, 2016

@nathanmarks I'm possibly seeing opportunities to expand on this but maybe we should just leave this as is and start improving performance where we can. For example, you could probably create a PR with these fixtures and test and wrap the internal TextField components with recompose's pure HOC and see if it passes.

@nathanmarks
Copy link
Member

@newoga Yup, great idea! I had a similar thought but didn't want over-abstract until we understood more of the performance test use cases better.

One thing that makes the helper tricker is keeping the config for it concise but allowing it to performance test components with various types of prop types and combinations. The boilerplate-ish code in the TextField.spec.js test is partly to pass through the testFn and testProp, which are fairly simple for this component but would change greatly for something like <Table /> integration tests. I'm not sure what the best implementation of an abstraction is at this point due to that -- it'll come naturally once we start benchmarking different components though.

The PerfTest component will also need some expansion for more complex use cases.

Later today/tonight I'll do up a PR to fix the textfield issues presented by this example, they're annoying me :)

@newoga
Copy link
Contributor

newoga commented Feb 21, 2016

I'm with you 💯 percent.

Sounds like a plan!

@cgestes
Copy link
Contributor Author

cgestes commented Feb 21, 2016

I'am delected to read about adding performance tests :)

And eventually fixing them.
My current solution to render >1000 items is to use a very simple component with CSS. (From react-mdl).

As you pointed the initial render time is quite important. I wonder if using CSS like hover instead of js state change would help also. (no rerender)

I dont have a lot of time to work on the subject and wait for memoization to come in, but I definitively plan to work on the perf of multi items components.

@nathanmarks
Copy link
Member

@cgestes When I take a look at this issue I'll add in some things to help measure initial rendering time -- the issue I chose for the initial example was regarding performance with re-renders.

I'll take a look at this after I get the chance to wrap that one up.

@nathanmarks nathanmarks changed the title [Performance] midinight benchmark [Core][Perf] Rendering performance in lists Apr 12, 2016
Holi0317 added a commit to Holi0317/lunar-event that referenced this issue Apr 17, 2016
This should not be merged until mui/material-ui#3289 is fixed.

Opening the dropdown causes about 4000ms under a desktop, using Chrome.
This provides a awful UX to users because of the lag issue.
@gchudnov
Copy link
Contributor

Got it, thank you!

@linde12
Copy link

linde12 commented Aug 5, 2016

Hey, just wondering if anyone is looking in to this or if some help is needed? I make heavy use of tables in my project and i would love to continue using material-ui to render them but this performance thing is a problem so i would love to help if anyone's got some info on whats going on.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 5, 2016

@linde12 As we have identified two major inefficient patterns:

  • The memory footprint of the current inline style approach.
  • Excessive calls to cloneElement
    I think that the next branch would be good place to solve them.
    @nathanmarks Any insight about what is missing and how we could help?

@gerbal
Copy link

gerbal commented Aug 14, 2016

For anyone interested here's a reskin of react-select I did as a stop gap to replace SelectField until its performance issues are sorted: http://codepen.io/gerbal/pen/dXaJyK

@petermikitsh
Copy link
Contributor

I made this GIF to illustrate the performance issue. It has ~500 elements in the list.

selectfield

@petermikitsh
Copy link
Contributor

As previously mentioned, inline styles degrade performance. 😞

Moving away from inline styles would be a wise long-term choice to keep material-ui snappy. Sluggish UI components aren't all that usable. The good news is I've used a good portion of the material-ui components and this is the first time I observed a substantial performance issue.

However, you likely wouldn't be able to move away from inline styles without introducing breaking changes. For example, consumers of the project may have to augment their existing build setups (e.g., adding css-loader if using webpack, etc).

@petermikitsh
Copy link
Contributor

petermikitsh commented Sep 13, 2016

Same story with AutoComplete as well. Takes about two seconds for the letter 'a' to render after pressing the key:

selectfield

@dcworldwide
Copy link

Just noticed this issue today. I have a List with nested ListItems that render tables. Sooooo slow. Glad to see you're on top of it 👍

@oliviertassinari
Copy link
Member

@dcworldwide In the meantime, you could try using https://github.com/bvaughn/react-virtualized to render a very large amount of ListItem. It's what I'm doing, it's pretty smooth.

@dotnwat
Copy link

dotnwat commented Oct 27, 2016

@oliviertassinari do you have an example of using react-virtualized with material-ui table or list? I'm having trouble seeing how the integration happens.

@oliviertassinari
Copy link
Member

@noahdesu Actually, I'm wondering if it's even possible with the master version of the component. The parent element is doing some cloneElement that are preventing any integration with react-virtualized. That limitation has been removed on the next branch. That should be working an any other item component.

@neil-rubens
Copy link

setting maxSearchResults to some reasonable value* allows for a very smooth experience.

* could not display more than 100 items on a screen anyways

p.s. works for me on json with 1K+ nested items

@slavab89
Copy link
Contributor

slavab89 commented Nov 5, 2016

@neil-rubens except when you want the user to see all his options, have some scroll bar there :)
Waiting so much for the next branch 😭 hehe

@yunderboy
Copy link

What's the status on this issue, what would be the best way for me to contribute?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 8, 2016

@yunderboy I have migrated a part of my app using the <ListItem /> with the next branch.
I have noticed that we spend 25% less time rendering the elements. That's much better.

I'm wondering about the first example benchmark. It's not creating x intermediary components.
The two following one are more interesting:

Inline: 4sec
CSS: 3sec

That's pretty much the win I have measured. That's a first approximation of what we are going to get with the CSS-in-JS approach of the next branch.
cc @nathanmarks

@KarenKelly818
Copy link

I know this is an ongoing discussion and some attempts have been made to alleviate the performance issues (e.g., shouldComponentUpdate). I have improved performance using Chrome on my Macbook, but in testing via Browserstack on IE11 and Edge, the Checkbox component I'm using in my material-virtualized table is basically unusable. It's taking several seconds to check/uncheck which obviously is untenable. We are currently using material-ui 0.16.4. Has the most recent version (0.16.7) made any improvements to the performance of material-ui Checkboxes?

@gchudnov
Copy link
Contributor

@KarenKelly818,
guess, it will be improved in the next branch. Here is the roadmap

@oliviertassinari
Copy link
Member

it will be improved in the next branch

Yes, the performance of the <List /> on the next branch are much better. I'm not sure we could do anything to speed it up at this point.
I think that we could be closing the issue. The only thing missing from my perspective is a performance test suite.

@oliviertassinari oliviertassinari changed the title [Core][Perf] Rendering performance in lists [List] Rendering performance issue Feb 12, 2017
@oliviertassinari oliviertassinari added the component: list This is the name of the generic UI component, not the React module! label Feb 12, 2017
@PratapAkshay
Copy link

I need to Perform UI Automation testing on my MUI Table but when I run my testing it Perform Automation on all tags when it comes to MUI table's Tr(table row) it does not perform any Automation. I am using XPath for this.

@nscozzaro
Copy link

nscozzaro commented Mar 30, 2020

Hi, I'm also struggling with performance issues with the List component. I'm using @material-ui/core 4.9.8.
I have 400 items in the list, and as you can see below, the response is really laggy. For some reason my screen cap video got slowed down when I converted to GIF, but it actually helps to highlight the performance issue.
Untitled
I also tried React Window (even though I'd prefer not to use another library if I don't have to, to avoid dependency bloat), however it also gives me an issue: the ripple is now too fast and glitchy. Below I'm clicking normally, which has almost instantaneous ripple (followed by a long click which goes really slowly).

ezgif-2-13ae23bab7d9

I've read through this issue, but I'm still not sure of the recommended approach to addressing this issue. Please let me know what I should try, thank you!

@dantman
Copy link
Contributor

dantman commented Mar 30, 2020

@nscozzaro Move your list item contents out of the loop you create them in to a small dedicated component, use memo on that component, and make sure the props you pass to it are pure and in your map you pass a selected boolean rather than the currently selected value(s) (so a selection change on one item doesn't make the props change on every list item).

@amitsaxucsb
Copy link

@dantman I tried the way you suggested but it is taking time to load if we have more then 200 records in list.

@KevinDanikowski
Copy link

I've been successful with using a timeout hook, so the initial render isn't bad. This combined with React.memo() will make your lists bearable, the only slow thing now is taking them off the DOM in an Accordian for example.

  useEffect(() => {
    if (timeout) {
      const interval = setTimeout(() => {
        setRender(true)
      }, timeout)
      return () => {
        clearTimeout(interval)
      }
    }
  }, [])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: list This is the name of the generic UI component, not the React module! performance
Projects
None yet
Development

No branches or pull requests