mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.52k stars 1.31k forks source link

[DataGrid] Default theme props degrade performance #10033

Open LinnaViljami opened 1 year ago

LinnaViljami commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

Link to live example:

https://codesandbox.io/s/react-typescript-forked-xddwq4?file=/src/App.tsx

Steps:

  1. Click row
  2. Check react profiler. Every row is rendered causing lagging performance.

Current behavior 😯

On click the whole grid is rerendered, including every row and header. This causes significant delay when rendering for example up to 20 rows with 10 columns = 200 cells

Expected behavior 🤔

Only rows which selection changes should be rerendered. No headers should be rerendered

Context 🔦

Row selection performance is not good enough

Your environment 🌎

npx @mui/envinfo ``` Don't forget to mention which browser you used. Output from `npx @mui/envinfo` goes here. ```

Order ID or Support key 💳 (optional)

72822

romgrk commented 1 year ago

You have clicked the "I have tested with the latest version" checkbox but it seems that you're not. Can you test with the latest version and confirm? This should be fixed.

LinnaViljami commented 1 year ago

I updated the latest packages now.

With react profilier we can see, that row rerendering is partially memorized. Headers are still all rerendered causing problems with large amounts. (for example 6ms per header causing significant lag if more than 10 headers)

I tested it out with 11 headers and 18 rows. The render time of the headers was 56,7ms, which happens on every row click. The render time of the rows was 24,5ms.

Here is the simplified profiler result with 2 columns and 8 rows

rows_performance

headers_performance

kealjones-wk commented 3 months ago

I just ran into this issue, with a bit of debugging I realized that this (at least for me) is caused by the presence of a custom theme specifying a defaultProps for MuiDataGrid.

Replicated here: https://codesandbox.io/s/focused-bas-rjzts7?file=/src/index.tsx:460-488

romgrk commented 3 months ago

For some reason that sandbox is stuck in loading state for me, so I can't inspect that code.

Any prop that isn't a primitive or memoized can cause re-renders, e.g. the initialState inline object changes on each render so it can cause some re-rendering:

<DataGrid initialState={{ a: 1 }} />

More details here: https://mui.com/x/react-data-grid/performance/

kealjones-wk commented 3 months ago

@romgrk Idk why that code sandbox is broke sorry about that, anyway i re-created it here: https://stackblitz.com/edit/react-mxv54z?file=index.tsx This and the previous code sandbox are forks of that exact same link you gave: https://mui.com/x/react-data-grid/performance/#visualization The only difference is the customTheme and ThemeProvider in index.tsx.

github-actions[bot] commented 3 months ago

:warning: This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

@LinnaViljami: How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

romgrk commented 3 months ago

This isn't completed yet, I need to refactor useThemeProps out for multiple reasons (this one and the design-system agnostic objective), I need to do more exploration.

romgrk commented 3 months ago

I could do a temporary fix for a bit more work, is this very impactful for anyone?

romgrk commented 1 month ago

@brijeshb42 Follow-up of the useThemeProps discussion: I've been trying to patch the issue here but it doesn't seem to be possible. We would need to re-implement useThemeProps to add a useMemo to it, but we don't have access to the default theme so we can't. What do you think about adding a stable option to that function to let it use useMemo only when required?