Skip to content
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

[Datagrid] Poor mount performance on Safari #12908

Open
lauri865 opened this issue Apr 25, 2024 · 3 comments
Open

[Datagrid] Poor mount performance on Safari #12908

lauri865 opened this issue Apr 25, 2024 · 3 comments
Labels
component: data grid This is the name of the generic UI component, not the React module! performance

Comments

@lauri865
Copy link
Contributor

lauri865 commented Apr 25, 2024

Steps to reproduce

There's been amazing performance improvements in rendering the V7 of Datagrid, huge kudos to @romgrk et al. One area that could still be optimised is initial rendering/mounting performance, which seems especially poor in Safari (unfortunately we have to support that).

Link to live example: (required)

Steps:

  1. Open e.g. this page in Safari: https://mui.com/x/react-data-grid/editing/
  2. Open Devtools -> timeline
  3. Referesh the page

I'm seeing 85%+, sometimes 100%+ CPU usage on page load. As a benchmark, the demo site of AG-grid only peaks at around 30% for a short time. https://ag-grid.com/example/

Current behavior

CPU gets choked by (most likely) grid measurements.

Example output – not sure if it's the culprit or just a result of choked CPU, but seeing measureScrollbarSize called many times on mount:
Screenshot 2024-04-25 at 16 46 32

Screenshot 2024-04-25 at 16 56 34

Gets especially bad when a page has multiple datagrids. We have a page with 5 grids, and while in Chrome the performance is completely acceptable, in Safari, there's a visual delay between page navigations. Removing the grids makes everything super snappy.

Expected behavior

CPU not getting choked on mounting.

Context

No response

Your environment

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Search keywords: safari, performance,slow

@lauri865 lauri865 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Apr 25, 2024
@romgrk
Copy link
Contributor

romgrk commented Apr 25, 2024

That function is only called at a single site, seems like it could be an excessive number of updateDimensions being triggered during mount.

I'd take it but I'm on linux, maybe @cherniavskii or @MBilalShafi?

@romgrk romgrk added performance component: data grid This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 25, 2024
@romgrk
Copy link
Contributor

romgrk commented Apr 25, 2024

If you copy the measureScrollbarSize function and set the prop scrollbarSize it could maybe improve performance easily on your side:

// calculate once
const scrollbarSize = measureScrollbarSize(document.body, -1, undefined)

function Component() {
  return <DataGrid scrollbarSize={scrollbarSize} />
}

The reason why measureScrollbarSize is not cached is because we can't know if the user is setting their grid scrollbars to different sizes, but you should be able to make that assumption.

We're doing a DOM write+read in the measure function which is quite expensive, it would be nice to find a solution.

@lauri865
Copy link
Contributor Author

Amazing, thanks a lot @romgrk. Definitely visibly better.

There's still somewhat of a performance gap with Safari (vs. Chrome) though, but at least it doesn't look like our client-rendered pages are fetched from the server anymore ;). I think Chrome is better at optimizing layout measurements/trashing, so you can get away with less optimised code / calling offsetWidth, etc. more frequently without incurring any performance penalty, so maybe there's more room to optimise around updateDimensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! performance
Projects
None yet
Development

No branches or pull requests

2 participants