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

[data grid] Improve vertical scrolling performance #11924

Merged
merged 7 commits into from
Feb 8, 2024

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Feb 4, 2024

Follow-up of #10059 (comment)
One iteration on #11866

Improve the vertical scrolling performance. This PR removes the transform: translate(...) style on cells

SCR-20240330-rpsq

and use an offset filler cell instead. It fixes a rendering bottleneck with the "Layerize" rendering step:

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

@romgrk romgrk added the component: data grid This is the name of the generic UI component, not the React module! label Feb 4, 2024
@mui-bot
Copy link

mui-bot commented Feb 4, 2024

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

Generated by 🚫 dangerJS against 35ff0de

@romgrk romgrk marked this pull request as draft February 4, 2024 23:43
@romgrk
Copy link
Contributor Author

romgrk commented Feb 5, 2024

Follow-up of #10059 (comment)

Once this style removed, the computation bottleneck seems to then move to recalculateStyles, this is very odd to me:

More in context: I don't know why. I ran out of time I can allocate to this 😄

I've inspected a bit the "recalculate styles" phase, it's triggered by the .removeChild to remove rows that are scrolled out of the viewport. Leaving the rows in (while still performing the rest of the virtualization logic) yields dramatic improvements:

image

But this seems fairly logical to me: because the rows that are removed are the first childs of their container (these tests were all scrolling down), the browser invalidates the layout of all the siblings that come after. In other words, all the rows are invalidated because they all come after the first rows, which are removed. I'll try to figure if we can find a way to have the browser avoid redoing the style/layout for all those elements.

@romgrk romgrk mentioned this pull request Feb 5, 2024
9 tasks
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2024
Copy link

github-actions bot commented Feb 7, 2024

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

1 similar comment
Copy link

github-actions bot commented Feb 7, 2024

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2024
@romgrk
Copy link
Contributor Author

romgrk commented Feb 8, 2024

I have reduced the scope of this PR, I'll just do the removal of of the transform style on the cells, which dimishes the number of layers and the Layerize phase in the browser rendering.

before after
image image

The style recalculation phase is still taking too long, because the number of invalidated elements is higher than before. I still need to experiment more to understand how to reduce that cost. From this exploration I have however learned that removing nodes from the DOM is actually very expensive: about 60-80% of the virtualization DOM changes cost comes just from removeChild() calls. I have benchmarked ag-grid, and they also have a similar performance profile. They do have a much smaller "Recalculate styles" phase though, which might have to do with the fact that the use transform to position the rows, which could allow for style invalidation to not touch row siblings (aka all the rows). But their styles are also much cleaner, they use flat class selectors everywhere, which also facilitate style invalidation for browsers.

Anyway, this is ready for review.

@romgrk romgrk marked this pull request as ready for review February 8, 2024 10:15
Comment on lines 785 to 790
{
"key": "cellOffsetLeft",
"className": "MuiDataGridPremium-cellOffsetLeft",
"description": "Styles applied to the left offset cell element.",
"isGlobal": false
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to @ignore - Do not document this one but it didn't work. How can I do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it does not impact the classes.
I'll submit a PR to the core repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romgrk romgrk merged commit 907646b into mui:next Feb 8, 2024
17 checks passed
@romgrk romgrk deleted the perf-vertical-scrolling branch February 8, 2024 15:50
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 11, 2024

Great, a nice step forward 👍. From what I can benchmark, this makes us not too far from where we were before #10059. I see a reduction of about 100ms spent in "Rendering", for every 1,000ms of rendering.

@oliviertassinari oliviertassinari changed the title [DataGrid] Vertical scrolling performance [DataGrid] Improve vertical scrolling performance Feb 11, 2024
@oliviertassinari oliviertassinari changed the title [DataGrid] Improve vertical scrolling performance [data grid] Improve vertical scrolling performance Feb 11, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 11, 2024

I was curious, since this change, when we enable

we see the whole grid container light up 💡:

Screen.Recording.2024-02-11.at.20.42.50.mov

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.

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

Successfully merging this pull request may close these issues.

None yet

5 participants