[system] Pre-serialize & cache styles to improve performance

[romgrk]

This PR uses emotion's serializer to pre-serialize stable styles and re-use them on every render, rather than have emotion pass them through createStringFromObject every time.

TestCase

Results:

Before: 101.6ms	+- 9.7
After:  82.2ms	+- 4.5

N = 100

Fix #43440

https://deploy-preview-43412--material-ui.netlify.app/performance/table-mui/

[romgrk]

I have removed the various attachTheme calls in favor of adding a style function that runs once and attaches the theme directly on the props. It relies on running before anything else.

[siriwatknp]

Is my understanding is correct that this PR only tackles Emotion, not included styled-components.

[romgrk]

Yes, I tried looking a bit into it but I'm not familiar with their codebase, not sure if the same thing is possible. They need to expose their serialization code and they need to accept pre-serialized objects as input for this optimization to make sense.

[mnajdova]

I don't think it's important to fix styled-components's usage it's less than 10% of the usage: https://npm-stat.com/charts.html?package=%40mui%2Fstyled-engine&package=%40mui%2Fstyled-engine-sc&from=2023-08-29&to=2024-08-29

[romgrk]

Isn't this mutating tag though?

[romgrk]

@siriwatknp Maybe you could confirm here, I think this line is mutating tag's style processors, but what we want is instead to skip adding the sx processor to the new styled component. Is that right?

[romgrk]

Regardless of the question above, I think I'll rename this function to internal_mutateStyles to better express what it's doing.

[romgrk]

I think this could be an (existing) issue/bug but I don't have the bandwidth to investigate it and I'm afraid of fixing it and possibly changing the behavior, so I've just renamed internal_processStyles to internal_mutateStyles to better highlight what is happening here.

[mnajdova]

I think we introduced this as an perf improvement as we were processing sx multiple times if we had a styled component invoked over another styled component. This is what I remember about it. Likely @siriwatknp could have more info, but +1 on not changing any behavior as part of this PR.

[siriwatknp]

Yes, @mnajdova is right.

[oliviertassinari]

It looks like it shouldn't even be exported #43440

Suggested change

export default memoTheme;

[romgrk]

I was worried about the typings in the initial PR (memoTheme<T>), having it here made it easy to include the MaterialTheme, though material doesn't use much TS from what I can see. I could remove it altogether from material but it would make it less accessible to external users in case they want it.

[oliviertassinari]

