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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] Scroll performance improvements #9037

Merged
merged 74 commits into from
Jun 14, 2023
Merged

[DataGrid] Scroll performance improvements #9037

merged 74 commits into from
Jun 14, 2023

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented May 18, 2023 •
edited
Loading

Closes #569
Closes #9013
Discussion #9001

This PR improves the scrolling performance as well as the overall performance by reducing the amount of re-renders, by applying the solution in point 1 of the discussion linked above. I have not included the proposed useGridRootPropsSelector in this PR to limit the scope & facilitate reviews. It can be added independently later.

Tasks:

Below are profiles of scrolling the following demo:

CPU: Intel Core 13th gen i7

export default function Component() {
  const { data } = useDemoData({
    dataSet: "Commodity",
    rowLength: 100,
    editable: true,
    maxColumns: 15
  });

  return (
    <Box
      sx={{
        height: 750,
        width: '100%',
      }}
    >
      <DataGridPro
        {...data}
        initialState={data.initialState}
        rowHeight={38}
        checkboxSelection
        disableRowSelectionOnClick
      />
    </Box>
  );
}
Before After
Screenshot from 2023-05-24 19-12-19 Screenshot from 2023-05-24 19-16-01

@romgrk romgrk marked this pull request as draft May 18, 2023 15:13
@mui-bot
Copy link

mui-bot commented May 18, 2023 •
edited
Loading

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9037--material-ui-x.netlify.app/

Updated pages

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 681.4 1,304.5 681.4 983.3 274.844
Sort 100k rows ms 575.7 1,245.5 575.7 900.66 236.462
Select 100k rows ms 228.9 394.3 299.2 303.62 52.73
Deselect 100k rows ms 156.5 288.5 213.4 222.34 51.97

Generated by 🚫 dangerJS against d583d74

@romgrk romgrk added performance component: data grid This is the name of the generic UI component, not the React module! labels May 18, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 24, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 24, 2023
@mui mui deleted a comment from github-actions bot May 25, 2023
MBilalShafi
MBilalShafi approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 👏
Thank you!

packages/grid/x-data-grid/src/components/cell/GridCell.tsx Show resolved Hide resolved
docs/data/data-grid/performance/performance.md Show resolved Hide resolved
@romgrk romgrk mentioned this pull request Jun 9, 2023
Merged
@MBilalShafi MBilalShafi mentioned this pull request Jun 9, 2023
Open
45 tasks
@romgrk
Copy link
Contributor Author

romgrk commented Jun 12, 2023

Should I merge this one despite the argos differences or should I debug them?

@romgrk romgrk mentioned this pull request Jun 12, 2023
Closed
2 tasks
@cherniavskii
Copy link
Member

@romgrk We can ignore those flags diffs, our teammates say they saw them in other PRs as well.

Could you merge the master into this branch? I'm surprised to see the grid on these screenshots:

Screenshot 2023-06-13 at 11 21 30

It should overflow after #9179 was merged, and I would expect Argos to have overflown screenshots as a reference for master 🤔

m4theushw
m4theushw approved these changes Jun 13, 2023
View reviewed changes
Copy link
Member

@m4theushw m4theushw left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! 🚀

packages/grid/x-data-grid/src/utils/Store.tsx Outdated Show resolved Hide resolved
packages/grid/x-data-grid/src/components/GridRow.tsx
getCellClassName,
} = rootProps;
const { slots, slotProps, disableColumnReorder } = rootProps;
const CellComponent = slots.cell === GridCellV7 ? GridCellV7 : GridCellWrapper;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do differently here and always render GridCellWrapper. There's some code duplication between these two components that make the file very long to read and understand. For example, the logic to compute params to pass to renderEditCell is the same. The difference is that GridCellV7 computes these params and renders the cell content, while GridCellWrapper computes the params and calls React.createElement with it. I don't know if it's worth it to improve this, duplicated code might be better than wrong abstractions.

https://github.com/mui/mui-x/pull/9037/files#diff-80d96e2b22a96f772bfef2ccadb051e7817a66729d2b9beff2c57d8fbcd04b2dR207-R222

