-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[data grid] Scroll performance regression since v7.18.0 (probably) #15168
Comments
@MBilalShafi, could it be due to the row spanning feature? We don't have it enabled, but could potentially open up new code paths regardless. Just based on quickly scanning the latest updates and commits around the affected version; sorry if I'm on the wrong path. Edit: Another way the issue shows itself on all browsers is the scroll performance when hovering over the scrollbar – then even on Chrome it becomes extremely choppy. |
We really need you to provide a reproduction and to fill the |
I completely understand. However, we spent the past few days debugging why our app had become sluggish recently, and narrowed it down to the Datagrid (after spending a lot of time optimising our own components to no end). And we narrowed it down to even to a specific version – so I did not open this on a whim. One cause was #15158, but it turned out we had to migrate even further back to restore a good level of performance, especially in Safari. I wish there was a common ground kitchen sink example with measurements in place that would facilitate these kinds of reports and save time for everyone involed, as it does come down to actually measuring and comparing performance, rather than eyeballing it. And that takes extra time of course, and even then it's difficult to compare performance across the history of versions if all the developers have to concoct their own repros. Anyways, here's the repro we have for now:
But make sure to check it in full screen as the preview window can barely fit any columns:
Video comparing the two affected versions in Safari 18.1 (if anything, I scrolled faster in the older version, yet the performance is all around better): Again, it's not only Safari that is affected from our testing, but it became borderline unusable in Safari, compared to e.g. Chrome where it resulted in perhaps a few dropped frames here or there, but still usable performance. I believe the performance impact is not limited to scrolling, but also mounting, as our page transitions (between pages tht both have datagrids) sped up quite significantly when migrating back to the older version. Environment (the repro is using React 18 and the problem exists there as well):
|
On Chrome there is a ~10 FPS difference on average between the two versions, but in certain cases we've seen much worse (I set the WAIT_DURATION to 1ms, I think if we'd ramp it up a bit more, it gets worse): |
could you maybe replace the avatar cell with something that does not include a network request? You could use a chart with random values instead. |
I added a simple div instead. The problem persists. But in general, I strongly believe that a performance-centric demo should include images + network requests. It's the most basic requirement, and something that can affect the baseline performance. Especially in my most hated browser (Safari). |
That's true ... I just wanted to rule that out. Thanks for the update. |
No, a performance reproduction case should be as minimal as possible. If the performance issue really comes from the datagrid, it should be possible to reproduce it without other code. You should instead use the devtools CPU slowdown to showcase a performance regression instead of adding unrelated elements. The problem with unrelated elements is that they add tons of noise to stack traces, which makes it harder to read what's going on. I'm not sure if I can reproduce a difference by scrolling manually. I tried to reproduce a difference by automatically scrolling (
I'm a bit short on bandwidth to look into this more in details, and using Safari might have an impact that I can't test. @MBilalShafi could you look into this one? |
This is surprising, if it also impacts mounting then the root cause could be very different. @lauri865 Could you get us a reproducible case of that? Investigating a mounting issue is considerably easier than a scrolling issue, due to how difficult it is to simulate scrolling in an easily reproducible manner. Also please don't use the |
I agree when it comes to honing in on a specific issue. But my point was more about a generic performance testbed – if you make sure that it includes all real-world requirements (=images for one, and maybe slightly more complex images than flags I might add), it's less likely that you let performance regressions slip in. You can get away with just rendering text for much longer of course, while adding image rendering + other things can break things apart completely, and showcase any changes (positive or negative) to the performance much earlier. @romgrk, the screenshots look rather narrow – did you make it narrow only for the screenshots? It makes a big difference how many columns you render. But Safari is the core issue here in any case. And Chrome admittedly doesn't show it as much in the repro, but it's definitely more pronounced in our real application – but still something we could live with. Safari on the other hand – our users are angry. |
After removing images, the Chrome repro doesn't really show as significant difference indeed (but to avoid any doubt, Safari is still bad). Which also illustrates my point a bit (if nothing else changed than a package version, yet images affect performance in one version but not the other, it's unlikely caused by the images). Performance issues in the browser happen when you overspend the CPU-time budget you have to render a frame. Very few things when it comes to rendering can only be tested in isolation, as there are often cascading effects all round. If it's a single slow component that's the cause, sure, maybe. Slow CSS for example, may not even register as an issue with few elements, but quickly becomes an issue depending on how many elements there are or how quickly they change in the DOM. |
I was able to reproduce the reduced performance in Safari, on debugging it did seem to relate to the row spanning hook, but weirdly so this selector turned out to be the culprit. Commenting out everything in the hook but this line doesn't solve the issue. I didn't dig yet into the virtualization logic, just did an experiment to remove the mentioned selector and it made the performance similar to the v7.17.0 example above for me. @lauri865 Could you confirm the finding by cloning and running the dev server in this repo. I used the package generated by the experiment PR in it. (Ideally this should've been tested in a generated codesandbox but Codesandbox CI is having some issues atm). The Safari performance timeline doesn't yield any particularly useful information, I will reiterate this with adding some throttling. |
@MBilalShafi, it certainly looks better, but when I measure, it seems like the main thread is still doing more work compared to this: https://react-vodudv-zq3jym.stackblitz.io/ Maybe you can double-check how it looks on your end? (I measured only because it still felt more sluggish when scrolling fast) |
When I do: Repo above: most expensive scroll event is 144ms Consistently 40-50% worse on my end. Edit: |
Thanks for sharing this finding, while debugging I was trying to reproduce a similar performance hump on the column pinning because it also coincidently uses the This finding connects with the exploration I did, it seems the issue has something to do with the virtualization. I'll explore this further to find the exact cause. |
@MBilalShafi, good news, I found a solution for the jumpy pinned columns:
Should target only safari with this though, and ideally only if there are pinned columns + vertical scrollbar. I think the problem is that safari creates a new rendering context when using sticky positioning, and hence the content can show up before the columns. There might be other solutions to this as well, but that would probably entail changing how to scroll container is rendered, which I doubt anyone wants do for only Safari ;) As for the other issues, we're still stuck at v7.17.0. |
Seems like v7.16.0 also negatively affected performance on Safari. Almost no banding even in heavy datagrids on v7.15.0 |
This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Note @lauri865 How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey. |
Steps to reproduce
Difficult to provide a reproduction right now. It's especially pronounced in one of hour heaviest datagrids where we draw a sparkline in a cell (on a canvas). In Chrome there are occasional dropped frames, but at least it's usable still, however, on Safari, CPU is completely clogged (=maxed out) and there's severe banding and delays in rendering, which completely breaks the UX.
Performance in v7.15.0 (but seeing similar performance in v7.17.0 as well)
https://github.com/user-attachments/assets/9f06d8f8-f0cc-4213-9f8d-97152fc032b7
Performance in v7.18.0:
https://github.com/user-attachments/assets/cf3f3163-0413-4e57-a36e-0742ce0d2f22
Especially bad frame from the last video (nothing even remotely close to that in the first video):
Same browser (Safari v18.1), same datagrid, only difference is the
mui/x-data-grid
version.If I would go crazy with scrolling on v7.18.0, my screen would go blank. The overall picture looks even worse (half of the grid looks blank at times) than the single column, but I cannot share that view due to the nature of the data displayed.
There might be small differences in performance between v7.15.0 and v.7.17.0 as well, but that would require more measurements, as I'm so far just eyeballing it. But completely usable still.
We decided to downgrade to v7.15.0 for now just to be safe, as anything later than v17.8.0 is unusable for us.
Current behavior
Janky scroll, dropped frames.
Expected behavior
Smooth scroll, no dropped frames.
Context
No response
Your environment
npx @mui/envinfo
Search keywords: scroll, performance, safari
The text was updated successfully, but these errors were encountered: