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

[Datagrid] activate vertical scroll page and checkbox selection causes performances issues #8932

Closed
2 tasks done
jdeca-decat opened this issue May 9, 2023 · 16 comments · Fixed by #8942
Closed
2 tasks done
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! performance

Comments

@jdeca-decat
Copy link

jdeca-decat commented May 9, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/lucid-cloud-oq39yg

Steps:

  1. install @mui/x-data-grid ^6.2.1 and react 18.2.0 on a React Typescript project (or see the codesandbox)
  2. make a div above the datagrid that collapse in vertical and add 5 rows (the goal is to activate the vertical scroll of the page)
  3. activate the collapse option to change the height of the div above the datagrid
  4. you can see the problem: this is not "smooth", I think there is some datagrid re-renders because its width has change due to the activation of the vertical scroll in the page, it's really embarrassing in term of performances... I have some messages like "forced reflow" or "click handler take ...ms"
  5. if you change the number of rows to 100 you can see the performances issues again when a box is selected, the datagrid generate a 1s latency at each selected checkbox...

Current behavior 😯

  • This is not "smooth", I think there is some datagrid re-renders because its width has change due to the activation of the vertical scroll in the page, it's really embarrassing in term of performances... I have some messages like "forced reflow" or "click handler take ...ms"
  • if you change the number of rows to 100 you can see the performances issues again when a box is selected, the datagrid generate a 1s latency at each selected checkbox...

Expected behavior 🤔

I have search a lot to optimize the datagrid and I have already include all the performances tips that recommended in Mui website for the latest version of the community datagrid, all github closed issues and stackoverflow discussions.. Please help all the people like me who are struggling to optimize this component.. Thanks !!!

Context 🔦

I NEED to keep a responsive width on the datagrid, an autoHeight and all renderCell methods

Your environment 🌎

npx @mui/envinfo @mui/x-data-grid ^6.2.1, react 18.2.0, typescript ^5.0.4, all browsers
@jdeca-decat jdeca-decat added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label May 9, 2023
@jdeca-decat jdeca-decat changed the title [MUI DATAGRID] performances issues when activating vertical scroll [MUI DATAGRID] vertical scroll page and checkbox selection performances issues May 9, 2023
@jdeca-decat jdeca-decat changed the title [MUI DATAGRID] vertical scroll page and checkbox selection performances issues [MUI DATAGRID] vertical scroll page and checkbox selection causes performances issues May 9, 2023
@jdeca-decat jdeca-decat changed the title [MUI DATAGRID] vertical scroll page and checkbox selection causes performances issues [MUI DATAGRID] activate vertical scroll page and checkbox selection causes performances issues May 9, 2023
@zannager zannager transferred this issue from mui/material-ui May 9, 2023
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label May 9, 2023
@romgrk romgrk self-assigned this May 9, 2023
@romgrk romgrk changed the title [MUI DATAGRID] activate vertical scroll page and checkbox selection causes performances issues [Datagrid] activate vertical scroll page and checkbox selection causes performances issues May 9, 2023
@romgrk
Copy link
Contributor

romgrk commented May 9, 2023

I can reproduce the issue, I'll look further into this.

@romgrk romgrk added performance and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels May 9, 2023
@romgrk
Copy link
Contributor

romgrk commented May 10, 2023

First observation, it seems that we're forcing an update in setCellFocus, which means that React is forced to do 2 updates: one for the mouseup event (which triggers focus management logic), and one for the click event (which triggers the checkbox update logic).

The above is no great, but the performance of the render update alone is a bigger issue: ~180ms to change a checkbox state is way too much.

Screenshot from 2023-05-10 04-25-16

@jdeca-decat
Copy link
Author

Thanks @romgrk ;) like you said changing the checkbox state causes a big re-render.. but I noticed that if we set a fixed height value for the datagrid, it's faster. Maybe the autoHeight props disable the datagrid virtualization ?

@jdeca-decat
Copy link
Author

And I tried to memoize the selection model but it does not change the problem

@yaredtsy
Copy link
Contributor

hey @jdeca-decat

It uses only one state for the whole thing. a simple state change like a focus updates the state which triggers rerender of the data grid, that's why the memoization feature was added.

