[material-ui] Performance: lazy Ripple

[romgrk]

Helps https://github.com/mui/material-ui/issues/35716

Context

I've been working on the grid's performance, and I've noticed that the MaterialUI components we use are causing performance issues during scrolling. The cause of them is the Button's Ripple child, which is mounted in a 2nd render. This triggers re-renders that compete with scroll event handlers, which are already on a tight budget (16ms to respond to scroll events). The graph below shows those re-renders, the 4-5ms stacks that follow the scroll events.

Solution

From the code comments, I can understand that the ripple is rendered in a 2nd render to avoid the server-side render cost of the ripple component. This PoC solves both the server-side and client-side costs by delaying rendering the ripple until it is actually needed.

The following graphs show the flamecharts for this benchmark.

Before	After

Note: the click event runtime cost is irrelevant, the disappearance of the 2nd render is the important part.

Open questions

The code in this PR is a draft, if you can give me the greenlight for this change I'll clean it up. I'm not sure where else the ripple is used, but it would probably be good to use this pattern in all those other uses as well, but I'm not sure if you'd like that to be done in this PR. Let me know what scope this PR should include. I also see that you have a next branch and a mui-material-next folder, I'm not sure how to proceed and what I should edit.

[mui-bot]

Netlify deploy preview

https://deploy-preview-41061--material-ui.netlify.app/

SpeedDial: parsed: +0.88% , gzip: +0.88% @material-ui/core: parsed: +0.13% , gzip: +0.18% CardActionArea: parsed: +1.05% , gzip: +0.90% TabList: parsed: +0.85% , gzip: +0.74% Chip: parsed: +0.90% , gzip: +0.80% TableSortLabel: parsed: +1.01% , gzip: +0.84% Breadcrumbs: parsed: +0.92% , gzip: +0.78% Radio: parsed: +0.95% , gzip: +0.80% StepButton: parsed: +0.93% , gzip: +0.79% @material-ui/lab: parsed: +0.27% , gzip: +0.28% Fab: parsed: +1.03% , gzip: +0.84% AccordionSummary: parsed: +1.03% , gzip: +0.85% Checkbox: parsed: +0.96% , gzip: +0.80% Autocomplete: parsed: +0.52% , gzip: +0.46% ButtonBase: parsed: +1.07% , gzip: +0.86% TabScrollButton: parsed: +0.99% , gzip: +0.79% ToggleButton: parsed: +1.01% , gzip: +0.82% LoadingButton: parsed: +0.88% , gzip: +0.75% BottomNavigationAction: parsed: +1.04% , gzip: +0.84% IconButton: parsed: +1.03% , gzip: +0.83% and 6 more changes

Bundle size report

Details of bundle changes (Toolpad) Details of bundle changes

Generated by :no_entry_sign: dangerJS against fca9fb602d025602d79435cdf49648f3cb7ef2b5

[oliviertassinari]

A side note, the TouchRipple seems to be somehow involved as a "scroll rect" in the Layers:

https://github.com/mui/material-ui/assets/3165635/9583de87-0c81-4dde-8060-447cf590bfa2

I assume we don't need to care about this, but funny.

[DiegoAndai]

This looks good to me, @romgrk 🤗 I think we should move forward with it

Regarding other places this change has to be implemented, this is the only one I can think of: https://github.com/mui/material-ui/blob/master/packages/mui-material-next/src/ButtonBase/ButtonBase.tsx. If I recall correctly, the ripple implementation is slightly different there, as it was updated.

This lazy mounting implementation looks useful 🙌🏼 I wonder if we have something similar somewhere else, either in Material UI or other products 🤔

CC @mnajdova in case you can spot anything I can't regarding the ripple implementation

[romgrk]

I've been trying to find time for this but I have more pressing optimizations to merge on the datagrid. The implementation is complete but the change broke a lot of tests which were all expecting the ripple to be mounted. If someone has interest in this PR feel free to look into those, otherwise I'll come back to it once I have more time.

[Janpot]

In Toolpad we've been doing conditional rendering during SSR/hydration without introducing secondary renders when the component mounts client-side by leveraging useSyncExternalStore. It only introduces a second render after hydration. Not sure if it's an option here as it still has a different performance profile than lazy rendering, but it would remove the second render as well. It does seem to be terser in terms of code and complexity. I believe it should not break the tests, at least not in the same way.

[romgrk]

Lots of focus-visible tests failing, I'll wait for #42467 to be merged to avoid duplicating work.

[romgrk]

The changes are complete and ready for review.