search

mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
94.07k stars 32.33k forks source link

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

Closed johnnynia closed 1 week ago

johnnynia commented 5 years ago

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

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: https://github.com/mui-org/material-ui/issues/15499

ypresto commented 5 years ago

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):

(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 commented 5 years ago

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

Considerations

Put cache logic to Box, styled or makeStyles?

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

ypresto commented 5 years ago

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 commented 5 years ago

@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 commented 4 years ago

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 https://github.com/mui-org/material-ui/issues/21657#issuecomment-707111575.

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:

Test cases in https://github.com/oliviertassinari/material-ui/commit/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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

tarwich commented 4 years ago

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 commented 4 years ago

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

tarwich commented 4 years ago

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

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 commented 4 years ago

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 commented 4 years ago

@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?

tarwich commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 1 week ago

I think this issue is not valid anymore.

github-actions[bot] commented 1 week ago

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 commented 1 week ago

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).