[DataGrid] Performance optimization

[dtassone]

Breaking changes

• Remove the params property element, rowIndex, colIndex, across all relevant params argument due to their heavy calculation work.

-  params.element
-  params.rowIndex
-  params.colIndex

• Refactor params.getValue across all params argument, due to reference change on each closure, this method is now pointing to apiRef.current.getCellValue.

-  params.getValue(field)
+  params.getValue(params.id, field)

• Remove usage of getValue in sort string number comparator .
• Refactor the arguments of setCellFocus to use id and field to match other api methods.

-  apiRef.current.setCellFocus: (indexes: GridCellIndexCoordinates) => void;
+  apiRef.current.setCellFocus: (id: GridRowId, field: string) => void;

• Add tabIndex and hasFocus in GridCellParams.

+  params.tabIndex
+  params.hasFocus

• Rename getRowIndexFromId to getRowIndex

-  apiRef.current.getRowIndexFromId(...)
+  apiRef.current.getRowIndex(...)

Work on #1506, #569, #1427

Preview: https://deploy-preview-1513--material-ui-x.netlify.app/components/data-grid/#commercial-version

[dtassone]

Test with 1 million rows

• New version
  https://7q6dc.csb.app/
• 4.0.0-alpha.26
  https://tyr6e.csb.app/

Filter by Quantity > 10000 => it shows a clear perf improvment ✅

[dtassone]

Filter and rendering should be faster.

Sorting will not be affected, as this change did not affect the comparator functions.

[oliviertassinari]

https://tyr6e.csb.app/

@dtassone I think that we should be very careful with using measures in process.env.NODE_ENV = development. It's ok to get a rough idea of where the bottleneck is, but I don't think that it has any other value. For instance, it should especially not be used to determine if a PR is an improvement or a regression. In our case, it was so slow before that it was as if the feature wasn't present 😁.

I had a look, using the production versions:

Sorting

In this reproducible benchmark, I have sorted the quantity column once.

https://deploy-preview-1513--material-ui-x.netlify.app/components/data-grid/#commercial-version

https://material-ui.com/components/data-grid/#commercial-version

It looks the same. If you account for the fact that sorting these very 100,000 rows takes <1ms, I don't see why it wouldn't render in ~100ms. I assume something is really wrong to require x10 more time.

Filtering

In this benchmark, I filter the rows with quantity > 1.

https://deploy-preview-1513--material-ui-x.netlify.app/components/data-grid/#commercial-version

https://material-ui.com/components/data-grid/#commercial-version

It looks like the filtering was not working before. This is a great win. However, if you account for the fact that filtering these very 100,000 rows takes <1ms, I don't see why it wouldn't render in ~100ms. I assume something is really wrong to require x10 more time.

[oliviertassinari]

Having a quick look at the bottleneck.

The x10 issue with the number sorting seems to come from the code structure, it does a lot of work that ends up trashed. Assuming N is the number of rows doesn't understand why:

• we compute the value O(2N), instead of O(N).
• we don't start by extracting all the values O(N) times, instead of doing it O(N log(N)) times.
• we compute row params to not use them.

I would expect a significant difference by solving the above

Regarding the filtering, the issue is simpler than sorting, there is no N log(N) scaling issue. It's more about computing a lot of things that are trashed in the end.

[oliviertassinari]

A summary of what we talk about during today's call

