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

[SelectField][Dropdown Menu] Menu with many items performance much worse with popovers #2787

Closed
tehleach opened this issue Jan 4, 2016 · 33 comments
Labels
component: select This is the name of the generic UI component, not the React module! performance

Comments

@tehleach
Copy link
Contributor

tehleach commented Jan 4, 2016

I was using a SelectField before the changes related to popovers (release 0.14.0-rc2). The menu has about 400 items to choose from. Before, expanding and collapsing the menu was very fast (almost instant) but with the popover change it is quite slow (.5-1 second). I have verified that this performance change happened in 0.14.0-rc2 by switching back and forth between that and 0.14.0-rc1, and observe fast expand/collapse in rc1 but slow expand/collapse in rc2. Has anyone else noticed a similar performance hit? Was there any testing done with large lists of menuItems?

It's a bummer because the popover is giving us lots of nice features (such as actually being able to scroll through the menu items :) ) we were looking for but as is, we cannot use it.

@oliviertassinari
Copy link
Member

@tehleach After code quality and documentation, performances are the next point I want to take care of. That's worrying. Could we have a gif of what's happening? Or do I just need to render 400 simple items?

@tehleach
Copy link
Contributor Author

tehleach commented Jan 4, 2016

I don't think I can give you a gif. I would assume rendering 400 simple items would do the trick, but I'm looking at the example on the docs site with 100 items and it seems quick. It might have to do with having long primaryText strings? I can do some more thorough testing in a bit.

@tehleach
Copy link
Contributor Author

tehleach commented Jan 5, 2016

Ok, here's a quick project I threw together.. pretty bare bones. I'm just rendering a DropDownMenu component with 400 items. It's possible I'm doing something really stupid but I am still having the same performance issue. If anyone pulls it down and runs it, let me know if you run into any issues. I'd like to look into and try to improve the performance but I'm not sure when I'll have time to.

@oliviertassinari oliviertassinari added this to the 0.14.2 Release milestone Jan 5, 2016
@Yuliang-Lee
Copy link

I have same performance issue.My SelectField have about 500 items and it's very very slowly(2-3 second) when expanding and collapsing.I try to increase items number from 100 to 500 and then the SelectField's performance exponential decrease.

@Shearerbeard
Copy link

I had a similar experience but also have some spots in my app with several SelectFields of 10-20 items - even thought the list of items is short the performance seems to slow exponentially depending on how many total select fields are rendered on the page at any given time.

@jgoux
Copy link
Contributor

jgoux commented Feb 17, 2016

I have the same performance issue on all these elements :

  • List
  • SelectField
  • Autocomplete

Maybe using something like https://github.com/bvaughn/react-virtualized would be the solution ?

@bvaughn
Copy link

bvaughn commented Mar 7, 2016

react-virtualized author here. I think that implementing windowing by default within components that may potentially contain a lot of data (like selects) is a great idea. If you'd be interested in discussing a possible integration, @oliviertassinari, please reach out to me.

@oliviertassinari
Copy link
Member

@bvaughn The Material-UI team is definitely interested in this integration. We are glad you are willing to help 👍.

@bvaughn
Copy link

bvaughn commented Mar 7, 2016

Let's talk more then. Let me know how best to do that. (Email? Hangout? Discord?)

@nathanmarks
Copy link
Member

@oliviertassinari This seems like there may be quite a bit of work/time before this is resolved. Do we need it for 0.15.0 or can we move it?

@oliviertassinari oliviertassinari removed this from the 0.15.0 Release milestone Mar 8, 2016
@oliviertassinari
Copy link
Member

@nathanmarks That would be better to have into 0.15.0 but I don't think that this issues requires to introduce any breaking changes.
I don't have time to have a look at it. I have just removed it from the 0.15.0 milestone.

@marcelobateira
Copy link

