[DataGrid] Improve resize performance #15549
Conversation
|
Deploy preview: https://deploy-preview-15549--material-ui-x.netlify.app/ |
|
On the topic of #12908 – this seems to fix all the remaining issues I have with the mount performance: if (!deepEqual(prevDimensions, newDimensions)) {
setDimensions(newDimensions);
apiRef.current.updateRenderContext?.();
}In: mui-x/packages/x-data-grid/src/hooks/features/dimensions/useGridDimensions.ts Lines 282 to 289 in 8b1d9c1 (definitely much faster page mounts with no visible delays anymore) |
|
I found an even bigger performance issue, but it only affects React 19. Which I understand doesn't have official support yet, but good to keep this in mind. Repro: https://drw2n4.csb.app/ Turn on devtools + highlight on changes + resize browser to see how everything rerenders. Rootprops changes on every render. Just for the mere existance of a ref prop with undefined value. Props is not referentially stable in conjunction with React.forwardRef. I'm not sure if it's a bug or a know issue in how they made ForwardRef backwards compatible, but it has some implications on how Mui itself should offer backwards compatibility, as just keeping ForwardRef around as is may not be a good idea. cc @romgrk, @cherniavskii |
packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx
Outdated
Show resolved
Hide resolved
|
Can you try to appease the CI? You'll want to run |
romgrk
added
performance
component: data grid
That might be something worth reporting to React :| If they made |
I did: facebook/react#31613 I agree, it's an issue. They didn't deprecate it, and it's not behaving the same way either. It was a major pain to actually debug this issue. Even more so, because the ref wasn't actually set, and not visible in the props, so it seemed like props were stable, but memoizations broke anyways. |
|
Hope the CI is happy now. Found out why all the grid header cells render on every resize as well, even if the columns don't need to resize. Every column is passed sectionLength only to calculate whether the right border needs to be applied – shouldn't it be done on the GridHeaderRow level and then passed as a boolean to the header cell instead (cc @cherniavskii)? Also, the style prop seems to trigger a re-render for pinned columns (but haven't debugged what's changing there, because the resulting DOM node is identical) Pinned column: |
|
If sectionLength was replaced by If style prop was replaced from object to pinnedOffset, similar to the GridCell implementation, the re-renderings would be fixed for those as well. Right now pinned GridHeaderItems re-render every time something changes, since a new object is created for the style prop, and |
|
After we applied all the fixes locally, we're getting pretty sweet performance on page navigation as well as resizing now – instant feedback to keyboard/pointer input. Even Safari is fast. Used to be pretty jittery w/ dropped frames. And this is with scroll restoration + charts + images + the 2nd page has 5 datagrids. Spamming enter+escape, no browser history/snapshots involved – UI stress test passed: |
cherniavskii
added
needs cherry-pick
The behavior is the same with React 18, no? |
Nope, in React 18 only rows re-render (which make sense, since they need to be width-aware), but not all cells and everything that depends on rootProps. The repro I posted under react's repo should show the difference between 18 vs. 19. Except for the the Checkbox cell, which I haven't looked into myself yet, but that one seems to re-render in all versions. |
Right 👍🏻
BTW, these repros are private, so I can't access them 😅 |
I can't see any noticeable performance gains with this change. |
Well, it depends on the test case. With a single datagrid where data is immediately available – unlikely. With a page where multiple datagrids (in our case 5) render in parallel and have loading states, autoHeight, etc. – definitely see a difference. I'm not sure it's the root cause of the issue though, but alleviates the symptoms for us at least for now. It could well be that this Might be a good idea to avoid flushSync calls, unless it's in direct reaction to user interaction (e.g. focus management), at least in my experience. Even though it might make the event loop easier in one place, you might unintentionally affect the rendering of other things on the page. Edit: correction, flushSync only affects scroll in this case, was on a wrong path there. |
|
Have to correct myself, the flush sync call doesn't affect mount at all, only scroll. However, removing flushSync does seem to improve scroll performance on Safari: With flush sync: Without flush sync: The only caveat is that it added a barely perceptible flash when restoring scroll. But it might be our implementation that's breaking there. If we run flushSync on first scroll, all is the same, but investigating further. Edit: fixed our scroll restoration. |
|
@romgrk, do you remember what's the rationale of the flushSync call? From my testing:
So, maybe the better solution would be to call flushSync only if the next renderContext skips over some between the next and current renderContext? Just a thought. Side note – this seems duplicated: |
|
My naive implementation, which seems to solve the flashing, yet enables avoiding flushSync when scrolling smoothly: const hasPreviousContext = previousRowContext.current.lastRowIndex !== 0;
const isJumpingOverRows = nextRenderContext.firstRowIndex > previousRowContext.current.lastRowIndex || nextRenderContext.lastRowIndex < previousRowContext.current.firstRowIndex;
const isJumpingOverColumns = nextRenderContext.firstColumnIndex > previousRowContext.current.lastColumnIndex || nextRenderContext.lastColumnIndex < previousRowContext.current.firstColumnIndex;
if (hasPreviousContext && (isJumpingOverRows || isJumpingOverColumns)) {
ReactDOM.flushSync(() => {
updateRenderContext(nextRenderContext);
});
}
else {
updateRenderContext(nextRenderContext);
}Very happy with the performance of this in all browsers. |
|
I haven't looked into I'm happy with your solution, I would also be ok with passing an |
| observer.observe(node); | ||
|
|
||
| if (reactMajor >= 19) { | ||
| return () => { | ||
| mainRef.current = null; | ||
| observer.disconnect(); | ||
| }; | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
Is this really correct? We're not disconnecting on react <= 18?
There was a problem hiding this comment.
Right, ref cleanup is only supported in React 19.
Also, I'm checking the React major because React 18 logs this error: https://github.com/facebook/react/blob/f1338f8080abd1386454a10bbf93d67bfe37ce85/packages/react-reconciler/src/ReactFiberCommitWork.new.js#L292-L295
I can add a cleanup effect for older versions.
There was a problem hiding this comment.
This issue wasn't addressed, I think this might be leaking memory.
There was a problem hiding this comment.
Yeah, I forgot about this one.
I'll open a PR with a cleanup effect.
There was a problem hiding this comment.
Btw I would skip the version checking and use a single strategy, to keep the code simple.
There was a problem hiding this comment.
Verify that no detached nodes are left behind.
How do you verify this?
I see retained detached nodes in Chrome Devtools, even with the resize observer commented out in useGridVirtualScroller:
Screen.Recording.2024-11-25.at.17.47.11.mov
There was a problem hiding this comment.
Try here: https://w8n3kg.csb.app/
Source: https://codesandbox.io/p/sandbox/w8n3kg
Whole Datagrid was a bad example perhaps. There may or may not be a memory leak somewhere else there. I've spent good deal of time trying to debug this in our own app, and I'm not sure at this point. Could be that it's very slow to garbage collect (which I don't understand why – (is it the DOM tree size?) and running a profile too quickly creates a reference to the detached node, which in turn prevents garbage collection). But not sure either if and when it would automatically get garbage collected. Bugs in devotools doesn't make it a joy to debug.
See: #15459 (comment)
There was a problem hiding this comment.
Could be that it's very slow to garbage collect (which I don't understand why – (is it the DOM tree size?) and running a profile too quickly creates a reference to the detached node
I clicked the "Collect garbage" button in the video above, but it didn't change anything.
Bugs in devotools doesn't make it a joy to debug.
Yep, I get different results and I'm not sure it's reliable at this point:
Screen.Recording.2024-11-25.at.18.25.25.mov
But it looks like there are no memory leaks from the resize observer, and the DOM node is garbage collected once removed from the DOM.
I think we shouldn't bother with cleanups here and take a look at #15459 instead
There was a problem hiding this comment.
Are you confident that everything is GC'ed correctly across all browsers? I would tend to add cleanup just to be safe.
There was a problem hiding this comment.
Can never be sure about that, but from quick checks with Safari and Firefox, it seems to be the case. But let's add it to be extra safe.
Probably makes sense to open a new PR for scroll-related things. In the meanwhile, I also found some areas of improvements for the virtual scrollbars.
Probably something along the lines of this would make sense – locking opposing scroll container events, but not blocking itself: const onScrollerScroll = useEventCallback((e) => {
// redacted
if (isScrollerLocked.current) {
isScrollerLocked.current = false;
return;
}
isScrollbarLocked.current = true;
}const onScrollbarScroll = useEventCallback((e) => {
if (isScrollbarLocked.current) {
isScrollbarLocked.current = false;
return;
}
isScrollerLocked.current = true;
}
|
|
Also, it seems that GridRows re-render on every renderContext change due to renderContext prop. |
|
Fixing row memoization almost completely eliminated flashing content as well when scrolling from top to bottom with scrollToIndexes. There's ever so slight flash, almost imperceptible at 120fps that flushSync takes care of, but that's probably just masking another issue that's actually behind the flash. I guess if scrollToIndexes triggered updateRenderContext directly, it would be gone, and there would be no need for flushSync. |
|
Correct, if we'd manually dispatch a scroll event alongside |
|
Cherry-pick PRs will be created targeting branches: v7.x |
Co-authored-by: Andrew Cherniavskyi <andrew@mui.com>
|
@lauri865 Thanks a lot for the detailed exploration, if you want to open PRs for those we'll be happy to see if those improvements don't have any downsides. IIRC, the scroll locking is to prevent the scroller & scrollbar from feeding events into each other. It does ignore events, because they are triggered even when one element is programatically scrolled. |
| import { useRtl } from '@mui/system/RtlProvider'; | ||
| import reactMajor from '@mui/x-internals/reactMajor'; |
There was a problem hiding this comment.
Just noticed this while working on React 19 bump.
@mui/xgrid Was there a reason to create a local utility, when everywhere else it is imported from @mui/internal-test-utils? 🤔
There was a problem hiding this comment.
We needed this in the data grid package itself, not in the test files.
There was a problem hiding this comment.
A possible solution mui/base-ui#1047. We could argue that if we can do import reactMajor from '@mui/x-internals/reactMajor'; then we should remove import { reactMajor } from '@mui/internal-test-utils'; as it can be a footgun.
Co-authored-by: Andrew Cherniavskyi <andrew@mui.com>
We noticed that our sidebar animation lags heavily with every page that have a Datagrid on them, even with pretty generous throttling. The root cause is in how resizing is handled currently within Datagrid. Namely, even though it uses ResizeObserver, it disregards the already cached measurements and triggers new measurements with
getComputedStyle(could be getBoundingClientRect to begin with, since it's only used to get the dimensions). Which triggers a new measurement, and leads to unnecessary trashing. With a smooth animation, the result is... laggy.mui-x/packages/x-data-grid/src/hooks/features/dimensions/useGridDimensions.ts
Line 142 in 8b1d9c1
Instead, we can use the cached contentRect values directly from the resize observer, and trigger a resize event from there.
I also moved the responsibility to publishing the event to
useGridVirtualScroller.tsx, as it's responsible for the mainElementRef. Attaching a resize observer from gridDimensions would have been inefficient and/or needed a breaking change of the resize api method.Since the resize method is no-longer used internally, or even useful for anything (from what I can see), I suggest removing it in the future versions to save some bundle size. Resizing can be triggered by resizing the parent container. Didn't do it right now, as that would have been a breaking change, and I wanted to keep this PR simple.
Note, the ref has a cleanup function that only has an effect from React 19 (but shouldn't cause any issues in earlier versions). It's not strictly necessary as it gets cleared when the node detaches automatically (correct me if I'm wrong), but included it anyways for good hygiene. Didn't add any backward compatible cleaning to save some bundle size and avoid detached logic (would need to add a separate ref for the observe and a useEffect for it).
Our animations are buttery smooth now, even with a page that has many datagrids on it.
Should also address some of the issues in #12908 – a datagrid with variable height (autoHeight or flex parent), would cause unnecessary trashing.