[DataGrid] Performance: avoid style invalidation by romgrk · Pull Request #12019 · mui/mui-x

romgrk · 2024-02-10T10:06:43Z

Closes #11866
Follow-up of #10059 (comment)

Avoid style invalidation by avoiding changing the CSS variables on the root container during scrolling.

Before:

After:

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

See https://www.youtube.com/watch?v=WRiOWJZoKlw

mui-bot · 2024-02-10T10:11:07Z

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

Generated by 🚫 dangerJS against cda4e3c

oliviertassinari

Avoid style invalidation by avoiding changing the CSS variables on the root container during scrolling.

Oh cool, so it's the same root cause as in https://emilkowal.ski/ui/building-a-drawer-component. CSS variables work well for static values but it's unusable for CSS variables who changes at 120 Hz (macOS screen refresh rate) with a lot of DOM descendants.

Regarding the benchmark, what I can measure on this PR is a reduction of about 70ms spent on Rendering for every 1,000 ms of scrolling but an increase of about 80ms spent on JavaScript.

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

After #10059 (add position: sticky) https://deploy-preview-10059--material-ui-x.netlify.app/x/react-data-grid/#pro-plan

After #11924 (solve Layerize) https://deploy-preview-11924--material-ui-x.netlify.app/x/react-data-grid/#pro-plan

This PR (solve CSS variable cascading) https://deploy-preview-12019--material-ui-x.netlify.app/x/react-data-grid/#pro-plan

It looks like we still have to optimize this JavaScript rerendering, and we might even be faster than before 🚀

The offsets prop leaks as a DOM attribute

romgrk · 2024-02-11T19:01:17Z

It looks like we still have to optimize this JavaScript rerendering, and we might even be faster than before 🚀

Yeah the offsets prop is an object and it's breaking react.memo in a few places, I got a commit fixing it in #12023 but it depends on this PR plus #12019, so it should be fixed shortly once we get these merged. I'm trying to make small PRs, the team has suffered enough with #10059.

romgrk · 2024-02-11T19:05:10Z

packages/x-data-grid/src/hooks/features/columnHeaders/useGridColumnHeaders.tsx