I have this performance problem with the AutoComplete with 500 records :(

Is there any workaround when its not fixed yet?

Thanx

@oliviertassinari
Copy link
Member

@marcelobateira We have merged a first PR that improve the rendering by 20%. I'm pretty sure we can do much better.

@keown
Copy link

keown commented May 31, 2016

same problem here with ~250 items using material-ui 0.15. any news about the fix?

@ColeMurray
Copy link

Also having performance issues with dropdown

@bvaughn
Copy link

bvaughn commented May 31, 2016

Still happy to lend a hand with react-virtualized integration if there's still interest and things have progressed far enough to work on it. (I think last time we talked there was some refactoring work in the queue that you guys wanted to do first.)

@noisecapella
Copy link

We implemented this using react-virtualized, though it's been modified for our uses without regard for back compat so it's not easy to merge back upstream. It works great! mitodl/micromasters#568

@bvaughn
Copy link

bvaughn commented Jun 23, 2016

though it's been modified for our uses

@noisecapella: "it" being react-virtualized or material-ui?

@oliviertassinari
Copy link
Member

Material-UI from what I have seen.

@bvaughn
Copy link

bvaughn commented Jun 23, 2016

Ah, yeah. They forked List and Menu. Should have looked before asking. Haven't had ☕ yet. 😄

If react-virtualized needed to be forked I was going to ask if I could help merge the changes back into the mainline. B'c I think getting material-ui and react-virtualized working together would be great. But since it's a fork of your guy's component... your call I guess. Still happy to help if there's anything I can help with!

@noisecapella
Copy link

@bvaughn Yes, we didn't need to modify react-virtualized at all.

@ligaz
Copy link

ligaz commented Jun 28, 2016

We hit this performance issue as well. A quick profiling reveals that pure rendering does not kick-in for MenuItems. Further investigations reveal that the issue is likely to be in this snippet from Menu:

return React.cloneElement(child, {
  desktop: desktop,
  focusState: focusState,
  onTouchTap: (event) => {
    this.handleMenuItemTouchTap(event, child, index);
    if (child.props.onTouchTap) child.props.onTouchTap(event);
  },
  ref: isFocused ? 'focusedMenuItem' : null,
  style: mergedChildrenStyles,
});

The problem is that the onTouchTap and style props are fresh new objects on each render which results in shouldComponentUpdate returning true for the child (MenuItem).

@robin-pham
Copy link

robin-pham commented Jul 15, 2016

@ligaz I tested out your idea by setting onTouchTap and style to null, and thus easily returning a false for shouldComponentUpdate. I did my tests with a DropDownMenu of 1000 MenuItems, using React's Perf.

Before ---------- After
For these two images, the first chart is wasted time upon opening the menu, second chart is wasted time upon closing the menu:

Without any modifications it seems like upon closing, the MenuItems re-render 3 times unnecessarily, which is fairly stupid considering that they are just going to be unmounted anyways. By setting those objects to null, it speeds up the closing of the menu entirely, no time wasted, and near instant.

Inclusive and Exclusive rendering time upon opening the menu. I think this is expected, as each of the 1000 items renders once, nothing has been wasted, as seen in the earlier two experiments. Opening the menu was extremely slow regardless of the setting-to-null experiment.

So the issue for the closing the menu is definitely a shouldComponentUpdate problem, whereas the issue for opening the menu could just be the slowness of material-ui's method of rendering menus with popovers.

@khieu
Copy link

khieu commented May 22, 2017

I am using SelectField with about 900-1000 items and it seems very slow. I wonder if there's any update on this issue ? Thanks :)

@oliviertassinari oliviertassinari modified the milestone: v1.0.0-prerelease Jul 14, 2017
@alexjoverm
Copy link

alexjoverm commented Dec 8, 2017

I'm facing this issue in the lastest version 1.0.0-beta.22. With just about 50 items you can start to notice it. Doing some profiling, I see a bunch of events being triggered when opening the Select

image

Takes around 1.5s to open and render around 300 items on a MacBook Pro. Similar when closing.

The code I'm using:

            <FormControl fullWidth required={required} error={!!error}>
                {label && (
                    <InputLabel shrink htmlFor="select">
                        {label}
                    </InputLabel>
                )}
                <Select
                    id={id}
                    native={native}
                    disabled={disabled}
                    value={value}
                    onChange={this.onChange}
                    input={<Input id="select" />}
                >
                    {showEmpty && <MenuItem value="">--</MenuItem>}
                    {native
                        ? rows.map(row => (
                              <option key={getId(row)} value={getId(row)}>
                                  {getTitle(row)}
                              </option>
                          ))
                        : rows.map(row => (
                              <MenuItem key={getId(row)} value={getId(row)}>
                                  {getTitle(row)}
                              </MenuItem>
                          ))}
                </Select>
                {helperText && <FormHelperText>{helperText}}</FormHelperText>}
            </FormControl>

What can we do from our side to improve this?

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 8, 2017

With just about 50 items you can start to notice it

@alexjoverm The component wasn't designed to display a large list of choice. Pick the native implementation if you need to display more items, like the list of all the countries. Otherwise, we will need to invest in virtualization.

@alexjoverm
Copy link

In the code above, you can see that if you pass native option it uses the native version. But, its performance is very similar to the non-native option.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 8, 2017

@alexjoverm I'm rendering 250 countries with no performance issue with the native option.

@senluchen2015
Copy link

senluchen2015 commented Mar 12, 2018

I found similar performance issue with a list of 300. My workaround is to combine react virtualized [List] with TextField select. You do have to handle any event such as onChange or onKeyup yourself.

i.e.

rowRenderer() {
  const { key, index, style } = row;
  const { options = [] } = this.props;
  const option = options[index];
  return (
    <MenuItem
      key={key}
      style={style}
      value={option}
    >
      <ListItemText primary={option} />
    </MenuItem>  
  );
}

render() {
  const { options = [] } = this.props;
  const { selectedValues = [] } = this.state;
  return (
    <TextField        
      select
      SelectProps={{
        multiple: true,
      }}
      value={selectedValues}
    >
      <List
        width={listWidth}
        height={listHeight}
        rowCount={options.length}
        rowHeight={48}
        rowRenderer={this.rowRenderer}
      />
    </TextField>
  )
}

@oliviertassinari
Copy link
Member

@senluchen2015 Sweet! Would you mind adding demo about in the documentation? However, I'm wondering. Does the keyboard navigation works?

@senluchen2015
Copy link

@oliviertassinari

I made this https://codesandbox.io/s/o7zo71nk2q to demo react-virtualized. The keyboard navigation doesn't work as default, but with some workarounds it functions just fine.

@hutber
Copy link

hutber commented Aug 26, 2020

@senluchen2015 any chance we get a working example :D

@RubberChickenParadise
Copy link

@senluchen2015 any chance we get a working example :D

I got it working with the Autocomplete virtualized example and the following small modifications to the Virtualize function from the code example:

export default function WindowedSelect({id, label, value, options, getOptionLabel, onChange}) {
  const classes = useStyles();

  return (
    <Autocomplete
      id={id}
      style={{ width: 300 }}
      disableListWrap
      selectOnFocus
      autoComplete
      classes={classes}
      ListboxComponent={ListboxComponent}
      renderGroup={renderGroup}
      options={options}
      renderInput={(params) => <TextField {...params} variant="outlined" label={label} />}
      getOptionLabel={getOptionLabel}
      value={value}
        onChange={(event, newValue) => {
            onChange(newValue);
        }}
    />
  );
}

The main changes are taking in the the various things I wanted to change. It works great for my 500ish item list

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

No branches or pull requests