[DataGrid] Experiment without raf

[oliviertassinari]

Opened to get the CI running

[dtassone]

So?

[dtassone]

Don't forget to scroll while you run the fast streaming update story

[oliviertassinari]

@dtassone No idea yet, I'm waiting for the deployment to test it in production mode. Is there a specific point to pay attention to?

[dtassone]

@dtassone No idea yet, I'm waiting for the deployment to test it in production mode. Is there a specific point to pay attention to?

Just FYI I tried all that. But yeah let me know what you think

[oliviertassinari]

Comparison cases:

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

Streaming:

• https://deploy-preview-507--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-streaming--fast-update-grid
• https://master--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-streaming--fast-update-grid

I can't notice a difference. Do you?

If I had to work on improving the performance of the grid, maybe I would start with this: If we apply this diff:

diff --git a/packages/grid/_modules_/grid/GridComponent.tsx b/packages/grid/_modules_/grid/GridComponent.tsx
index f4c77126..a7a21487 100644
--- a/packages/grid/_modules_/grid/GridComponent.tsx
+++ b/packages/grid/_modules_/grid/GridComponent.tsx
@@ -92,6 +92,8 @@ export const GridComponent = React.forwardRef<HTMLDivElement, GridComponentProps
       [gridState.options, gridState.containerSizes],
     );

+    console.log('render')
+
     return (
       <AutoSizer onResize={onResize}>
         {(size: any) => (

We see that the whole grid is rendered at each update (scroll or screaming push). Can we scale the performance of the grid by retaining this behavior? The more features we have, the more hooks function will be called at each render. I think that it could be interesting to benchmark the difference of performance when a scroll event or a streaming event re-render the least possible amount of hook/components, the Viewport only?

[dtassone]

There is already a logger.info at every render.

Check the perf in develop...

[oliviertassinari]

Check the perf in develop...

Yeah agree, we also need to consider this environment, as developers will experience it too. Could you confirm that the issue in development is that the scrolling events don't have enough priority over the update events (it freeze)? Or is there something else?

[dtassone]

Check the perf in develop...

Yeah agree, we also need to consider this environment, as developers will experience it too. Could you confirm that the issue in development is that the scrolling events don't have enough priority over the update events (it freeze)? Or is there something else?

Multiple things!

1. The logger makes things slow but it's good to see what is going on, hence we can easily deactivate it
2. When you do a setState with a function in parameters or with useReducer, then all updates will run. Look at the counter in streaming it slowly inc +1. If you use raf, we have something like 3 updates.
   So in prod it's faster but all updates run separately it's just well optimised.
3. Scrolling and streaming are separates, so you don't scroll and just stream it will still be slow, but when you scroll, then you need to show a smooth refresh hence why I coordinated with raf.
   Rows => update state, trigger rerender => scroll start at the same time => cancel render => trigger a new one => render latest state... ie (50 partial row updates, + scroll , can do 1 render with raf), if you use setState it will do 50 updates. In prod it's more optimised but still the raf is faster in my opinion

[dtassone]

Fastest as the raf are well coordinated as they are shared in the whole component
https://deploy-preview-506--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-streaming--fast-update-grid

[oliviertassinari]

1. I'm not sure to understand. Is there a difference in logging behavior? By default, the logger is disabled for debug and info levels.
2. You are right. I can reproduce this re-rendering issue in: https://codesandbox.io/s/cocky-shaw-upc12?file=/src/App.tsx. I was expecting that React would batch two setState calls but we can see 3 console.log. I have found this batching on raf option from redux: https://github.com/reduxjs/redux/blob/1eae9d8ec151d32ddb95bc65893560e913f5da11/docs/advanced/Middleware.md#seven-examples.
   @eps1lon do you know when React batch calls to setState?

In prod it's more optimised but still the raf is faster in my opinion

Do we have a benchmark to compare the two? I think that it should be paramount and come before any other type of discussion. Maybe we can invest time in it later on?

---

@dtassone What do you think about the following to finish #506, I see two possible paths:

a. useRafUpdate is a generic helper, it can't have a leaking side effect. We leave the logic untouched. It's used/can be used in different parts of the component. Instead, we add the RAF batching in the useGridReducer logic.
b. We refactor useRafUpdate so that the RAF timer can be shared for the SAME component, making it hard (⚠️ especially not by default) to be shared between two different components (which would create bugs).

I think that a. is enough and simpler. Thoughts?

[eps1lon]

@eps1lon do you know when React batch calls to setState?

Event handlers only as far as I know.

Could you show me where you think requestAnimationFrame makes a difference? Renders should be pretty fast so I'm curious where you're hitting a bottleneck and why a custom scheduling implementation helps instead of the usual perf tricks.

[oliviertassinari]

Event handlers only as far as I know.

@eps1lon Oh nice! Yeah, I can confirm that in a codesandobx.

[eps1lon]

Do you actually need to batch those though? I'd like to see a case where you hit a bottleneck without batching. Just because getting the scheduling right is incredibly hard (react is working on it for 2 years now) and will be obsolete once concurrent mode lands.

[oliviertassinari]

@eps1lon Maybe we need, seeing React not batching imperative calls made me think that its could be required. One case we have used to stress test the data grid is streaming:

1. Without raf: https://deploy-preview-507--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-streaming--slow-update-grid
2. With raf: https://master--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-streaming--slow-update-grid
3. With raf batched: https://deploy-preview-506--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-streaming--slow-update-grid

In the above, I can't observe any difference in production between the 3. Maybe we need a better test case.
In development, I can definitely see that using raf helps with the logic that disables updates in the grid during scrolling.

[eps1lon]

Maybe we need, seeing React not batching imperative calls made me think that its could be required.

If state updates aren't batched then react has the opportunity (in the future) to schedule them accordingly. React not batching is not inherently a bad thing.

In development, I can definitely see that using raf helps with the logic that disables updates in the grid during scrolling.

Development is slow and not an accurate representation of production performance. It is only really viable to be used with react's profiler.

Could you explain how I can reproduce the bottleneck you observe in development i.e. what command to run, which page to visit, which interaction to trigger and when you observe the bottleneck.