[system] Remove theme/styling allocations

[romgrk]

The problem with theme allocations is:

const Button = styled('button', {
  name: 'Button',
})(({ theme }) => /* <----------- this function here */ ({
  color: theme.palette.fg,
  /* lots more styling */
  variants: [
    ...
  ],
}));

That function there runs on every render for every button, and allocates the huge style object for each of them. It's a huge cost, and that style object is completely identical as long as the theme stays constant. There's a very simple option, it's to add a memoization wrapper around that function when we know it's pure and doesn't need access to props.

We get a ~20% improvement compared to the current next branch (which already includes some of the other perf changes, but this would be ~15% improvement compared to baseline before I did any perf optimization).

Test case (Button, Checkbox, Switch, TextField)

Before: 137.6ms	+- 5.4
After:  105.8ms	+- 2.9

N = 20

[romgrk]

Not sure how PigmentCSS works, could this mess up its extraction?

[siriwatknp]

I checked. Pigment CSS could work if the fromTheme is an independent function, not a member of the styled. Is it possible?

[siriwatknp]

Ok, found the fromTheme function below. This should work with Pigment CSS:

import { fromTheme } from '../some-path';

styled('div')(fromTheme(({ theme }) => ({ … })))

[romgrk]

The function doesn't need to be attached but I did it to make it easy for typescript types, but I think maybe TS can handle inferring theme's type just by being a styled(...)() argument.

As for the argument, I made it only (theme) => ... instead of ({ theme }) => ... because that ensures that the function only needs to memoize theme. If the whole props bundle is included, then there's more stuff to memoize and that is more expensive. I've already sketched out what needs to happen for functions that need access to props: https://github.com/mui/material-ui/pull/43372/files#diff-8d1ff500648fb816e094c97e276c5ae924a36d6ab51d2f13afb5e3974e00c490R259-R323

[mnajdova]

Agree, let's use fromTheme(({ theme }) => ({ … }). Can we update the types to only expect theme here and how TS errors if people try to access other props (that would be my only worry).

[romgrk]

I've updated the code to fit in those constraints, and I've settled on memoTheme for the name.

[mui-bot]

Netlify deploy preview

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

@material-ui/core: parsed: +0.12% , gzip: +0.05%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 8921455

[romgrk]

There's a problem with prettier, it's going to forcefully increase the indentation level everywhere inside memoTheme(({ theme }) => /* here */), which is one of the reasons I dislike prettier. Anyway, I've left the PR without prettifying it to ease the review, but I'll run it as a last step before merging.

[mnajdova]

I've tested the Pigment CSS vite app, it works as expected, I couldn't think of anything that would break. It's a clean solution!

cc @siriwatknp anything you would like to check before merging?

[siriwatknp]

👍 Awesome!

[siriwatknp]

Ready to be merged but I don't know if the argos change is flaky or related to this change, could someone confirm? If it's unrelated feel free to merge directly.

Yeah, it's a flaky one. I'm merging this.

[oliviertassinari]

Are we sure about the DX of this? As a developer looking at the source:https://github.com/romgrk/material-ui/blob/8921455731c32f13dd9d61fa49002feb21b673fa/packages/mui-material/src/Accordion/Accordion.js#L36, the first thing I ask myself: why is the source verbose like this? Couldn't this be a flag?

 const AccordionRoot = styled(Paper, {
   name: 'MuiAccordion',
   slot: 'Root',
+  onlyTheme: true,
   overridesResolver: (props, styles) => {
     const { ownerState } = props;

     return [
       { [`& .${accordionClasses.region}`]: styles.region },
       styles.root,
       !ownerState.square && styles.rounded,
       !ownerState.disableGutters && styles.gutters,
     ];
   },
 })(
  ({ theme }) => {

If we had a transpilation step, it could automatically detect those and add the flag 😄, this way, nobody will forget about it, or use it wrong.

[romgrk]

It could have been a flag, I didn't think about it, but I didn't think about it too much because it can be refactored anytime and I want to iterate on this further.

Emotion is serializing+hashing those style objects on every render of every component, even if we return the same styling object. For internal components we can know, either with the flag on the styled option or a flag on the memoTheme return value (e.g. make it return a function with a .memoized = true flag), that all the styling only needs to react to theme changes. This means we could use emotion's serializeStyles when we memoize the style value, and return the serialized object directly, which emotion will handle here and skip the createStringFromObject on every render/component.

The only thing I need to figure out is variants, we probably need to transfer them from our style value to emotion's serialized style object.

One thing though, the flag would need to affect typings, I hope it's not too complex to setup.

[romgrk]

I remember why I picked a function though, initially I also had memoProps that would memo the value based on which props were accessed (in addition to memoTheme), so it was possible to have for the same component style functions that accessed the props and some that didn't (like the Grid).

I didn't end up including memoProps because the implementation was a bit complex (used a proxy to cache which props values were accessed, like SignalJS props work), and most components didn't access the props so the performance gains would have been lower, though I didn't benchmark the difference. That could improve Grid though.

[oliviertassinari]

This means we could use emotion's serializeStyles when we memoize the style value, and return the serialized object directly, which emotion will handle here and skip the createStringFromObject on every render/component.

Oh, nice, like it used to be in Material UI v4 with JSS. Styles created once per component type.

The only thing I need to figure out is variants, we probably need to transfer them from our style value to emotion's serialized style object.

Maybe we can create custom class name for each variant and apply them conditionally, like Pigment CSS is doing or like we were doing in Material UI v4. The difference now is that we have CSS variables, we have fewer variants to create, e.g. only one for color and not one for color primary, color secondary, etc.

[romgrk]

Follow-up here: #43412

[oliviertassinari]

The location of the helper doesn't seem to make sense from a product perspective, issue open #43440.