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.55k stars 1.33k forks source link

[data grid] Performance issues when updating a large number of columns in a grid #15296

Open tp0ns opened 2 weeks ago

tp0ns commented 2 weeks ago

Steps to reproduce

Link to live example: (required) https://codesandbox.io/p/sandbox/xwkr2h

Steps:

  1. Click on Transpose Button
  2. Witness slow down

Current behavior

Grid is loading for a long time after charging a large data set of rows/ columns.

Expected behavior

Grid should load faster when charging large data set of rows/ columns.

Context

Hello, i was trying to implement a table transpose as explained in the summary of this feature request. When doing so on a large data set (here 8k rows/columns), grid is loading significantly slower (8,85 sec for 8800 rows/columns). I think transpose isn't the issue here but large data set is.

Environment is not at the latest version. But live example is.

Capture d’écran 2024-11-05 à 18 47 33

Your environment

npx @mui/envinfo ``` System: OS: macOS 15.0.1 Binaries: Node: 20.16.0 - ~/.nvm/versions/node/v20.16.0/bin/node npm: 10.8.1 - ~/.nvm/versions/node/v20.16.0/bin/npm pnpm: 9.9.0 - ~/.nvm/versions/node/v20.16.0/bin/pnpm Browsers: Chrome: 130.0.6723.92 npmPackages: @emotion/react: 11.13.3 => 11.13.3 @emotion/styled: 11.13.0 => 11.13.0 @mui/icons-material: 6.1.0 => 6.1.0 @mui/lab: 6.0.0-beta.9 => 6.0.0-beta.9 @mui/material: 6.1.0 => 6.1.0 @mui/styled-engine: 6.1.0 => 6.1.0 @mui/styles: 6.1.0 => 6.1.0 @mui/system: 6.1.0 => 6.1.0 @mui/x-data-grid-premium: 7.17.0 => 7.17.0 @mui/x-date-pickers-pro: 7.17.0 => 7.17.0 @mui/x-license-pro: 6.10.2 => 6.10.2 @mui/x-tree-view: 7.17.0 => 7.17.0 @types/react: 18.3.7 => 18.3.7 react: 18.3.1 => 18.3.1 react-dom: 18.3.1 => 18.3.1 typescript: 5.5.4 ```

Search keywords: performance data-grid data grid large number columns rows transpose createColumnsState Order ID: 86047

tp0ns commented 2 weeks ago

Correction : Slow down happens only on the second click on the transpose button.

tp0ns commented 2 weeks ago

I found a way to get around the performance issues using apiRef : Code here.

michelengelen commented 2 weeks ago

Hey @tp0ns ... thanks for raising this. The problem comes from the massive update in the columns. It gets clear if you change the code so that it simply goes from 8k columns to 1 (DEMO)

const columns = isTransposed
  ? transposedColumns
  : [...transposedColumns.slice(0, 1)];

I'll add it to the board 👍🏼

romgrk commented 2 weeks ago

Does this really need to be added? The updateColumns method is the recommended way to update large number of columns, React props don't let us make the same optimizations.

michelengelen commented 2 weeks ago

@romgrk Correct me if I am wrong, but does updateColumns support deleting columns? This will only update the first columns headerName and not remove the other columns:

apiRef.current.updateColumns([
  {
    ...transposedColumns[0],
    headerName: "update",
  },
]);

That's what is being asked for here.

tp0ns commented 2 weeks ago

The workaround i found uses updateColumns. As @michelengelen said, this method doesn't allow to delete columns so I used setColumnVisibilityModel to hide the one I want to hide / delete.

flaviendelangle commented 2 weeks ago

The updateRows method supports deleting rows:

export type GridUpdateAction = 'delete';

export interface GridRowModelUpdate extends GridRowModel {
  _action?: GridUpdateAction;
}

See the updateRows doc section

We could probably have the same approach for the column. Unless it's already doable but I did not find anything in the code.

lauri865 commented 1 week ago

Key prop to the rescue: https://codesandbox.io/p/sandbox/performance-data-grid-forked-gyzzrn

My old rant on this topic is very relevant – you should almost always resort to declarative updates, unless it's a very granular update to the state or you need the internal state intact: https://github.com/mui/mui-x/issues/10205

You probably don't want to keep the old internal state intact if you're transposing the columns – you're almost guaranteed to have issues if you use any other features of the grid (pinning, grouping, filtering, editing, etc.). Imperatively you'd have to manually take care of all that, and good luck with that. Plus you need to maintain it all the time if new features are added. Been there, done that, never again.

PS! If you build my version for production, the performance will be much better (it has a higher tax in dev mode, but you should never assess performance in dev mode in React). Doubt it'll be visibly faster imperatively. And more importantly, you have a much more maintainable codebase. The key prop value could be replaced with a more generic refreshKey state that you increment whenever you want to reinit the datagrid.

https://github.com/user-attachments/assets/759998a8-ad1d-4c5a-9de4-255e71d6bb20

tp0ns commented 1 week ago

I did use key prop as a get around (sorry for the late update). But I still think it's not ideal, if first load is that fast it seems possible to have good performances on re-render without the key prop trick.

lauri865 commented 1 week ago

Are you saying that you have performance issues with the key prop in a production bundle compared to apiRef? If anything, the inverse is true in this case.

Key prop is far from a trick. It's core functionality of React.