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

[docs-infra] Fix light/dark mode & RTL change speed regression #44534

Open
oliviertassinari opened this issue Nov 24, 2024 · 9 comments · May be fixed by #45360
Open

[docs-infra] Fix light/dark mode & RTL change speed regression #44534

oliviertassinari opened this issue Nov 24, 2024 · 9 comments · May be fixed by #45360
Assignees
Labels
bug 🐛 Something doesn't work performance priority: important This change can make a difference regression A bug, but worse scope: docs-infra Specific to the docs-infra product

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 24, 2024

Steps to reproduce

The light/dark mode and RTL toggle is today x4 slower? than what good enough is at in production, and the RTL/LTR toggle in dev mode is x5-50 slower than what good enough is at? It used to be ok-ish:

v4.12.4 - Dev: mode ~600ms, RTL: ~600ms, Prod: mode and RTL ~150ms

v4 dark

v5.0.6 - Dev: mode ~1,369ms, RTL, ~10,865ms, Prod: mode and RTL ~200ms

v6.1.8 - Dev: mode ~2,253ms, RTL ~ 59,761ms, Prod: mode and RTL ~800ms

Material UI doesn't feel like a credible option for developers with this performance.

Context

Same problem as #41117. Material UI is not usable like this, not truly. I mean, as a developer, this reinforces this story: I would only adopt Material UI for a new React project so that I can then contribute a fix to this. Otherwise, I wouldn't want to touch Material UI, there are better options out there nowadays, and it would only make sense to use the rich components of MUI anytime I can't find anything better.

Your environment

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

cc @romgrk as related to performance on styled engine.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work performance priority: important This change can make a difference status: waiting for maintainer These issues haven't been looked at yet by a maintainer regression A bug, but worse scope: docs-infra Specific to the docs-infra product labels Nov 24, 2024
@romgrk
Copy link
Contributor

romgrk commented Nov 25, 2024

I guess this comes from every style being recomputed, even with a super fast styling engine this would be a noticeable delay. Imo the real solution to this problem would be CSS variables based theming, which I think aligns with the styling engine direction.

@siriwatknp Wdyt?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 25, 2024

I guess this comes from every style being recomputed, even with a super fast styling engine this would be a noticeable delay

@romgrk I agree, if we look at the v4 performance level, it's not excellent. The alternative UI libraries in the space are faster now. So we should aim to be faster than v4.

Now, here when I see v6 vs. v5 vs. v4. I can't help myself thinking: something is broken. What happened? We should be able to revert the changes that led to this outcome.
I suspect that the root cause is that we miss performance regression tests in each PR. If we had, I imagine that the PRs that led to this outcome wouldn't have been merged.

@DiegoAndai DiegoAndai removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 29, 2024
@DiegoAndai
Copy link
Member

DiegoAndai commented Nov 29, 2024

Do we know the root cause for the change? Between Material UI v5 and v6, if emotion is used, not much changed. Or is this mainly a docs-infra issue?

@DiegoAndai DiegoAndai moved this to Selected in Material UI Nov 29, 2024
@DiegoAndai DiegoAndai removed this from Material UI Nov 29, 2024
@romgrk
Copy link
Contributor

romgrk commented Nov 30, 2024

No idea about the cause. The docs pages are different though, so the system styling code isn't the only variable, we should test with a consistent test case.

I'd love to look into it but I'm busy with the design-agnostic datagrid refactor, I could come back to this next month.

@DiegoAndai DiegoAndai added this to the Material UI v7 milestone Jan 28, 2025
@DiegoAndai DiegoAndai self-assigned this Jan 28, 2025
@DiegoAndai DiegoAndai moved this to Backlog in Material UI Jan 28, 2025
@mnajdova mnajdova assigned mnajdova and unassigned DiegoAndai Feb 19, 2025
@mnajdova
Copy link
Member

I am starting to look into this. On dev mode, on the first Right to left switch it takes very long (I am testing on the current master - v7), I even got a page not responsive on one try. Consecutive switching are faster, but still slow. I will create an isolated example to see if the issue is the docs or the styled engine.

@DiegoAndai DiegoAndai moved this from Backlog to In progress in Material UI Feb 19, 2025
@mnajdova
Copy link
Member

mnajdova commented Feb 19, 2025

The CacheProvider from Emotion takes up most of the time when switching RTL on the docs, around 70%. I will try to reproduce in isolation to see what may be the issue. It may be something related to the docs set up, as on an isolated page it works fast.

@siriwatknp
Copy link
Member

siriwatknp commented Feb 25, 2025

For mode improvement, there are 2 PRs:

  1. Add option to preserve theme values between modes [system] Disable theme recalculation as default behavior #45405
  2. Restructure the theme (remove root ThemeProvider from _app.js) to CSS variables [docs-infra] Restructure docs theme context to CSS variables #45386

After tested with the above PRs, it's 2x faster on my dev server:

Before: 309-360ms

Image

After: 158-169ms

Image

@siriwatknp
Copy link
Member

siriwatknp commented Feb 25, 2025

I don't think RTL can be improved that much.
This is because it requires a theme change (causing React to rerender every components) and Emotion reinsert the stylesheet (e.g. from margin-left to margin-right).

@mnajdova
Copy link
Member

I tried to test rtl isolated here: https://mui-rtl-v7.vercel.app/, it's start to get noticed after rendering 1000 card components. There may be ways to improve rtl too but it will require changes. Some conceptual ideas may include:

  • generate rtl specific styles upfront, render then under the dir="rtl" selector
  • don't store rtl in context if possible - not sure if we can do this, e.g. for Base UI we were investigating this, but in the end had to create a DirectionProvider

However, there may be slight improvements we can do on the docs in regards to this, there is a BIG difference between component pages vs other docs pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work performance priority: important This change can make a difference regression A bug, but worse scope: docs-infra Specific to the docs-infra product
Projects
Status: In progress
Development

Successfully merging a pull request may close this issue.

5 participants