-export const GridColumnHeaderRow = styled('div', {
-  name: 'MuiDataGrid',
-  slot: 'ColumnHeaderRow',
-  overridesResolver: (props, styles) => styles.columnHeaderRow,
-})<{ ownerState: { params?: GetHeadersParams; leftOverflow?: number } }>(


This test breaks if this slot doesn't have an ownerState prop, and I don't understand why. Removing the slot stuff made the test pass. Passing an empty ownerState={{}} also made it pass, but didn't felt clean.

Did you manage to fix it? All tests pass

Well I removed the slot stuff, so yes but the slot ColumnHeaderRow doesn't exist now. Is that ok?

Also why is that test failing? Isn't that override targetting the root slot of the datagrid?

I have restored the ownerState thing.

cherniavskii · 2024-02-12T18:47:26Z

packages/x-data-grid/src/hooks/features/virtualization/gridVirtualizationSelectors.ts

+ * @category Virtualization
+ * @ignore - do not document.
+ */
+export const gridOffsetsSelector = createSelector(


If it's meant to be internal only, we shouldn't export it from the package.

I have commits to evolve this a bit and get rid of it, in #12023. I'll adjust in a follow-up PR if that's fine.

I think it should be fine to remove it in a follow-up since it is not documented anyways.

cherniavskii · 2024-02-12T19:12:12Z

packages/x-data-grid/src/hooks/features/columnHeaders/useGridColumnHeaders.tsx

-export const GridColumnHeaderRow = styled('div', {
-  name: 'MuiDataGrid',
-  slot: 'ColumnHeaderRow',
-  overridesResolver: (props, styles) => styles.columnHeaderRow,
-})<{ ownerState: { params?: GetHeadersParams; leftOverflow?: number } }>(


Did you manage to fix it? All tests pass

MBilalShafi

Nice 👍

MBilalShafi · 2024-02-15T12:00:54Z

packages/x-data-grid/src/components/GridRow.tsx

    >
      {leftCells}
-      <div className={gridClasses.cellOffsetLeft} role="presentation" />
+      <div


Could you mention the steps you are doing to test it, in a few of my performance snapshots I could see a noticeable reduction in the recalculate styles portion, but it regresses a bit the layout part of the rendering.
I took into account those style revalidations which came right after the scroll event.

Here are the average results from a few tests on the M1 Pro and Chrome Version 121.0.6167.184

Before: (on https://next.mui.com/x/react-data-grid/#pro-plan)
Recalculate style: 4.95ms
Layout: 0.91ms
Pre-paint: 0.91
Paint: 1.14ms

After: (on https://deploy-preview-12019--material-ui-x.netlify.app/x/react-data-grid/#pro-plan)
Recalculate style: 2.91ms
Layout: 7.75ms
Pre-paint: 0.90
Paint: 0.80ms

Am I testing it incorrectly?

Bdw, on 4x CPU slowdown, it appears to improve the layout phase as well (almost 40% improvement), so overall it seems improved, especially on low-end devices. I am all in for pushing for this improvement since the overall performance (summary) is better in all the tests I did!

But if you can guess the possible reason for the increase in the layout calculation time without throttling, that'd add to my knowledge.

I run scroller.scrollTo({ top: 100_000, behavior: 'smooth' }) and inspect one of the cycles in the middle of the graph. I'm not sure exactly, but it could be fixed in #12023. Using an offsetTop on the row container in sticky was a mistake, it triggers style invalidation, and it might be triggering layout invalidation.

oliviertassinari · 2024-02-19T18:00:18Z

One more step in the right direction 🙏

romgrk added 2 commits February 10, 2024 04:59

fix: style invalidation

6331a57

lint

c7a7dd8

romgrk added the scope: data grid Changes related to the data grid. label Feb 10, 2024

romgrk mentioned this pull request Feb 10, 2024

[DataGrid] Sticky headers #10059

Merged

9 tasks

romgrk added 8 commits February 10, 2024 05:46

lint

3eff377

fix: pinned border

c5dc461

fix: empty columns bug

da2c2f1

fix: empty rows bug

f1a819f

fix: layout ownerState test failing

a821c66

test: fix offset top

380995c

docs: api

2b3c933

lint

1a03542

romgrk requested review from MBilalShafi and cherniavskii February 10, 2024 18:10

oliviertassinari changed the title ~~[DataGrid] Performance: avoid style invalidation~~ [data grid] Performance: avoid style invalidation Feb 10, 2024

oliviertassinari added the performance label Feb 10, 2024

romgrk mentioned this pull request Feb 10, 2024

[data grid] Performance: separate row cleanup #12023

Closed

oliviertassinari reviewed Feb 11, 2024

View reviewed changes

romgrk commented Feb 11, 2024

View reviewed changes

lint

f338d47

cherniavskii approved these changes Feb 12, 2024

View reviewed changes

MBilalShafi approved these changes Feb 15, 2024

View reviewed changes

MBilalShafi changed the title ~~[data grid] Performance: avoid style invalidation~~ [DataGrid] Performance: avoid style invalidation Feb 15, 2024

romgrk added 3 commits February 15, 2024 11:37

refactor: no breaking change

132e539

lint

a4b77f6

fix: ownerState

29bd207

romgrk enabled auto-merge (squash) February 15, 2024 16:57

lint

cda4e3c

romgrk merged commit 4613e48 into mui:next Feb 15, 2024

romgrk deleted the perf-style-invalidation branch February 15, 2024 17:58

thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024

[DataGrid] Performance: avoid style invalidation (mui#12019)

d8eef5a

romgrk mentioned this pull request Jan 31, 2025

[DataGrid] Avoid <GridRoot /> double-render pass on mount in SPA mode #15648

Merged

1 task

Uh oh!

Conversation

romgrk commented Feb 10, 2024 • edited by oliviertassinari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mui-bot commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romgrk commented Feb 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MBilalShafi Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MBilalShafi left a comment

Choose a reason for hiding this comment

Uh oh!

MBilalShafi Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari commented Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

romgrk commented Feb 10, 2024 •

edited by oliviertassinari

Loading

mui-bot commented Feb 10, 2024 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

MBilalShafi Feb 15, 2024 •

edited

Loading

MBilalShafi Feb 15, 2024 •

edited

Loading

oliviertassinari commented Feb 19, 2024 •

edited

Loading