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] Explore scrolling performance issues #569

Closed
2 tasks done
westhouseit opened this issue Nov 10, 2020 · 28 comments · Fixed by #9037
Closed
2 tasks done

[DataGrid] Explore scrolling performance issues #569

westhouseit opened this issue Nov 10, 2020 · 28 comments · Fixed by #9037
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! performance

Comments

@westhouseit
Copy link

westhouseit commented Nov 10, 2020

I've only just started using this component so I might be missing something, but it looks like there's a lot of unnecessary re-renders. The three I've split out are repeated many times, it looks like one repeat for every row, but I will test more.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behaviour 😯

whyDidYouRender.min.js:8 RowCells
whyDidYouRender.min.js:8 {RowCells: ƒ}RowCells: e=> {…}arguments: (...)caller: (...)length: 1name: ""__proto__: ƒ ()[[FunctionLocation]]: index-esm.js:15[[Scopes]]: Scopes[3]__proto__: Object "Re-rendered because of props changes:"
whyDidYouRender.min.js:8 props.hasScroll
whyDidYouRender.min.js:8 different objects that are equal by value. (more info at http://bit.ly/wdyr02)
whyDidYouRender.min.js:8 {prev hasScroll: {…}}prev hasScroll: x: falsey: false__proto__: Object__proto__: Object "!==" {next hasScroll: {…}}next hasScroll: x: falsey: false__proto__: Object__proto__: Object
whyDidYouRender.min.js:8 GridCell
whyDidYouRender.min.js:8 {GridCell: ƒ}GridCell: e=> {…}arguments: (...)caller: (...)length: 1name: ""__proto__: ƒ ()[[FunctionLocation]]: index-esm.js:15[[Scopes]]: Scopes[3]0: Closure {react__WEBPACK_IMPORTED_MODULE_0__: {…}, _material_ui_core_utils__WEBPACK_IMPORTED_MODULE_1__: Module, _material_ui_core_Checkbox__WEBPACK_IMPORTED_MODULE_2__: Module, _material_ui_core_Badge__WEBPACK_IMPORTED_MODULE_4__: Module, _material_ui_core_IconButton__WEBPACK_IMPORTED_MODULE_5__: Module, …}1: Closure (./node_modules/@material-ui/data-grid/dist/index-esm.js) {__webpack_exports__: Module, __webpack_require__: ƒ}2: Global {window: Window, self: Window, document: document, name: "", location: Location, …}__proto__: Object "Re-rendered because of props changes:"
whyDidYouRender.min.js:8 props.children
whyDidYouRender.min.js:8 different React elements (remember that the <jsx/> syntax always produces a *NEW* immutable React element so a component that receives <jsx/> as props always re-renders.). (more info at http://bit.ly/wdyr02)
whyDidYouRender.min.js:8 {prev children: {…}}prev children: {$$typeof: Symbol(react.element), type: {…}, key: null, ref: null, props: {…}, …}__proto__: Object "!==" {next children: {…}}next children: {$$typeof: Symbol(react.element), type: {…}, key: null, ref: null, props: {…}, …}$$typeof: Symbol(react.element)key: nullprops: {element: undefined, value: undefined, field: "__check__", data: {…}, getValue: ƒ, …}ref: nulltype: {$$typeof: Symbol(react.memo), compare: null, displayName: "CellCheckboxRenderer", type: ƒ}_owner: FiberNode {tag: 15, key: null, elementType: {…}, stateNode: null, type: ƒ, …}_store: {validated: true}_self: null_source: null__proto__: Object__proto__: Object
whyDidYouRender.min.js:8 CellCheckboxRenderer
whyDidYouRender.min.js:8 {CellCheckboxRenderer: ƒ}CellCheckboxRenderer: ({rowModel:e,value:t})=> {…}arguments: (...)caller: (...)length: 1name: ""__proto__: ƒ ()[[FunctionLocation]]: index-esm.js:15[[Scopes]]: Scopes[3]__proto__: Object "Re-rendered because of props changes:"
whyDidYouRender.min.js:8 props.getValue
whyDidYouRender.min.js:8 different functions with the same name. (more info at http://bit.ly/wdyr02)
whyDidYouRender.min.js:8 {prev getValue: ƒ} "!==" {next getValue: ƒ}

Expected Behaviour 🤔

Little to no re-renders

Steps to Reproduce 🕹

Using demo code for a basic Data Grid from the docs: https://codesandbox.io/s/material-ui-issue-forked-ni3nx

Open devtools console to see the output.

Context 🔦

Your Environment 🌎

Tech Version
Material-UI X 4.0.0-alpha.9
React ^16.13.1
Browser Chromium 86.0.4240.183 (Official Build) snap (64-bit)
@westhouseit westhouseit added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 10, 2020
@oliviertassinari oliviertassinari added performance component: data grid This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 10, 2020
@oliviertassinari oliviertassinari changed the title Reducing re-renders [DataGrid] Reducing re-renders Nov 12, 2020
@oliviertassinari
Copy link
Member

Did you measure a specific performance issue? Why does it matter how many time it rerenders?

@westhouseit
Copy link
Author

westhouseit commented Nov 13, 2020

Ok, I've updated the demo to include 900 rows. Run it, open Devtools in clean browser, set cpu slowdown to 6x, and run the profiler. I'm getting:

Animation Frame Fired
WarningRecurring handler took 898.07 ms

I'm not a performance expert, so I may well be misinterpreting this. Come to think of it, I wonder if this is simply a case of needing the row virtualization.

Edit: Oh, and I disabled whyDidYouRender for profiling since it looks like it adds significantly to the apparent slowness.

@ognjenio
Copy link

I think I'm facing the same issue. The grid I have has 87 rows with 15 columns for a total of 1260 cells.

It seems that scrolling horizontally re-renders all the cells while scrolling vertically doesn't.

It also seems that all columns are rendered five times when the grid is first rendered slowing it down.

Changing columnBuffer to 25 seems to have improved it slightly.

Setting renderCell made it significantly worse to the point of not being usable. The component I'm rendering is literally just a text field that calls a function on blur.

@dtassone
Copy link
Member

@ognjenio do you have a codesandbox?

@ognjenio
Copy link

@ognjenio do you have a codesandbox?

It was already late last night, so here it is: https://codesandbox.io/s/awesome-black-b812m

@oliviertassinari
Copy link
Member

@ognjenio Thanks. I have further simplified the reproduction down to: https://codesandbox.io/s/infallible-archimedes-339tz, deployed to production: https://csb-339tz.netlify.app/.
For comparison, react-window: https://codesandbox.io/s/wandering-sun-6o6er, deployed to production https://csb-6o6er.netlify.app/

The latter performs better. However, It seems that TextField is simply too slow for this use case.

@ognjenio
Copy link

Just FYI we replaced the TextField with a div with contenteditable eventually.

However, once the number of rows increased to ~2000 it once again became unusable so we've had to drop it. Luckily we discovered this early.

@oliviertassinari
Copy link
Member

@ognjenio with virtulization, the number of rows shouldn't matter. Do you have an example we can look into?

@ognjenio
Copy link

It's the same thing but with more rows. Instead of having ~1200 cells as in the previous example, there are now ~60k cells.

Scrolling vertically is still fine. But scrolling horizontally still renders all the cells. Even though now it's just a div (and not a TextField) it takes a while.

Plus all the columns are being rendered five times on init, so it even takes a while for the grid to render in the first place.

@phettz

This comment has been minimized.

@EdmundsEcho

This comment has been minimized.

@Richard87
Copy link

Richard87 commented Mar 15, 2022

Hi, we also have a issue with the DataGrid (Pro), while having a modal open to add a new row (Doctor) in our case, the datagrid re-renders about 15 times, while 1 frame is for our modal (changing a letter in a textfield).

Is there any specific prop that should be memoized to avoid excesive re-rendering?

Also, the rerender takes a long time, which caused the modal form field to feel extremely sluggish, without any simulation, on a Macbook Pro M1 / Firefox

profiling-data.15.03.2022.18-13-38.json.zip

@EdmundsEcho
Copy link

EdmundsEcho commented Mar 15, 2022

@Richard87 There may be some pointed advice from others, but having worked with the grid and re-rendering issues, have you tried the following:

  1. ensure the state of your modal is on a different state branch than the grid

  2. use the api (refApi if memory serves); it's not the "first go to", but you are changing the collection size and thus the array that points to each record so may want to have that extra control. My hunch is that memoizing the record values will help, however, only if you avoid "inserting" a record, but rather concatenate/append it to the collection. This way record 3 in the array remains 3. The sorting, and rendering therein, should be isolated from the add/remove operations. So, you may get away with an imperative mutation (adding a new prop to the existing object array), and a pure change in the sort order which catches the new record.

  3. the last idea that comes to mind, in the event you can use a server-side pagination, then whatever goes on in the browser is limited, and more performant accordingly (how often do users really want to track more than even 50 records simultaneously?... as long as pulling records is quick).

All in all, the last statement has me wonder if it's at all feasible to avoid a re-render of every record when the js-array of records changes. If anyone has tricks to get around this, I look forward to learning how to do so myself.

@dildamaara
Copy link

Still facing this issue, any idea how to resolve this?
Am not even using Textfield within renderCell, using "Select" instead
so pretty sure problem is not with Textfield but with library itself specially "renderCell" as it's rendering again and again on scrolling either horizontally or vertically.

If we don't use renderCell , then it's pretty smooth. However have to use renderCell bcz of usability constriants.
Any help will be appreciated to resolve this.

@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Jan 25, 2023
@cherniavskii
Copy link
Member

The issue is still there in v6-beta.0, although the behavior is a bit different.
Codesandbox: https://codesandbox.io/s/strange-andras-ngi2ht?file=/src/Demo.js
Netlify deployment: https://csb-ngi2ht.netlify.app/

@cherniavskii cherniavskii changed the title [DataGrid] Reducing re-renders [DataGrid] Explore scrolling performance issues Jan 31, 2023
@cherniavskii cherniavskii moved this from 🆕 Needs refinement to 🔖 Ready in MUI X Data Grid Jan 31, 2023
@benosmond
Copy link

We're also having this issue when using renderCell to display some custom formatting for certain cells. We noticed a huge drop in grid performance once renderCell was added, even when it is just displaying the contents of the cell without custom formatting.

@m4theushw
Copy link
Member

@benosmond
Copy link

benosmond commented Mar 8, 2023

@benosmond You can try https://mui.com/x/react-data-grid/performance/#memoize-inner-components-with-react-memo

@m4theushw We actually already tried implementing that change, but it didn't have any impact on performance. We're not populating the grid with a lot of rows so I don't think this is the issue.

@tyukesz
Copy link

tyukesz commented Mar 29, 2023

We are using DataGridPremium and I'm facing the same issue. On vertical scrolling, when the cell enters or leaves the viewport, it rerenders each cell.

Even React.memo doesn't solved this issue.

Edit:
Apparently this rerender issue is solved by disableVirtualization. However it takes a bunch of time to load the time for first time. And I was just testing with ~20 rows. Disabling virtualization won't be a good approach for thousands of rows.

@romgrk
Copy link
Contributor

romgrk commented May 9, 2023

I have been able to observe performance issues with the example posted by @cherniavskii above, but the clear culprit is MUI TextField, not renderCell: replacing TextField with input removes the performance issue. In dev mode, TextField also displays more details about the root issue for that particular case:

image

But there might be other issues beyond the case specific to TextField.

@benosmond or @tyukesz: Could you provide a runnable code example that displays the issue?

@cherniavskii
Copy link
Member

In dev mode, TextField also displays more details about the root issue for that particular case:

I believe this error comes from <TextField multiline />. Could you try removing the multiline prop?
I think the point of using TextField here is to exaggerate the problem and look for potential optimizations.

@romgrk
Copy link
Contributor

romgrk commented May 9, 2023

If I review the previous comments, here are the issues I see that have been reported:

1. Huge drop in performance if renderCell is used, even if just displaying the contents of the cell without custom formatting
I have not been able to reproduce this issue. I would need a reproducible case to investigate more.

2. On vertical scrolling, when the cell enters or leaves the viewport, it rerenders each cell
I haven't been able to observe this with the react component profiler but I'll investigate more with WDYR today.

Other notes

I should also note that I've been profiling the performance of scrolling the datagrid with cells filled with either native elements or MUI elements, and the performance hit of using MUI elements is significant. Scrolling (or in other words, mounting+rendering a row) with native elements is smooth-ish, in the 15ms range. The same operation with MUI elements goes to 80ms for <Button>, and to 100-150ms for <TextField>. So some performance reports above might be attributed to this. But this is unrelated to the unnecessary re-renders.

Native element MUI Core element
<button>/<Button> scroll-button-native scroll-button-mui
<input>/<TextField> scroll-input-native scroll-input-mui

@mnajdova
Copy link
Member

From Core side, it really depends on the components that are used in the DataGrid. Using <Button /> vs <button /> I would expect that it takes more time to render - there is the business logic of the component and the styling solution. On the <input /> vs <TextField />, our <TextField /> component is doing a LOT, we already discussed that it's probably best if we split this component to multiple components. One quick win here would be setting up globally the disableInjectingGlobalStyles prop on the <InputBase /> so that global styles are not injected with each component that is rendered, see mui/material-ui#29213.

Going back to the issue on X side, what are the components used the most there? I am asking because we can try to optimize for these use cases. For e.g., we can try to avoid the time it takes to style the component by using the headless hook that Base UI provides and moving the styles up to the parent DataGrid component (similarly as it is done for example for the cells). This would be my general recommendation for any components that are rendered many times. In terms of how much performance gain we can do with this, we can use https://mui.com/system/getting-started/usage/#performance-tradeoffs - row c. is what is interesting for us.

Note, we are currently looking for ways how we can improve the performance in terms of the styling solution, and we will likely go with some zero-runtime CSS-in-JS solution, which long term may help if the styling is the thing that takes most of the time.

@benosmond
Copy link

1. Huge drop in performance if renderCell is used, even if just displaying the contents of the cell without custom formatting I have not been able to reproduce this issue. I would need a reproducible case to investigate more.

I checked our implementation and even though we weren't using <TextField /> our custom cell template did use MUI components like <Box /> and <Tooltip /> - however, I experimented with replacing these with just <p>Hi!: {params.formattedValue || params.value}</p> and while it's a little better the grid is still massively slower with renderCell than without.

For reference this is a grid with 50 records per page, and ~45 columns.

@romgrk
Copy link
Contributor

romgrk commented May 10, 2023

@benosmond Do you have a reproducible test case? I am unable to observe the behavior. Ideally a codesandbox link. You can fork any codesandbox demo on the documentation to base your test case on.

@MBilalShafi MBilalShafi moved this from 🔖 Ready to 🏗 In progress in MUI X Data Grid May 19, 2023
@cherniavskii cherniavskii moved this from 🏗 In progress to ✅ Done in MUI X Data Grid Jun 14, 2023
@akullpp
Copy link

akullpp commented Aug 16, 2023

Issue with scrolling still persists, no custom components or renders, disableVirtualization fixes it for me.

@romgrk
Copy link
Contributor

romgrk commented Aug 16, 2023

@akullpp Do you have a reproducible example I could inspect? Please open a new issue if it's related to a feature in particular (eg renderCell)

@akullpp
Copy link

akullpp commented Sep 21, 2023

Never mind, it was my mistake.

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
Status: Done
Development

Successfully merging a pull request may close this issue.