[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/

[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.44% @mui/joy/DialogTitle: parsed: +0.41% , gzip: +0.47%

Bundle size report

Details of bundle changes (Toolpad) Details of bundle changes

Generated by :no_entry_sign: dangerJS against 080d16674c52fb3f3b62dcbf16d7104aec2185e8

[oliviertassinari]

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

We will need to check https://mui.com/blog/introducing-pigment-css/#better-runtime-performance at one point. If emotion keeps getting faster and we can implement RSC using https://github.com/emotion-js/emotion/issues/2978#issuecomment-1935131675 and Pigment CSS to work needs x3 more resource allocation, It could start to make sense for Pigment CSS to become a fork of emotion, rather than of linaria.

[romgrk]

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]

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

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 https://github.com/mui/pigment-css/issues/209 and https://github.com/mui/pigment-css/issues/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.

[romgrk]

The issues in PigmentCSS are solved, this PR is ready for review.

[misantronic]

I just updated to mui v6 and got problems with one of your changes: https://github.com/mui/material-ui/commit/7308dd06ee215d0c4bb5f3e4aa31767d06401684#diff-8d1ff500648fb816e094c97e276c5ae924a36d6ab51d2f13afb5e3974e00c490R28

I dont know the details about the code, but I was wondering why is attachTheme manipulating a prop? according to react that's not allowed and I think that's also why my application is crashing while running this code:

TypeError: Cannot assign to read only property 'theme' of object '#

props is read-only and thus cannot be manipulated by said function.

I think this happens when nesting ThemeProviders with custom themes, but I am not 100% certain 🤔

[romgrk]

Performance optimization, the prop object is being recreated at multiple times in the stack (2x by React, 1x by Emotion, previously about 4-6x by MUI). Do you have a reproducible case you can share?

[misantronic]

Performance optimization, the prop object is being recreated at multiple times in the stack (2x by React, 1x by Emotion, previously about 4-6x by MUI). Do you have a reproducible case you can share?

Ok, that's understadable. I found the issue on my side, here's the case:

I have a hook usePopoverIconButton that is internally creating an IconButton and can also receive a theme as prop. I am passing the props on to the mui-IconButton-component. The issue was that I also passed the theme to said IconButton. When removing it works again. Still, this shouldn't cause the application to crash. Maybe filter theme out and print a warning that it should be removed?

[romgrk]

The prop needs to stay for compat reasons, it's passed to style functions down the line, removing it would be a breaking change. We could pay the performance cost if there is an unsolvable problem, you can file an issue with a reproducible case if you want to discuss it more.