mui-x icon indicating copy to clipboard operation
mui-x copied to clipboard

[data grid] Cells are rerendered on every click

Open rgavrilov opened this issue 2 years ago • 15 comments

Order ID 💳

52426

Duplicates

  • [X] I have searched the existing issues

Latest version

  • [X] I have tested the latest version

The problem in depth 🔍

Same as: https://github.com/mui/mui-x/issues/3480#issue-1084636084

I just need you to confirm that a trivial UI operation is still causing unnecessary rerendering of all the cells (which is ridiculous).

Sandbox: https://codesandbox.io/s/datagrid-rerendering-issue-tzdri2

just click around and watch Value column being rerendered.

The issue: In my actual use case cells are rendered with 4 buttons inside. Even scrolling becomes too sluggish. I'm questioning whether this component would be usable in the UI.

Your environment 🌎

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

rgavrilov avatar Feb 02 '23 15:02 rgavrilov

At the very least, I need an example of how to cache a rendered cell. I have tried caching a result of renderCell:

            renderCell: (params: GridRenderCellParams<Response, Row, any>) => {
                const response = params.value!;

                if (cache[params.rowNode.id]) {
                    return cache[params.rowNode.id];
                } else {
                    console.log("missed cache");
                    return cache[params.rowNode.id] = <CheckboardCell response={ response }/>;
                }

            },

but that didn't help much. Even assuming that response doesn't change, the CheckboardCell is being rerendered.

rgavrilov avatar Feb 02 '23 16:02 rgavrilov

I have checked it with the profiler and everything is being rendered for some reason.

https://user-images.githubusercontent.com/34584266/216557645-1a8ff067-5967-4442-b0a2-f5347160b8c0.mp4

I think it should have been like this.

https://user-images.githubusercontent.com/34584266/216560002-d370c078-4c90-4885-9606-d381a13096d1.mp4

yaredtsy avatar Feb 03 '23 09:02 yaredtsy

I just need you to confirm that a trivial UI operation is still causing unnecessary rerendering of all the cells (which is ridiculous).

Yes, this happens because each cell is responsible for calling .focus() when it should be focused. AG-Grid handles this differently. In their case, the grid itself finds which element should have focus and calls .focus(). The DataGrid implements a declarative approach, while AG-Grid uses an imperative one. The problem with switching to the imperative approach now is that we need to ensure that the next cell to be focused is in the DOM (ReactDOM.flushSync could be used) and, when providing custom cell renderers containing interactive elements, users will have to handle the focus outside the cell component. Also, the management of the tabIndex attribute is tied to the focus logic, so it will also be impacted.

At the very least, I need an example of how to cache a rendered cell.

You could try wrapping CheckboardCell in React.memo but I don't believe this will bring any relevant improvement.

m4theushw avatar Feb 03 '23 17:02 m4theushw

hey @m4theushw, anything you do in the datagrid causes every componet(ColumnHeader, footer, row, cell) to rerender, even moving the mouse in and out of the datagrid is causing rerender.

https://user-images.githubusercontent.com/34584266/216837874-ee54a703-300e-499f-81da-1d6ff9d4cb09.mp4

yaredtsy avatar Feb 05 '23 18:02 yaredtsy

At the current state we can't easy solve this issue because the state of the grid is all stored in the same place, so if the parent component re-renders, their children will also re-render. Also, we can't wrap all components in React.memo because it will cause other problems like cells displaying outdated values. However, one workaround is to memoize the custom cell renderers, like I showed in https://github.com/mui/mui-x/issues/7821#issuecomment-1419427531

m4theushw avatar Feb 06 '23 17:02 m4theushw

I've been working in a way to allow to use React.memo to memoize rows, cells and column headers in #7846. By doing so, the number of re-renders reduced drastically. Could you check if performance has improved? Here's the CodeSandbox using the code from the PR: https://codesandbox.io/s/datagrid-rerendering-issue-forked-vizc7w?file=/src/DataTable.js

m4theushw avatar Feb 07 '23 23:02 m4theushw

it's perfect now, the only problem I had was, with the custom cell, I had to memorize it myself.

yaredtsy avatar Feb 08 '23 07:02 yaredtsy

@m4theushw : Really appreciation if memorize of rows, cells and column can get in place! The model should be to create custom Components like you do in the PR link?