Maybe the autoHeight props disable the datagrid virtualization ?

When you use the autoHeight option, the data grid resizes itself based on the number of rows, making all rows visible to the user at the same time. However, for large datasets, this may cause performance issues. Since the virtualization feature only renders what can fit in the grid's height, using autoHeight means that it can fit all rows and virtualization won't work properly. You can read more about this in the Doc.

@jdeca-decat
Copy link
Author

@yaredtsy thanks for your message. But I think it's always an issue to have a big latency when a row is selected in a datagrid with 100 lines and autoHeight props :( this component should be faster.

@yaredtsy
Copy link
Contributor

@romgrk i have tested your solution, and it even works well with 1000 rows https://codesandbox.io/s/jovial-microservice-m773m7?file=/package.json

@jdeca-decat
Copy link
Author

@yaredtsy the checkbox selection works well !!! But there is a very big latency when I want to collapse the Card element above the table (but I think it's "normal", the free version of the datagrid not support more than 100 rows, and in our example the virtualization is not enable -> autoHeight prop)

@yaredtsy
Copy link
Contributor

@jdeca-decat this is not Datagrid fault. It's b/c you are tracking the collapse state with react state so when the state changes its rerenders everything you must wrap the data grid component with React.memo.

Demo https://codesandbox.io/s/xenodochial-curie-pxugcs?file=/src/pages/home/components/datagrid.tsx

@jdeca-decat
Copy link
Author

@jdeca-decat this is not Datagrid fault. It's b/c you are tracking the collapse state with react state so when the state changes its rerenders everything you must wrap the data grid component with React.memo.

Demo https://codesandbox.io/s/xenodochial-curie-pxugcs?file=/src/pages/home/components/datagrid.tsx

Thanks for your response @yaredtsy ;) so what do you recommend in terms of best practices for placing component states ? Personally I have observed better performance when I group the states of my components in the same places like I do in the first codesandbox, and I use the useEffect hook to load server data so the page only re-renders all components under certain conditions (for example in the dependency of the useEffect I place the "nbrLine" state, in this way a call to the server is re-done when its value change and it cause updates across child components with the new values). I am looking for optimizations for our professional web application so if you have any tips please let me know :))

@yaredtsy
Copy link
Contributor

you can just use it the way you're using it. just make sure to use React.memo on the child components that are affecting the performance. you can use react profiler extension to monitor which component is rerendering and why.

@jdeca-decat
Copy link
Author

Okay thanks, so I will place my Datagrid into React.memo. And yes I know the react profiler extension but I need to look further into this because I have an incompatible warning. I think this extension is not available in React V18 or it needs to be updated.

@romgrk
Copy link
Contributor

romgrk commented May 15, 2023

Please also note that the DataGrid props need to be memoized or extracted. E.g.

<DataGrid slots={{ row: CustomRow }} />

Should be transformed to:

// outside the component
const slots = { row: CustomRow }
// inside the component
<DataGrid slots={slots} />

Or this transformation is also fine:

<DataGrid slots={useMemo(() => ({ row: CustomRow }), [])} />

Otherwise the slots object keeps changing, and the rows need a full re-render, because they depend on the slots. This requirement may be lifted in the future.

@jdeca-decat
Copy link
Author

Thanks @romgrk for your work, I will apply the changes immediately ;)

@MBilalShafi
Copy link
Member

Should be transformed to
// outside the component
const slots = { row: CustomRow }
// inside the component
<DataGrid slots={slots} />

I think it's a very nice point and it is something we should suggest in our documentation too 🤔

@romgrk Did you get a chance to somehow measure the impact of this optimization in your experimentation, so that we can relate the gains in docs too?

@romgrk
Copy link
Contributor

romgrk commented May 24, 2023

Yeah but our doc is filled with the inline version style, which is nicer for readability but bad for performance. I'll find a way to add that to the docs along with the scroll improvements (aka by default memoization).

@romgrk Did you get a chance to somehow measure the impact of this optimization in your experimentation, so that we can relate the gains in docs too?

Yeah, root props change implies all rows re-render, which can be from 50-100ms cost on my laptop.

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

Successfully merging a pull request may close this issue.

5 participants