1. filtering doesn't work >60s => remove read DOM, x100 faster [XGrid] Performance issue when sorting/filtering 100k rows client-side #1506. We can add the same test case as we did for the sorting that was reading the DOM
2. slow rerender when heavy components in cell => prune rendering at one layer before the cell level [DataGrid] Explore scrolling performance issues #569. It work much better. We can add a test case to test if it rerenders or not
3. keyboard too slow => improve rerendering perf (2.) or bypass React with imperative DOM actions [DataGrid] Keyboard Navigation perf is slow #1427
4. sort n*log(n) => prepare arguments for the sortCompartor. win => log(n), x6 with 100k rows
5. filtering & sort computing extra params that are not used. win potential x4, maybe reproduce what vercel has to monitor performance feat(build): Log whether type checking is actually performed vercel/next.js#24440
6. hook dependencies comparison cost => maybe later in the future (no proof that it's a bottleneck now)
7. 60 FPS resize freeze: [DataGrid] Poor performance when container is resized #799
8. tree-shaking:

[oliviertassinari]

I hope the breaking changes are too disruptive. It looks great otherwise. One thing that would be awesome is to add a performance test to make sure we can't completely break it in the future, as we did a month ago.

I have pushed two commits to improve the performance of sorting, leveraging the log(N) point I made earlier. I have benchmarked on the "Quantity" column of https://deploy-preview-1513--material-ui-x.netlify.app/components/data-grid/#commercial-version.

Before

After

Enjoy.

The performance of filtering is borderline, I couldn't find any obvious win like for sorting, maybe there are, I don't have the time to look.

[oliviertassinari]

(Regarding the PR titles, I have renamed this one. We uppercase after the [label] and renamed optimisation, we use en_US not en_GB. This aims to have a clean and easy to read changelog, nothing more or less)

[oliviertassinari]

One thing that would be awesome is to add a performance test to make sure we can't completely break it in the future, as we did a month ago.

Actually, since we fixed the rerendering logic. I think that it's really important that we add a test for this as well, something in this order. It seems easy to break.

[oliviertassinari]

It doesn't seem that we need to render so many rows. I didn't look into how much it contributes to the time spent on this test but I assume it's not negligeable.

[oliviertassinari]

I don't know what this test is testing but not the rerender fix. It passes on HEAD. I suspect because we test the header and not the cell.

[oliviertassinari]

As a general note, it would be better to move this into a useEffect, so we can leave React start and interrupts its rendering as he sees fit, and only see when a change is actually committed.

[dtassone]

Not sure what you mean.
If I add a useEffect in the TestComponent, I don't think it will be meaningful as it won't reflect the grid internal rendering

[oliviertassinari]

In strict mode, React renders twice for each commit. In concurrent mode, React can render but not commit. useEffect allows to reliably assert that a React.memo() logic has pruned the rendering. Feel free to ask Sebastian for more details. It's important to well understand the notion if we want to be able to build the fastest data grid.

[eps1lon]

While useEffect would allow React 17.0 to yield rendering, it also introduces tearing. Say we want to render new state. Normally React would render the full UI and let the browser paint it. But now you're basically saying "hey just paint with the new state in certain areas of the UI but with the old state in other parts". And generally we don't want our UI to tear since it makes for a worse UX and harder to reason about the UI/state. It can even lead to infinite loops if you go back and forth with old/new state.

I would avoid tearing at all cost because it will degrade UX and not improve when you work on perf. Even worse, the UX will stay degraded in concurrent React when concurrent React is the solution to the problem. The render will become more "yieldy" i.e. if there's user input during render, React will process that input first before trying to finish rendering.

useEffect isn't a tool to improve performance. It's just for scheduling side-effects. Not for controlling the render timing. You're in for a world of pain if you think you can build your own scheduler that just considers a single component. React has been trying to build a scheduler for the whole UI for years now. I don't think you can solve that bottom-up.

[oliviertassinari]

@eps1lon Thanks for sharing these thoughts, however, I'm not sure we can act on it here.
The problem we are trying to solve is: How to reliably test that a memo logic is working (to avoid regressions)?
I had linked https://github.com/mui-org/material-ui/blob/4a0e2459dc33c5e99df8419fc251db90638d65e7/packages/material-ui-styles/src/makeStyles/makeStyles.test.js#L508 as a way to approach it. What would you recommend?

[dtassone]

if you just want to add

        const rendered = React.useRef(1);
        React.useEffect(() => {
          rendered.current += 1;
        });

Then you need to add it in a component that will be rerendered when the grid renders, that could be a header, footer, cell, or column header.

The way I did it, is via the renderCell and renderHeader functions, which get called when the cell or header render so it's the same. I can add an intermediary component such as below but I doubt it will add any value

const TestCell = ()=> {
        const rendered = React.useRef(1);
        React.useEffect(() => {
          rendered.current += 1;
        });
...
}

renderCell: ()=> <TestCell {...params} />

[oliviertassinari]

I can add an intermediary component such as below but I doubt it will add any value

Yes, this what I had assumed would ensure we reliably test the memo, based on: https://github.com/mui-org/material-ui-x/pull/1513/files#r629548477, e.g. if we are in strict mode, you should get two render call for each "interaction".

[eps1lon]

I'm not sure we can act on it here.

I was responding to your claims. Neither Damien nor I could make sense of them so I tried to bring some clarity.

How to reliably test that a memo logic is working (to avoid regressions)?

You don't. You monitor performance because that's the feature React.memo is implementing. You don't test the actual implementation detail.

Outside of the notes I have left, nothing to report :)

[oliviertassinari]

@dtassone So no peer reviews (I don't count 😁)?

[dtassone]

@dtassone So no peer reviews (I don't count 😁)?

😄 Well I took Outside of the notes I have left, nothing to report :) as ready beside the tests.

It's a very important PR, as it's touching also the new issues that @m4theushw is looking at now, so we needed it in right now. It's very important to integrate otherwise we will have to refactor new work...
I have created a new issue for the tests #1653

[bgandy94]

Edit 2: Must have been a webpack issue of some sort, I thought I restarted the dev server, but after killing it and restarting it, the issue is now resolved.. I can delete this comment if it's deemed to be unhelpful for any future purpose.

Edit 1: So it's working fine in the sandbox I created... So I'm still trying to figure out what the issue is. I'll reply back here when I track it down

So I'm sure it's something in my code, and I'll try to create a CSB asap, but in a valueGetter in a GridColDef, is it still valid to use the getValue() method to retrieve the cell value?

I just tried upgrading to 4.0.0-alpha.29 and had to change my getValue(name) to getValue(id, name) and now the return of the getValue call is undefined. is this related to #1731?

[oliviertassinari]

@dtassone Why did we change the API of getValue? I have tried:

diff --git a/packages/grid/_modules_/grid/hooks/features/rows/useGridParamsApi.ts b/packages/grid/_modules_/grid/hooks/features/rows/useGridParamsApi.ts
index 17126faa..b0f4783c 100644
--- a/packages/grid/_modules_/grid/hooks/features/rows/useGridParamsApi.ts
+++ b/packages/grid/_modules_/grid/hooks/features/rows/useGridParamsApi.ts
@@ -63,7 +63,7 @@ export function useGridParamsApi(apiRef: GridApiRef) {
         value: row[field],
         colDef: apiRef.current.getColumn(field),
         cellMode: apiRef.current.getCellMode(id, field),
-        getValue: apiRef.current.getCellValue,
+        getValue: (...args) => apiRef.current.getCellValue(...args),
         api: apiRef.current,
         hasFocus: cellFocus !== null && cellFocus.field === field && cellFocus.id === id,
         tabIndex: cellTabIndex && cellTabIndex.field === field && cellTabIndex.id === id ? 0 : -1,

I couldn't notice a change in the number of React renders nor in the time taken by the filtering to handle 100k rows.

[dtassone]

       getValue: (...args) => apiRef.current.getCellValue(...args),

You create a new function every time you call getCellParams, so it will break the component memoization.

[oliviertassinari]

it will break the component memoization

@dtassone Thanks for the clarification. I have tried to follow to see where the property is used. getValue is used by getRowParams which results is never applied on a React element. getValue is also used by getBaseCellParams, spread on getCellParams, that is then provided to renderCell.

I guess it saves a rerender if a developer spread all the props into a component that is memo? But isn't the parent Cell, in the core of the data grid, already memo? avoiding the rerenders.

So it seems that we can restore the previous logic, is it accurate?

[oliviertassinari]

I guess it saves a rerender if a developer spread all the props into a component that is memo? But isn't the parent Cell, in the core of the data grid, already memo? avoiding the rerenders.
So it seems that we can restore the previous logic, is it accurate?

@dtassone I had a closer look, I'm giving up on the idea. Even if the parent of renderCell has a memo logic, making a memo of the children harmful in most of the cases (spending CPU cycles looping on the props to evaluate if it needs pruning or not). The breaking change has been released for a couple of weeks now, doing the opposite BC would likely be more harmful than helpful.

-params.getValue(params.id, field)
+params.getValue(field)