[data grid] Performance: scrolling experiment

[romgrk]

Stacked on #12013 and #12019.

Experiment with scrolling performance.

The idea behind this is that most of our scroll event callback runtime is spent removing DOM nodes, while appending new ones is much less expensive. This is a characteristic of the DOM spec and outside of our control. The graph below shows the amount of time spent adding new rows (green) and removing old rows (red). Other virtualized grids such as AG-grid have the same performance profile. The goal here is to try to remove the red section from the flushSync path, and try to pay that cost at a different moment, possibly with requestIdleCallback.

Before: https://next.mui.com/x/react-data-grid/virtualization/#column-virtualization After: https://deploy-preview-12023--material-ui-x.netlify.app/x/react-data-grid/virtualization/#column-virtualization

Changes

• Rendered rows are stored in a RowIntervalList, an interval list data structure that holds row indexes and their render context.
• Avoid colSpan computations in scroll event, those are very expensive and unnecessary unless there is a .colSpan somewhere
• Rows and detail panels are now position: absolute; top: ... positioned.

[mui-bot]

Deploy preview: https://deploy-preview-12023--material-ui-x.netlify.app/

Generated by :no_entry_sign: dangerJS against f82c480c1a074300fa63b5dc3a62811aa5b27803

[romgrk]

@oliviertassinari I'll reply to this comment here:

Options:

1. I tested if adding a translate3d on each row (rather than on each cell before this PR), helped. It doesn't seem to. We spent less time in the Paint phase, but more in the Layout phase, overall it's more. So doesn't work.
2. What AG Grid and https://github.com/adazzle/react-data-grid seem to do to avoid that paint is to recycle their DOM nodes. We don't, we unmount/remount them in the DOM. I didn't benchmark it, but maybe it's faster to avoid the pain and mutate the DOM attributes than to remove/inject a new DOM node.
3. If 2. speed JavaScript, then it could also be combined with this one: giving each row a position: absolute + top: x, it seems to skip the Paint when removing/adding DOM nodes at the top of the container. I would expect this to speed things up if we can easily set these absolute coordinates.

In this PR I have already done the position: absolute + top: x change. The problem with the previous approach (since sticky) was that we had an inline style (transform: translateY(offsetTop)) set on the row container, and that was causing style invalidation just like setting CSS variables did. With this change the browser can do layout & paint separately as we add & remove rows.

Recycling DOM nodes

I'm not completely convinced by that idea. I've been benchmarking a lot, and as I explain in the top comment what seems to be wasting most of the CPU time is removing DOM nodes. Here is a profile of ag-grid, we can clearly see that most of the time is spent on the removeChild() method. So even though recycling DOM nodes could provide some improvements, I think that we can get much more interesting results by avoiding removing DOM nodes during scrolling.

Kooha-2024-02-11-15-55-27.webm

Preliminary results

I have done the "avoid removing nodes" part of this experiment (but not yet the "remove the nodes when there is idle time" part), I think the results are very promising, check below. I still need to implement the cleanup logic & fix horizontal scroll bugs, but it's going to be the same mechanism as the recording in this comment, so I don't think it should be a problem.

Kooha-2024-02-11-16-03-07.webm

[github-actions[bot]]

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

[romgrk]

I've finished implementing the cleanup logic, here is a quick performance comparison. The new implementation does its cleanup after 1s without user interaction.

Kooha-2024-02-22-23-04-28.webm

[oliviertassinari]

I've finished implementing the cleanup logic, here is a quick performance comparison. The new implementation does its cleanup after 1s without user interaction.

@romgrk When I run https://github.com/mui/mui-x/issues/11866#issuecomment-1963168133, the benchmark, and UX is noticeably worse on this PR. Not sure it's working 😁

[romgrk]

Weird results, this PR makes it smoother for me. Any chance you can send me a recording and a profile of the benchmark? (the "Save profile..." button in the Performance tab)

[cherniavskii]

@romgrk I can reproduce this as well. It gets slower the more you scroll:

https://next--material-ui-x.netlify.app/x/react-data-grid/#pro-plan

https://github.com/mui/mui-x/assets/13808724/4375d31e-d020-49a6-a667-ce2d5ba84faf

---

https://deploy-preview-12023--material-ui-x.netlify.app/x/react-data-grid/#pro-plan

https://github.com/mui/mui-x/assets/13808724/aa527bf7-2c22-4317-9a3c-e02684eecad7

Here's the profile: https://drive.google.com/file/d/1in_kUcfi9s4_zv06mobVPAPNsXjCzPTT/view?usp=drive_link

[github-actions[bot]]

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

[github-actions[bot]]

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

[romgrk]

I WAS WRONG.

Anyway long story short I have a benchmarking chrome user-profile that is usually clean (no react-dev-tools extension or anything else that affects runtime performance) so I can benchmark precisely. Well I had activated the devtools experimental flag Timeline: invalidation tracking a few weeks back for the other performance issues (style recalculation). Turns out it was slowing down considerably the removeChild operation but only in profiling mode, which I had been running in the whole time while benchmarking this PR. So yes, the row cleanup thing is useless.

There's a few other performance improvements on this PR that I'll pick up separately.

[kurtextrem]

@romgrk maybe worth reporting as crbug? I'm not sure if it's expected that removeChild becomes slower with invalidation tracking enabled

[romgrk]

Don't think so. Invalidation doesn't just get the experimental warning:

It gets an additional 2nd warning about their unstability: