mui-x icon indicating copy to clipboard operation
mui-x copied to clipboard

[DataGrid] Vertical scrolling performance

Open romgrk opened this issue 1 year ago • 4 comments

Follow-up of https://github.com/mui/mui-x/pull/10059#issuecomment-1904425888

Restore the vertical scrolling performance.

Changes:

  • Remove the transform: translate(...) style on cells and use an offset filler cell instead
  • Remove the virtual scroller render zone DOM node, keep only the virtual scroller content and rename it to "centerContainer" to match the "topContainer" and "bottomContainer"

romgrk avatar Feb 04 '24 10:02 romgrk

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

Generated by :no_entry_sign: dangerJS against 35ff0de049e4744b348efaf3244f333691c7cf2b

mui-bot avatar Feb 04 '24 10:02 mui-bot

Follow-up of https://github.com/mui/mui-x/pull/10059#issuecomment-1904389411

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 avatar Feb 05 '24 02:02 romgrk

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

github-actions[bot] avatar Feb 07 '24 10:02 github-actions[bot]

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

github-actions[bot] avatar Feb 07 '24 10:02 github-actions[bot]

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 avatar Feb 08 '24 10:02 romgrk

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 avatar Feb 11 '24 00:02 oliviertassinari

I was curious, since this change, when we enable

we see the whole grid container light up 💡:

https://github.com/mui/mui-x/assets/3165635/18506f75-2600-4b46-90a6-20cc8d7382fc

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.

oliviertassinari avatar Feb 11 '24 19:02 oliviertassinari