[system] Boxes with identical style props use the same class

[johnnynia]

Every call of a Box component with identical style props creates a new CSS class.

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

All calls of a Box component with identical style props use the same CSS class.

Current Behavior 😯

Every call to a Box component with identical props creates and uses a new CSS class.

Examples 🌈

https://codesandbox.io/s/create-react-app-3its9
Box1 and Box2 have different classes.

Context 🔦

Using the Box component instead of CSS classes looks very promising. But while the multiplication of CSS classes may may be negligible in the browser, I'm hesitant to use it in our application which is very reliant on SSR and injecting critical CSS into HTML.

Maybe it would be possible to add some sort of a cache, in which the style props would act as key and the class as value?

A solution like this would lend itself to an "atomic CSS classes" approach (see related issue) while still only serving classes that are actually used.

– Or maybe I'm just missing something.

Related: #15499

[ypresto]

Box is currently using makeStyles() via styled(), which inserts <style> tag for every style using dynamic props.
Inlining styles into HTML instead of using styled() performs much faster and not slower than static styled().

Rendering 3000 elements (tried two times, not production):

• MBP 15 inch 2019 i7

• Chrome 75.0.3770.100

• inlined style version of Box: 620ms, 640ms

• static styled(): 780ms, 765ms

• styled() version of Box (original): 3.28s, 3.28s

(They have different border each other, to distinguish whether rendering is completed or not.)

You can test it here:
https://codesandbox.io/embed/box-inlined-css-perf-b9buv

I think it is a good choice to let Box inline styles, as it's style is always dynamic and perhaps relatively small.
(The challenge is to support media query.)

[ypresto]

Implemented caching makeStyles() result in Box (#16858)

Considerations

Put cache logic to Box, styled or makeStyles?

• makeStyles is responsible to cache styles, but logic around cache key is bit specific to styled or Box.
• Box only accepts non-function types (as style() and responsivePropType()). We cannot restrict props to JSON-compatible values if it used for arbitrary style functions.
• styled handles meta data from style functions (filterProps and propTypes). We can use some flag like isStaticProps or cachePerProps.
• maybe it is jss-side issue: Intelligently deduplicating rules for function values cssinjs/jss#501

Which the cache key is better, JSON.stringify(props) or whole style sheet?

• Props of Box should be relatively small and expects fast performance.
• Whole stylesheet requires large string comparison and large memory, but
  • has no JSON-compatibility issue.
  • alias-aware cache (e.g. m and margin)
  • insensitive for order in props object

[ypresto]

Which the cache key is better, JSON.stringify(props) or whole style sheet?

stylesOptions.jss.createStyleSheet() is the slowest part of dynamic style handling (2nd seems to be dynamicSheet.update(props).attach()).
Whole style sheet cache is more difficult because we should generate cache key without touching jss; to evaluate style object by hand.

Put cache logic to Box, styled or makeStyles?

makeStyles could be completely replaced by react-jss in the future, as #16180.
I chose styled() in my PR #16858.

[elxris]

@ypresto I forked your codesandbox and added styled-components implementation with @material-ui/system styled functions. I'm surprised to found that styled-components don't generate different class names for same props and are super performant.

https://codesandbox.io/embed/box-inlined-css-perf-2rztv

This seems like a beneficial point to the recent discussion about migrating to styled-components (#6115)

[oliviertassinari]

From what I understand this issue is all about performance. The v5.0.0-alpha.13 version of the Box component is a lot faster (x3) than v4, see #21657 (comment).

We can use ui-box which pushes in the atomic CSS path for a Box (but without shorthand, theme, breakpoints, etc.) as an approximation to understand the potential. If you take the benchmark @mnajdova is building #23180, and run a similar test case, you will find:

• noop (baseline):
  Mean: 2.45ms
• React primitives:
  Mean: 70.73ms
• Cost of React:
  Mean: 76.71ms
• UI Box:
  Mean: 99.68ms
• Styled alternative:
  Mean: 116.12ms
• Box emotion:
  Mean: 162.04ms

Test cases in oliviertassinari@f01a3fa992, build on top of #23180 (@mnajdova hopefully it's helpful and you can leverage some of it)

So yes, I think that using atomic class-names has the potential to help with performance, in the order of winning 60% of performance. Which is about what using Tailwind would yield (but come with different issues: CSS purging that doesn't scale, low TypeScript safety, large API to remember).

Atomic class-names also has significant challenges. Does the win potential worth spending time on it? What about bundle size? ui-box is almost larger than emotion. If we use emotion can we really implement it without making performance worse?

We have also seen https://stitches.dev/ try that direction, I haven't seen a compelling benchmark. It also requires to drop the support for nested selectors (but rarely used in the system anyway).

I'm adding the "waiting for upvotes" in order to edge against the potential high-opportunity cost.

[tarwich]

I hope this doesn't just add noise, but I arrived here from specific use case which I'd like to share in case it helps.

When I have a page with 1000 items on it, and I'm in the browser developer tools, I'd like to make an adjustment to a single class and see them all update. Sometimes this is my flow for testing out padding or sizing in odd lists. I aware that I can write styled('div') or makeStyles(), but the wonderful thing about Box was having the css right there in the JSX so it was easy to tweak and manage.

When I noticed that every render was creating 1000 new class names, it scared me. I thought it would slow the browser down or eat up memory. However, even when I chose to accept those potential issues, the developer tools thing was still important to me.

[oliviertassinari]

@tarwich Would this cut it?

import React from "react";
import Box from "@material-ui/core/Box";

export default function App() {
  return (
    <Box sx={{ "& > .child": { m: 1, bgcolor: "primary.main" } }}>
      {new Array(1000).fill().map(() => (
        <div className="child">a</div>
      ))}
    </Box>
  );
}

https://codesandbox.io/s/gallant-star-iqlxo?file=/src/App.js:0-288

---

The performance tradeoff is now documented in https://next--material-ui.netlify.app/system/basics/#performance-tradeoff. We can likely make it faster.

[tarwich]

I like that thought, @oliviertassinari and I didn't know you could even do that. I'm not sure it will work precisely for my situation. I've ported a real example to CodeSandbox to demonstrate the issue.

1. Each grid box has a separate class, so if I wanted to mess around with font size or borders, for example, I can't use a single class to do it.
2. Every time you type in the filter box, it generates a minimum of 54 new classes, which is probably fine, but is a scary thought.

https://codesandbox.io/s/material-theme-colors-l9jxv?file=/src/App.js

Once again, I'm hoping this helps support this issue and not detract from it.

[mbrookes]

@tarwich Off topic, but mind if I ask what you're working on? Looks intriguing. 🙂

[tarwich]

What are we doing with the colors?

That's part of a "Kitchen Sink" inspired by Semantic UI's Kitchen Sink. Most of the components that make up our app have a demo section on our kitchen sink page so that new developers can easily see what to use and how to implement it. It also helps when developing new components as it has some minor Storybook functionality. The particular colors page is so I can easily see the colors defined in our theme and quickly find one i.e. error. Since I'm not intimately familiar with the Material UI palette, sometimes it's easier to find a color visually, then see what it's called instead of the other way around. In other cases, the names aren't immediately obvious to me. The linked theme, for example has secondary.main, and secondary.contrastText, so I would expect to see secondary.text, but instead it's text.secondary 🤯.

What are we doing in general?

Our application is an online education platform, and was built using makeStyles all over the place. That works, and is similar to CSS, but there's a bit of a cognitive break when you have to leave your spot, jump back up to the styles, tweak something, then go back. We found <Box /> and went nuts, refactoring everything to use Box was easy and makes our code much cleaner. Our rule of thumb is basically that if you need the same Box styles in multiple places, it should probably be a component. However, even doing that results in different classes being output every time. For example, with the colors we could have made <Swatch /> components which would've been cleaner, but if internally the component output <Box /> components every time, we would still have had thousands of classes. In the end we might have to go back to makeStyles, but I was really hopeful for Box as it was very clean.

What are we doing with box?

If I want all "Swatch" instances to have the same class, then I might do something like this:

const gatherMap = new Map<Component, CssDefinition>();

const Swatch = () => {
  return <Box gather={Swatch} name="swatch" />
}

Wherein the component definition itself could be used to group class definitions. That helps prevent two components from using the same "key" for example having two distinct "swatch" components. I toyed with this idea a bit like so:

const gatherMap = new Map<Component, CssDefinition>();

const builtStyle = (component, name, style, child) => {
  if (gatherMap.has(component)) return gatherMap.get(component);

  const newComponent = styled(child)(style, {name});

  gatherMap.set(component, newComponent);

  return newComponent;
}

After writing this a bit, I realized I might be too small-brained to make this work, so I started googling and found I'm not the only one with this problem and better men than me are struggling, so I've decided I need to rethink life. Either we can't use Box the way I wanted (go back to makeStyles), we have to wait for a solution, or we have to accept thousands of classes being spit out, or (insert someone smarter's idea here).

[mbrookes]

@tarwich I'm so glad I asked! Thank you for sharing your experience in such detail.

[tarwich]

We have a view that's taking 30 seconds to update.

I worked hard to reproduce my problem from our internal project. It's in CodeSandbox now, but it's a little too big. While cutting out the part that was breaking, I pulled in a lot of things that aren't required for reproduction, so it's a little noisy.

The code is public at https://codesandbox.io/s/heuristic-sammet-5iyyq?file=/src/components/course-map.tsx
The idea is to present a map of our online course and the progress of each student in every lesson. We have created a basic skeleton, but it's hosing the browser.

Example render (click to expand)

Flamegraph (click to expand)

task	time
render	17.8s
recalculate Style	10.59s

total nodes	5,404
.MuiBox-root	3,169
.MuiAvatar-circle	620
.MuiAvatar-colorDefault	620
.MuiAvatar-root	620
.MuiSvgIcon-root	63
.MuiTypography-h6	4
.MuiTypography-root	5

Next steps

• If this is beneficial to the team and you'd like me to do something specific just let me know
• Remove all instances of <Box /> and see if that helps
• Remove all unnecessary code from the example in order to clarify problem points
• Perhaps try the v5 version of Box and see if it's faster

System information

Windows 10 Pro x64 32GB i5 3.5GHz
Firefox: 82.0.3 (64-bit)
Chrome: 86.0.4240.198 (64-bit)

Chrome profile session (flamegraph)

Profile-20201119T220720.zip

[tarwich]

Removing <Box /> fixed all my problems 🎉🎈 (also 😭). Total Render time 6s.

Flame Graph (click to expand)

Class	Count
.makeStyles-studentBar-7	2,640
.makeStyles-student-5	880
.MuiAvatar-root	880
.MuiAvatar-circle	880
.MuiAvatar-colorDefault	880
.MuiSvgIcon-root	89
.makeStyles-flex-10	88
.makeStyles-students-3	88

Minor Classes

Class	Count
.MuiTypography-root	6
.makeStyles-moduleName-4	5
.MuiTypography-h6	5
.makeStyles-grid--NNN	2
.makeStyles-flex--NNN	1
.MuiTypography-h-NNN	1
.makeStyles-header--NNN	1
.MuiInputBase-root	1
.MuiInput-root	1
.MuiInput-underline	1
.MuiSelect-root	1
.MuiSelect-select	1
.MuiSelect-selectMenu	1
.MuiInputBase-input	1
.MuiInput-input	1
.MuiSelect-nativeInput	1
.MuiSelect-icon	1
.makeStyles-courseMap--NNN	1

I haven't pushed the code, because I'm not sure how to do so without removing the bad code. However, all I did was switch out all the <Box /> for code like this.

const useStyles = makeStyles((theme) => ({
  student: {
    display: 'grid',
    gridTemplate: `
      ' avatar  avatar avatar ' auto
      ' success error  pending' 5px
      / 1fr     0fr    1fr
    `,
    gridRowGap: '16px',
    '& span:nth-of-type(1)': {
      gridArea: 'avatar',
    },
  },
  studentAvatar: {},
  studentBar: {
    '&:nth-of-type(1)': { background: theme.palette.success.main },
    '&:nth-of-type(2)': { background: theme.palette.error.main },
    '&:nth-of-type(3)': { background: theme.palette.grey[300] },
  },
}));

In the end, it seems I can't use Box the way I want in material-ui 4.11.0. It was really nice to have the styles inline, but 30 seconds to render a simple page is too long. I'm still interested in this issue, so I'm going to continue to mess around. Next step for me is to put <Box /> back and try Material 5 which supposedly fixes this.

[oliviertassinari]

@tarwich Thank you for sharing this stress test! Very interesting. I have recently seen a friend face a similar issue, rendering many Box. In your case, I can count >3,000 of them. I have updated your codesandbox to v5 in https://codesandbox.io/s/romantic-pine-247ef?file=/src/data.ts. And push it in production in https://csb-247ef.netlify.app/. The performance isn't great. It's still too slow to render.

Reducing the number of Box with:

      <Box
        sx={{
          display: 'grid',
          gridTemplateRows: 'auto 5px',
          gridGap: '16px',
          '& .container': {
            display: 'grid',
            gridTemplateColumns:
              type === ActivityType.Test ? '5fr 3fr 2fr' : '7fr 0fr 3fr',
            gridAutoFlow: 'column',
          },
          '& .success': {
            bgcolor: 'success.main',
          },
          '& .error': {
            bgcolor: 'error.main',
          },
          '& .bg': {
            bgcolor: '#AAA',
          },
        }}
      >

helps a bit: https://codesandbox.io/s/upbeat-morse-j7n19?file=/src/components/course-map.tsx:4787-5348. Once pushed in production: https://csb-j7n19.netlify.app/ is a bit faster, it takes ~1s to switch between from the first item of the menu to the 3rd one in v5. This is to be compared with the production version of the reproduction with v4 that takes ~6s: https://csb-ngcjx.netlify.app/.

So what next?

• I think that it would be interesting to test with the experimentalStyled API, see if it helps with the performance in your demo.
• I would hope that there are enough content on the screen to have it make sense introduce virtualization
• Caching the generated styles between identical instances could significantly boost performance. There is a challenge around implementing an efficient caching key.
• We can still speed-up the system() transformation function.

[tarwich]

Just to let y'all know I'm going to have to take a short break. We have to get this view out, and so we're going to restructure it completely to be able to render without performance issues, then we'll circle back to the Box questions. We're probably going to do something like add a pager or something. That being said -- not all of this is related. I found that removing the Tooltip component made is 3x faster as well. Perhaps the Tooltip component isn't optimized for use on 3,000 elements but that's a conversation for another thread. As soon as I get this view out, I'll circle back to punching holes in the performance, because I really want to get to where I can use <Box /> instead of styles since it drastically simplifies the codebase.

[tarwich]

Removing the tooltips on https://codesandbox.io/s/upbeat-morse-j7n19?file=/src/components/course-map.tsx:4787-5348 drops the render from 8s to 3.5s.

So, essentially it seems like using Box for custom CSS

1. Will work with limited success if you're rendering < 1k <Box> elements at a time
2. Prevents you from targeting all instances of a component for live CSS editing

I tried googling the experimentalStyled API, but couldn't figure out what it refers to. Not sure if this is a thread I should pull on at this time.

Finally, the solution we ended up using in production was to use collapsed content where the content is all collapsed and the user can click to expand sections. This allows the user to get to a state where everything is visible, but they control the "loading", which means it loads many times faster since it's only loading like 50 items at a time. Basically this:

I would hope that there are enough content on the screen to have it make sense introduce virtualization [UP]

I think I'm done at this point, since the conversations about V5 styling will have massive impacts on this whole topic. If there is anything useful I can contribute LMK.

You buys are fantastic. Thanks for everything you've done, and are doing.

[tarwich]

Wrap-up from the stress test

I found a bug in the way I was using MobX, which was causing the view to render 2-4 times. Normally it's not noticeable since a render takes around 5-50ms, but when they're taking 1000ms, you kinda notice.

box-sx

Uses <Box /> with the sx={} property to allow an inline-declared stylesheet to affect divs within.

1400ms
100% (baseline)
500 students
(8900ms before fixing a re-rendering issue)
https://codesandbox.io/s/box-sxmaterial-ui-16704-szt11

emotion

Uses css={...} to style the same way I wanted to with , but is more performant and caches classnames.

680ms
48% (-720ms)
500 students
https://codesandbox.io/s/emotionmaterial-ui-16704-d0hoe

html

Uses <div /> with emotion (css={...}) to style the same way I wanted to with , but is more performant and caches classnames. Material-UI isn't used at all in this project.

410ms
29% (-990ms)
500 students
https://codesandbox.io/s/htmlmaterial-ui-16704-6dk6r

---

As I mentioned before, I've moved on to using Accordions to hide content, drastically reducing the first-render load. However, 680ms wouldn't be a bad render either if I had decided to go with that approach.

[siriwatknp]

I think this issue is not valid anymore.

[github-actions]

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Note

@johnnynia How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

[oliviertassinari]

Since the problem was solved with Material UI v5 by moving from JSS to emotion, I'm removing the "waiting for 👍" label as we use this with closed issues too (to see if there are some that we should reopen because our initial won't fix assessment was wrong).