[DataGrid] Make possible to memoize rows and cells #7846

m4theushw · 2023-02-06T23:25:15Z

First step for #7807

Preview: https://deploy-preview-7846--material-ui-x.netlify.app/x/react-data-grid/performance/

Changelog

Breaking changes

The cellFocus, cellTabIndex and editRowsState props are not passed to the component used in the row slot. You can use the new focusedCell and tabbableCell props instead. For the editing state, use the API methods.

mui-bot · 2023-02-06T23:30:34Z

	Messages
📖	Netlify deploy preview: https://deploy-preview-7846--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	663.6	996.9	738.7	823.88	142.119
Sort 100k rows	ms	620.2	1,161.5	1,161.5	961.4	205.379
Select 100k rows	ms	262.5	351.1	306.9	308.7	31.769
Deselect 100k rows	ms	156.8	358.4	223	232.34	74.246

Generated by 🚫 dangerJS against 723a823

github-actions · 2023-02-07T12:34:55Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2023-02-08T22:46:58Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

m4theushw · 2023-02-15T17:46:24Z

packages/grid/x-data-grid-pro/src/DataGridPro/DataGridPro.tsx

@@ -27,13 +28,15 @@ const DataGridProRaw = React.forwardRef(function DataGridPro<R extends GridValid
  const privateApiRef = useDataGridProComponent(props.apiRef, props);
  useLicenseVerifier('x-data-grid-pro', releaseInfo);

+  const pinnedColumns = useGridSelector(privateApiRef, gridPinnedColumnsSelector);


The pinned columns must be passed via props now otherwise, once the column headers are memoized, they won't reflect the changes.

m4theushw · 2023-02-15T17:47:47Z

packages/grid/x-data-grid/src/hooks/features/columnHeaders/useGridColumnHeaders.tsx

-  const visibleColumns = useGridSelector(apiRef, gridVisibleColumnDefinitionsSelector);
-  const columnPositions = useGridSelector(apiRef, gridColumnPositionsSelector);
-  const columnHeaderTabIndexState = useGridSelector(apiRef, gridTabIndexColumnHeaderSelector);
-  const cellTabIndexState = useGridSelector(apiRef, gridTabIndexCellSelector);
-  const columnGroupHeaderTabIndexState = useGridSelector(
-    apiRef,
-    unstable_gridTabIndexColumnGroupHeaderSelector,
-  );
-  const columnHeaderFocus = useGridSelector(apiRef, gridFocusColumnHeaderSelector);
-  const columnGroupHeaderFocus = useGridSelector(
-    apiRef,
-    unstable_gridFocusColumnGroupHeaderSelector,
-  );
-  const densityFactor = useGridSelector(apiRef, gridDensityFactorSelector);
-  const headerGroupingMaxDepth = useGridSelector(apiRef, gridColumnGroupsHeaderMaxDepthSelector);
-  const filterColumnLookup = useGridSelector(apiRef, gridFilterActiveItemsLookupSelector);
-  const sortColumnLookup = useGridSelector(apiRef, gridSortColumnLookupSelector);
-  const columnMenuState = useGridSelector(apiRef, gridColumnMenuSelector);
-  const columnVisibility = useGridSelector(apiRef, gridColumnVisibilityModelSelector);
-  const columnGroupsHeaderStructure = useGridSelector(
-    apiRef,
-    gridColumnGroupsHeaderStructureSelector,
-  );


Using selectors may cause problems if the component is wrapped in React.memo. The solution is to pass the selected value via props, which makes the component to re-render if the value changes.

m4theushw · 2023-02-15T17:50:07Z

packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx

+    if (invalidatesCachedRenderedColumns) {
+      cachedRenderedColumns.current = visibleColumns.slice(firstColumnToRender, lastColumnToRender);
+      prevFirstColumnToRender.current = firstColumnToRender;
+      prevLastColumnToRender.current = lastColumnToRender;
+      prevVisibleColumns.current = visibleColumns;
+    }
+
+    const renderedColumns = cachedRenderedColumns.current;


The previous approach with a simple Array.slice was making the renderedColumns reference to change in every render. Here I created a small cache for it.

m4theushw · 2023-02-15T17:51:56Z

packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx

-          style={{
-            ...rowStyle,
-            ...rootRowStyle,
-          }}


If kept like that, the component will never be memoized because style is always changing. To support React.memo, the styles are computed once and saved to a cache.

m4theushw · 2023-02-15T18:28:04Z

docs/data/data-grid/performance/performance.md

+/>;
+```
+
+The following demo show this trick in action.


I didn't add a "before" demo because the comparison would be too ridiculous but here's https://codesandbox.io/s/crimson-tdd-fb0vec?file=/demo.tsx. Basically, in any click everything re-renders.

cherniavskii · 2023-02-16T15:47:24Z

docs/data/data-grid/performance/performance.md

+{{"demo": "GridWithReactMemo.js", "bg": "inline", "defaultCodeOpen": false}}
+
+:::warning
+We do not ship the components above already wrapped with `React.memo` because if you have cells that display custom content whose source is not the received props, these cells may display outdated information.


Shouldn't we memoize the Cell slot though?
renderCell is called in GridRow component and the returned value is passed to the GridCell.
So if there's renderCell in the column - the cell will always rerender - see this demo: https://codesandbox.io/s/dreamy-roentgen-px945x

True, I added React.memo to the cell component by default now.

Thinking about this more, wouldn't it make more sense to call renderCell inside the GridCell component?
This should allow memoizing the GridRow by default (as opposed to GridCell), and then the GridCell can be memoized by users. (Maybe then we could go a step further and add a check in React.memo to memoize GridCell by default if colDef.renderCell is defined?).
If it's too time-consuming, we can work on this later - this PR is already a great improvement!

What do you think?

If we add this check to React.memo we lose the default shallow comparison function. I don't know what's the best approach here. I propose to leave this as it is and see the implications.

cherniavskii · 2023-02-16T15:48:36Z

docs/data/data-grid/performance/performance.md

+{{"demo": "GridWithReactMemo.js", "bg": "inline", "defaultCodeOpen": false}}
+
+:::warning
+We do not ship the components above already wrapped with `React.memo` because if you have cells that display custom content whose source is not the received props, these cells may display outdated information.


By the way, is there a way to reduce rerenders of the cells that have renderCell? Would memoizing the component returned by renderCell work?

Would memoizing the component returned by renderCell work?

I tested and it didn't work. To memoize cells with custom renderers, we should call renderCell and cache its content, then invalidate the cached value if some prop changes. If we use the row prop as cache key, we fall again into the problem of the cell renderer depending on a selector. It's better to leave this for the user to cache the value. We can do a follow-up explaining how to do this.

github-actions · 2023-02-17T17:06:33Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

cherniavskii · 2023-02-20T16:58:41Z

docs/data/data-grid/performance/GridWithReactMemo.js

+      sx={{
+        height: 400,
+        width: '100%',
+        '& .updating': {


Suggested change

'& .updating': {

'&& .updating': {

To override the row hover background color, otherwise it might look like the row wasn't rerendered

Looks like it's still not enough - &&& is needed to actually override the hover color

I didn't understand. The row doesn't appear to be rendering again on hover.

I managed to reproduce the bug, it's tricky to see it.

docs/data/data-grid/performance/GridWithReactMemo.js

cherniavskii · 2023-02-20T18:50:53Z

docs/data/data-grid/performance/performance.md

+import {
+  GridRow,
+  DataGrid, // or DataGridPro, DataGridPremium
+  DataGridColumnHeaders, // or DataGridProColumnHeaders, DataGridPremiumColumnHeaders


Side note: This seems inconsistent with the other components' names that do not change regardless of the package used (one exception is DataGrid/DataGridPro/DataGridPremium).
Should we rename it to GridColumnHeaders for all packages?

cherniavskii · 2023-02-20T18:53:27Z

docs/data/data-grid/performance/GridWithReactMemo.tsx

+  return <TraceUpdates ref={ref} Component={DataGridProColumnHeaders} {...props} />;
+});
+
+const MemoizedRow = React.memo(RowWithTracer);


Is it expected that the whole row is rerendered on cell focus change?

Screen.Recording.2023-02-20.at.19.52.49.mov

Yes, because which cell is focused comes from the focusedCell prop. If I remove this prop and pass it to the cell, from inside the row, then the row won't re-render because no prop has changed, even though the props passed to the cell will have changed.

docs/data/data-grid/overview/DataGridProDemo.js

cherniavskii · 2023-02-20T19:09:21Z

docs/data/data-grid/performance/performance.md

+{{"demo": "GridWithReactMemo.js", "bg": "inline", "defaultCodeOpen": false}}
+
+:::warning
+We do not ship the components above already wrapped with `React.memo` because if you have cells that display custom content whose source is not the received props, these cells may display outdated information.


Thinking about this more, wouldn't it make more sense to call renderCell inside the GridCell component?
This should allow memoizing the GridRow by default (as opposed to GridCell), and then the GridCell can be memoized by users. (Maybe then we could go a step further and add a check in React.memo to memoize GridCell by default if colDef.renderCell is defined?).
If it's too time-consuming, we can work on this later - this PR is already a great improvement!

What do you think?

packages/grid/x-data-grid/src/components/GridRow.tsx

packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx

MBilalShafi

Great improvement 🚀

docs/data/migration/migration-data-grid-v5/migration-data-grid-v5.md

packages/grid/x-data-grid/src/DataGrid/DataGrid.tsx

docs/data/data-grid/performance/performance.md

docs/data/migration/migration-data-grid-v5/migration-data-grid-v5.md

Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com> Signed-off-by: Matheus Wichman <matheushw@outlook.com>

oliviertassinari · 2023-05-14T16:11:37Z

docs/data/data-grid/performance/GridWithReactMemo.js

+        '&&& .updating': (theme) => ({
+          background: teal[theme.palette.mode === 'dark' ? 900 : 100],
+          transition: theme.transitions.create('background', {
+            duration: theme.transitions.duration.standard,
+          }),


I love the idea, but I think that we could revisit the animation. Done in #8986 😁

m4theushw added the component: data grid This is the name of the generic UI component, not the React module! label Feb 6, 2023

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2023

m4theushw force-pushed the memo-rows-cells branch from 48a4b8f to 95736ad Compare February 7, 2023 13:17

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2023

This was referenced Feb 7, 2023

[data grid] Cells are rerendered on every click #7807

Closed

[data grid] updateRows triggers renderCell too many times #7821

Closed

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 8, 2023

cherniavskii added the performance label Feb 10, 2023

cherniavskii added this to the v6-beta milestone Feb 10, 2023

m4theushw mentioned this pull request Feb 13, 2023

DataGrid scrolling issue #7920

Closed

2 tasks

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 15, 2023

[DataGrid] Make possible to memoize rows and cells

0562193

m4theushw force-pushed the memo-rows-cells branch from 0f87516 to 0562193 Compare February 15, 2023 17:55

m4theushw commented Feb 15, 2023

View reviewed changes

m4theushw requested review from DanailH, cherniavskii, joserodolfofreitas and MBilalShafi February 15, 2023 18:47

m4theushw marked this pull request as ready for review February 15, 2023 18:47

cherniavskii reviewed Feb 16, 2023

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 17, 2023

m4theushw added 3 commits February 20, 2023 12:46

Memoize cell by default

3065d0f

Merge branch 'next' into memo-rows-cells

29aa143

Merge remote-tracking branch 'origin/next' into memo-rows-cells

7dd4395

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 20, 2023

cherniavskii reviewed Feb 20, 2023

View reviewed changes

Update colors for dark mode

b0fdc7e

m4theushw requested a review from cherniavskii February 20, 2023 17:50

cherniavskii reviewed Feb 20, 2023

View reviewed changes

m4theushw added 5 commits February 20, 2023 18:44

Increase specificity

c93175b

Rename column headeres component

437879e

Remove cell memoization

02acd1f

Avoid any

34975ee

Avoid listing slots as dependency

1381e47

MBilalShafi reviewed Feb 27, 2023

View reviewed changes

docs/data/migration/migration-data-grid-v5/migration-data-grid-v5.md Outdated Show resolved Hide resolved

packages/grid/x-data-grid/src/DataGrid/DataGrid.tsx Outdated Show resolved Hide resolved

m4theushw added 5 commits February 27, 2023 13:45

Use defaultMemoize to cache rendered columns

c5d1625

Improve migration guide

c342db6

No need to pass ColumnHeadersComponent prop

3fea0a0

Organize imports

221be7d

Fix markdown

7b899d1

m4theushw requested review from MBilalShafi and cherniavskii February 27, 2023 17:32

cherniavskii approved these changes Feb 27, 2023

View reviewed changes

m4theushw and others added 2 commits February 27, 2023 18:58

Andrew's suggestions

c083ab1

Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com> Signed-off-by: Matheus Wichman <matheushw@outlook.com>

Rerun CI

723a823

m4theushw merged commit 13c7b10 into mui:next Feb 27, 2023

m4theushw deleted the memo-rows-cells branch February 27, 2023 22:49

oliviertassinari mentioned this pull request May 14, 2023

[core] Add assertion about checkbox rerenders #8974

Merged

oliviertassinari reviewed May 14, 2023

View reviewed changes

oliviertassinari mentioned this pull request May 14, 2023

[docs] Smooth performance animation #8986

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Make possible to memoize rows and cells #7846

[DataGrid] Make possible to memoize rows and cells #7846

m4theushw commented Feb 6, 2023 •

edited

Loading

mui-bot commented Feb 6, 2023 •

edited

Loading

github-actions bot commented Feb 7, 2023

github-actions bot commented Feb 8, 2023

m4theushw Feb 15, 2023

m4theushw Feb 15, 2023

m4theushw Feb 15, 2023

m4theushw Feb 15, 2023

m4theushw Feb 15, 2023

cherniavskii Feb 16, 2023

m4theushw Feb 20, 2023

cherniavskii Feb 20, 2023

m4theushw Feb 27, 2023

cherniavskii Feb 16, 2023

m4theushw Feb 20, 2023

github-actions bot commented Feb 17, 2023

cherniavskii Feb 20, 2023

cherniavskii Feb 20, 2023

m4theushw Feb 20, 2023

m4theushw Feb 20, 2023

cherniavskii Feb 20, 2023

cherniavskii Feb 20, 2023

m4theushw Feb 20, 2023

cherniavskii Feb 20, 2023

MBilalShafi left a comment

oliviertassinari May 14, 2023

[DataGrid] Make possible to memoize rows and cells #7846

[DataGrid] Make possible to memoize rows and cells #7846

Conversation

m4theushw commented Feb 6, 2023 • edited Loading

Changelog

Breaking changes

mui-bot commented Feb 6, 2023 • edited Loading

github-actions bot commented Feb 7, 2023

github-actions bot commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBilalShafi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4theushw commented Feb 6, 2023 •

edited

Loading

mui-bot commented Feb 6, 2023 •

edited

Loading