[data grid] Performance: separate row cleanup #12023
Conversation
romgrk
added
the
component: data grid
|
Deploy preview: https://deploy-preview-12023--material-ui-x.netlify.app/ |
| removalList.nodes.at(0)!.start + CLEANUP_ROWS, | ||
| ); | ||
| } else { | ||
| removalList.keep(removalList.nodes.at(0)!.end - CLEANUP_ROWS, removalList.nodes.at(0)!.end); |
There was a problem hiding this comment.
Index should be -1, not 0
packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualization.tsx
Outdated
Show resolved
Hide resolved
| export function useIdleCallback() { | ||
| return useLazyRef(createIdleCallback).current; | ||
| } |
There was a problem hiding this comment.
Unmount cleanup logic?
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
removed
the
PR: out-of-date
|
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 |
romgrk
changed the title
@romgrk When I run #11866 (comment), the benchmark, and UX is noticeably worse on this PR. Not sure it's working 😁 |
|
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) |
|
@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 Screen.Recording.2024-02-26.at.12.46.20.movhttps://deploy-preview-12023--material-ui-x.netlify.app/x/react-data-grid/#pro-plan Screen.Recording.2024-02-26.at.12.46.49.movHere's the profile: https://drive.google.com/file/d/1in_kUcfi9s4_zv06mobVPAPNsXjCzPTT/view?usp=drive_link |
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
removed
the
PR: out-of-date
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
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 There's a few other performance improvements on this PR that I'll pick up separately. |
|
@romgrk maybe worth reporting as crbug? I'm not sure if it's expected that removeChild becomes slower with invalidation tracking enabled |
|
Don't think so. Invalidation doesn't just get the experimental warning:
It gets an additional 2nd warning about their unstability:
|
Stacked on #12013 and #12019 to explore #11866
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
flushSyncpath, and try to pay that cost at a different moment, possibly withrequestIdleCallback.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
RowIntervalList, an interval list data structure that holds row indexes and their render context.colSpancomputations in scroll event, those are very expensive and unnecessary unless there is a.colSpansomewhereposition: absolute; top: ...positioned.