I want us to move to a point where we force developers to provide their theme (so we can remove all @mui/system re-exports from @mui/material, so we need a great DX to not rely on a default theme, so this seems OK.

[romgrk]

Ok. So should I remove this file and import unstable_memoTheme from @mui/system everywhere?

[oliviertassinari]

I think this is beyond the scope of this PR #43440.

[mnajdova]

Btw, I added the export from @mui/material because I wanted to use it in the lab, for the LoadingButton.

[mui-bot]

Netlify deploy preview

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

PigmentGrid: parsed: +10.25% , gzip: +8.06%
@material-ui/system: parsed: +0.57% , gzip: +0.46%
@mui/joy/DialogTitle: parsed: +0.41% , gzip: +0.47%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 79fd545

[romgrk]

This prevents the internal_processStyles refactor and makes the build fail. It's annoying that it's in a separate repo and depends on internal details:

https://github.com/search?q=repo%3Amui%2Fpigment-css%20internal_processStyles&type=code

Also weird that the build succeed in one of the previous commits, it should have been failing from the start.

[romgrk]

I could either patch this in this repo with something like:

import * as sc from '@mui/styled-engine'
const serialize = sc.internal_serializeStyles ?? x => x
// and ignore the internal_processStyle => internal_mutateStyles refactor

Or I could open a PR on pigment and do the changes and wait for it before releasing this one.

[brijeshb42]

The fix can be localized to this repo. Let me check.

[oliviertassinari]

The approach of emotion shouldn't be able to compete with the approach of linaria, but I probably need to have a pass on PigmentCSS. We always author code with an FP style, it can hurt performance in hot code sections like this one.

@romgrk True, but it will be interesting to compare both against Tailwind CSS and CSS Modules. We have to consider that the bundler integration is a friction point for people, e.g. https://x.com/hyeumair/status/1828669441544261970. So I think looking at the performance data, and the cost to make each work will give us a sense of what's most pragmatic.

Now, looking at this PR, when I run https://github.com/mui/material-ui/tree/master/benchmark/browser, I don't see much difference.

When I compare those, the win is light, if not a bit slower:
Before: https://mui.com/performance/table-mui/

After: (the big chunk first is preprocessStyles) https://deploy-preview-43412--material-ui.netlify.app/performance/table-mui/

Overall, in the test cases,

React components:
  54 ±1%
Styled MUI:
  99 ±4%
Styled emotion:
  83 ±5%
Styled SC:
  76 ±3%
makeStyles:
  82 ±4%

it suggests that Pigment CSS can be x2 faster than Emotion.

[romgrk]

So the improvement in this PR is that in a case like this:

<Button />
<Button />
<Button />
<Button />
<Button />

Before this PR, the Button styles object would be passed 5 times through emotion's serializeStyles function to turn it into a CSS style string. And before the memoTheme refactor, it would have also been re-created 5 times, but memoTheme prevents that now. So what this PR does is save time on repeated usages of the same component. If the test case was something like this instead:

<Chip />
<Badge />
<Button />
<Switch />
<Checkbox />

There would be no performance improvement, because there is no work that can be re-used. But on a real-world use-case, it's safe to assume that components will be used multiple times and this will be saving some work.

I haven't looked at the implementation of the benchmarks you've linked, but for the "benchmark/browser" one seems to be testing Box & Grid mainly. Box seems to have no default styles that would be affected by this PR, and Grid doesn't have a stable style object because it accesses ownerState instead of using variants, and no stable style means no work re-use across usages of the component; memoTheme is a prerequisite for this to work. I have another branch where I go try to fix problematic components like Grid individually but it's based on this one so I haven't opened the PR yet, the diff would be too wild.

And for the table benchmark, it might not have big enough style objects for this to matter (e.g. something like { color: 'red' } costs nearly nothing to turn into a string, so it's normal to see no significant impact for this PR), and the improvement could be drowned in the CPU noise that's always present on a personal device (unlike something more controlled like a server). When I run a benchmark I include many runs (100 in the results in the top-comment) to ensure statistical outliers haven't distorted the results too much.

Those reasons are why I've been benchmarking/profiling using either the test case I linked in the top-comment or the dashboard template, those are more likely to be representative of what happens for actual users. Relevant sindresorhus blog post.

it suggests that Pigment CSS can be x2 faster than Emotion.

Maybe, but imho we'd need to get rid of the styled() wrapper like I previously proposed here. But with all the work I did recently now that I understand the codebase I should be able to do that exploration fairly quickly with a few components to validate if it's impactful enough.

[oliviertassinari]

So what this PR does is save time on repeated usages of the same component

@romgrk This sounds like the same as what I/Oleg implemented in the past: https://github.com/mui/material-ui/blob/v4.x/packages/material-ui-styles/src/makeStyles/makeStyles.js#L77 with makeStyles. It helped a lot 👍. We were using a ref counter, but the problem then became that React 18 started to be unreliable when doing this. We were no longer able to remove <style> element. But then, on the other hand, emotion just doesn't care, they almost keep everything in the DOM (they still have to unmount global styles). So maybe we could have just ignored that problem and we could have made makeStyles work in React 18. It's not great though, the larger the CSSOM the slower rendering becomes.

I haven't looked at the implementation of the benchmarks you've linked

Source here: https://github.com/mui/material-ui/blob/master/docs/pages/performance/table-mui.js.

those are more likely to be representative of what happens for actual users

I think that this specific benchmark is representative to real case. One of the complaints I heard a lot about in the past was how slow the Table was, meaning how early people had to add pagination or virtualization to keep the rendering time bearable. It's like in #41330: it was fine not to use virtualization in Material UI v4 to render 2,000 SVGs but it's not possible in Material UI v5 anymore, too slow.

Maybe, but imho we'd need to get rid of the styled() wrapper like I previously proposed here. But with all the work I did recently now that I understand the codebase I should be able to do that exploration fairly quickly with a few components to validate if it's impactful enough.

This could have potential. With @Janpot, we look at two performance opportunities mui/pigment-css#209 and mui/pigment-css#208.

[romgrk]

I can't reproduce your results for the table benchmark, in particular the large chunk which you say is preprocessStyle. It has a yellow band under it in your screenshot, which could be a Major/Minor GC pause. Those pauses are non-deterministic and can happen at any moment, and they would partially deform the profile (e.g. the whole GC pause time would be included in the "self time" of the function in which the pause happens, even if the function didn't cause the pause). That's why it's preferable to run the benchmark a ton of times and use the mean/stddev to determine if there has been an improvement. Here is an example of a GC pause happening in emotion's handleInterpolation, and deforming the profile for that run/function:

Single-run profiles are good to get insights into what's happening, but the GC makes them too unreliable to measure improvement.

Here is what I get for that test case, though this is extracted in my local test repo (which builds a non-minified production bundle so the function names are readable):

before	after

I have also highlighted above that emotion's createStringFromObject forms 9.9% of the total runtime in the before case. In the after case, that function doesn't appear (so less than 0.05%).

The After case in your screenshot is surprising though, if you can tell me more about how to reproduce it I could investigate more.

---

By the way this is a very limited improvement compared to what could be done if I was free to cache things more aggressively and remove unneeded work, but the PRs on emotion are slow (e.g. the hashing function changes) and the need to avoid breaking changes also limit a bit what I can do. If you think it's worth exploring which changes (breaking or not) could improve performance further, I could spend more time on playing with emotion.