Will test your commit and report back: https://pkg.csb.dev/mui/mui-x/commit/794239b6/@mui/x-data-grid <3

Note: Already memoized my custom renderCell, but it seem that the apiRef used through the passed props makes this problematic.

yelodevopsi avatar Feb 13 '23 14:02 yelodevopsi

The model should be to create custom Components like you do in the PR link?

@rompeldunk Almost yes, I'm actually only wrapping the default components in React.memo.

The model should be to create custom Components like you do in the PR link?

Only memoizing renderCell doesn't bring much benefit. What really needs to be done is to memoize the component that renders the cell and only with the change from this PR it's possible.

m4theushw avatar Feb 13 '23 16:02 m4theushw

@m4theushw Would it be possible to test an equivalent PR to x-data-grid-pro? I tried mixing the commit with my existing code and memoize it, but it it broke, as expected.

We're trying to render many hundreds, up to many thousand of rows. One checkboxSelection event renders the whole stack. Also tested with renderCell disabled, but as you mentioned, it doesn't matter much.

<DataGridPro
  rows={rows}
  columns={browseColumns}
  columnBuffer={3}
  rowHeight={32}
  autoHeight
  autoPageSize
  components={{
    // Memoized comp's here
  }}
  disableSelectionOnClick={true}
  checkboxSelection
  rowBuffer={3}
  onSelectionModelChange={handleSetSelectedRowIds}
  onCellEditCommit={handleCellEditCommit}
  onColumnHeaderClick={handleColumnHeaderClick}
  />

yelodevopsi avatar Feb 13 '23 21:02 yelodevopsi

@rompeldunk

Would it be possible to test an equivalent PR to x-data-grid-pro?

Yes, you can find links to the packages in the Packages section of Codesandbox CI build: https://ci.codesandbox.io/status/mui/mui-x/pr/7846/builds/344182

cherniavskii avatar Feb 13 '23 21:02 cherniavskii

@rompeldunk

Would it be possible to test an equivalent PR to x-data-grid-pro?

Yes, you can find links to the packages in the Packages section of Codesandbox CI build: https://ci.codesandbox.io/status/mui/mui-x/pr/7846/builds/344182

@cherniavskii I notice that there's a lot of breaking changes. Maybe I've picked version 6? Extranote: By this I mean other dependencies in other files (outside this scope/example) which have changed name, thus if this is not v5.**.* build

I tested this one: https://pkg.csb.dev/mui/mui-x/commit/794239b6/@mui/x-data-grid-pro

Currently using "@mui/x-data-grid-pro": "5.15.1",

yelodevopsi avatar Feb 14 '23 10:02 yelodevopsi

Just to clarify: (in my use-case at least)

  • It is the params from the renderCell (params: GridRenderCellParams) which cause the problems, since i.e. the api reference which changes for every render?

  • Wouldn't using a callback instead (to a state, setState) with none-changing ref instead do the trick?

  • Would an alternative optimization-technique be to use the components and componentsProps props?

yelodevopsi avatar Feb 15 '23 13:02 yelodevopsi

I notice that there's a lot of breaking changes. Maybe I've picked version 6?

Yes, it's v6. We do not plan to release this improvement on v5 since we are going to release v6 stable during the next 3-4 weeks.

cherniavskii avatar Feb 15 '23 13:02 cherniavskii

It is the params from the renderCell (params: GridRenderCellParams) which cause the problems, since i.e. the api reference which changes for every render?

@rompeldunk Partially yes, but the problem here appears before reaching renderCell. It's the props passed to the component responsible for rendering the rows that are changing on every render.

m4theushw avatar Feb 15 '23 18:02 m4theushw

Is there a specific use case where you can observe the data grid performance degradation? We need a real-world example to take action on this. It's not clear whether it's a UX or DX issue.

cherniavskii avatar Apr 25 '23 14:04 cherniavskii

Since the issue is missing key information and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

github-actions[bot] avatar May 02 '23 15:05 github-actions[bot]

Still facing the problem. The whole DataGrid is rerendered on every cell or row click, all rows and cells are rendered. It causes a significant performance issue with a table over about hundred cells

LinnaViljami avatar Aug 11 '23 11:08 LinnaViljami