https://github.com/mui/mui-x/pull/9037/files#diff-80d96e2b22a96f772bfef2ccadb051e7817a66729d2b9beff2c57d8fbcd04b2dR708-R723

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more details in some collapsed thread above, but the performance improvements required to move some of the logic inside the cells, but doing so affected the cell's props which are public, so the wrapper is there to provide a compatibility layer until V7. At V7, we can remove the duplicated code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m4theushw relevant discussion: #9037 (comment)

docs/data/data-grid/state/state.md Outdated Show resolved Hide resolved
romgrk and others added 3 commits June 13, 2023 18:17
@romgrk @m4theushw
Update docs/data/data-grid/state/state.md
096b735 
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
Signed-off-by: Rom Grk <romgrk@users.noreply.github.com>
@romgrk
lint
2028f8f
@romgrk
fix: column props propagation delay
d583d74
@romgrk romgrk merged commit 8380eae into mui:master Jun 14, 2023
@romgrk romgrk deleted the perf-scroll-improvements branch June 14, 2023 11:26
@romgrk romgrk mentioned this pull request Jun 14, 2023
Merged
4 tasks
oliviertassinari
oliviertassinari reviewed Jun 14, 2023
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

packages/grid/x-data-grid/src/hooks/features/focus/useGridFocus.ts
Comment on lines +330 to +331
(_, ev) => {
if (ev.relatedTarget?.className.includes(gridClasses.columnHeader)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this in #9336

packages/grid/x-data-grid/src/components/virtualization/GridVirtualScroller.tsx
Comment on lines 38 to 49
>(function GridVirtualScroller(props, ref) {
const { className, ...other } = props;
const rootProps = useGridRootProps();
const classes = useUtilityClasses(rootProps);

return (
<VirtualScrollerRoot
ref={ref}
className={clsx(classes.root, className)}
{...props}
className={clsx(classes.root, props.className)}
ownerState={rootProps}
{...other}
/>
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there are a good number of cases in Material UI where we could apply this optimization. To consider if it's worth doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we're doing a lot of allocations. There is also this issue with the spread operator.

packages/grid/x-data-grid/src/utils/fastMemo.ts
import * as React from 'react';
import { fastObjectShallowCompare } from './fastObjectShallowCompare';

export function fastMemo<T>(component: T): T {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it could be great to leave a comment that explains how this comparison is different from the one of React https://github.com/facebook/react/blob/4ddc019aca8e08fd59cb43de5e0032be77d6174e/packages/react-reconciler/src/ReactFiberBeginWork.js#L570

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it goes faaaaast, but yeah we should explain also fastObjectShallowCompare, it's tailored for a specific use-case.

package.json
Comment on lines 24 to 26
"eslint": "eslint . --cache --report-unused-disable-directives --ext .js,.ts,.tsx --max-warnings 0",
"eslint:fix": "yarn eslint --fix",
"eslint:ci": "eslint . --report-unused-disable-directives --ext .js,.ts,.tsx --max-warnings 0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the main repo, these are called "lint" (there are json, esm, css, etc. types of lints), I find it confusing. It's better here with eslint.

@joserodolfofreitas joserodolfofreitas mentioned this pull request Jul 11, 2023
Closed
@romgrk romgrk mentioned this pull request Aug 11, 2023
Merged
@MBilalShafi MBilalShafi mentioned this pull request Nov 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cherniavskii cherniavskii cherniavskii approved these changes

@MBilalShafi MBilalShafi MBilalShafi approved these changes

@flaviendelangle flaviendelangle flaviendelangle left review comments

@oliviertassinari oliviertassinari oliviertassinari left review comments

@m4theushw m4theushw m4theushw approved these changes

@DanailH DanailH Awaiting requested review from DanailH

Assignees
No one assigned
Labels
component: data grid This is the name of the generic UI component, not the React module! performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possibility to memoize rows when pinnedColumns are set [DataGrid] Explore scrolling performance issues
7 participants
@romgrk @mui-bot @cherniavskii @oliviertassinari @flaviendelangle @MBilalShafi @m4theushw