Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

makeStyles API creates thousands of <style> tags synchronously #16756

Closed
2 tasks done
Swatinem opened this issue Jul 26, 2019 · 10 comments
Closed
2 tasks done

makeStyles API creates thousands of <style> tags synchronously #16756

Swatinem opened this issue Jul 26, 2019 · 10 comments
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. performance

Comments

@Swatinem
Copy link

We use hooks with dynamic props extensively.
I might even say it completely got out of hand, our codebase basically has hooks for every CSS property you can set, for example:

    const { widthClass } = useWidth({ width })
    const { fullWidthClass } = useFullWidth({ fullWidth })
    const { maxWidthClass } = useMaxWidth({ maxWidth })
    const { flexChildClass } = useFlexChild({ flexGrow, flexShrink, alignSelf })
    const { spacingClass } = useSpacing(props)
    const { verticalSpacingClass } = useVerticalSpacing({ verticalSpacing })
    const { horizontalSpacingClass } = useHorizontalSpacing({ horizontalSpacing })
    const { overflowClass } = useOverflow({ overflow })

However, this extensive usage is super bad for performance, as it seems the hooks API creates and injects 1) empty styles 2) synchronously.

The profile starts here:
https://github.com/mui-org/material-ui/blob/d4c7c0586bb7853705ed1337f3f8392eb892a1bc/packages/material-ui-styles/src/makeStyles/makeStyles.js#L225
and ends up here:
https://github.com/cssinjs/jss/blob/0b17c5d9717141565e466ea0dafdf3ccbe86df5a/packages/jss/src/DomRenderer.js#L335

However, up to 99% of the <style> tags end up empty:

> [...document.querySelectorAll('[data-jss][data-meta="makeStyles"]')].length
1227
> [...document.querySelectorAll('[data-jss][data-meta="makeStyles"]')].filter(e => e.innerHTML.trim() === '').length
1215
> 1215 / 1227
0.9902200488997555
  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Possibly related to #16111 maybe?

Expected Behavior 🤔

  • jss should not create empty style tags
  • jss should group DOM updates

Current Behavior 😯

  • 99% of the <style> tags created by jss are empty
  • the style tags are inserted inside useSynchronousEffect and do DOM thrashing

Steps to Reproduce 🕹

Link: https://www.eversports.style/design/icons

Just loading the page takes ages, navigating between Typography and Icons sections takes in the order of 2-3 seconds, on a beefy desktop browser.

sorry for not providing a reduced testcase, but meh, its quite clear whats going on.

Context 🔦

Your Environment 🌎

Tech Version
Material-UI v4.2.1
React v16.8.6
jss v10.0.0-alpha.17
@Swatinem
Copy link
Author

Bildschirmfoto von 2019-07-26 11-02-02

This is how the profile of a single navigation / render looks like.

@eps1lon eps1lon added package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. performance labels Jul 26, 2019
@oliviertassinari
Copy link
Member

Related to #16180.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 4, 2019

for example:

@Swatinem I wouldn't do that! Have you considered not using @material-ui/styles, using for instance, styled-components? The main reason for the existence of @material-ui/styles is to avoid the need to duplicate a CSS-in-JS runtime. If you are OK with +14 kB gzipped, you should use styled-components instead.

Related to #6115 and #16947.

@Swatinem
Copy link
Author

Swatinem commented Sep 4, 2019

Have you considered not using @material-ui/styles, using for instance, styled-components?

We have discussed this internally today, @priscilawebdev will evaluate moving our uses to styled-components, and keep an eye on how the performance changes…

@wereHamster
Copy link
Contributor

@Swatinem I don't think these style tags are actually empty. When you create a <style> element and add the rules to it via JS API (which many of the CSS-in-JS solutions do, due to performance reasons), then the resulting CSS won't show up as the innerHTML string. You should use document.styleSheets to enumerate all stylesheets and then check .rules.length to see if they are really empty.

@Swatinem
Copy link
Author

Swatinem commented Sep 7, 2019

@wereHamster oh nice, I didn’t know…

Then it becomes:

Array.from(document.styleSheets).map(s => s.rules.length).length
1266
Array.from(document.styleSheets).map(s => s.rules.length).filter(l => l > 0).length
1249
Array.from(document.styleSheets).map(s => s.rules.length).filter(l => l > 1).length
48

So most of these are single rule stylesheets.

[...document.styleSheets].flatMap(s => [...s.cssRules]).length
1416
new Set([...document.styleSheets].flatMap(s => [...s.cssRules]).map(r => r.cssText)).size
1347

Most of the rules however are unique. That kind of surprises me…
But actually looking at some of the rules makes me facepalm even more…

[...document.styleSheets].flatMap(s => [...s.cssRules]).map(r => r.cssText)
​​1087: ".c61 { font-weight: normal; }"
​​1088: ".c91 { font-weight: normal; }"
… this goes on for hundreds of rules…
​​​​1138: ".c94 { }"
​​1139: ".c107 { }"
… hundreds of these as well…
​​​201: ".c103.c103 > * { margin-right: 8px; }"
​​​202: ".c103.c103 > :last-child { margin-right: 0px; }"
… hm, some specificity hacks maybe?

So there is no deduplication going on …

Lets investigate further…

new Set([...document.styleSheets].flatMap(s => [...s.cssRules]).map(r => r.cssText).filter(t => t.startsWith('.c')).map(t => t.replace(/^\.c\d+/, ''))).size
84
new Set([...document.styleSheets].flatMap(s => [...s.cssRules]).map(r => r.cssText).filter(t => t.startsWith('.c')).map(t => t.replace(/^(\.c\d+)+/, ''))).size
66

So we are down to < 100 unique classname rules, even less when we remove duplicates due to specificity…

Well not only do we have all these mostly duplicate rules injected into the head, they also have uselessly unique classnames, which defeats the whole purpose style sharing via classes, and blows up the generated html as well…

@Swatinem Swatinem changed the title makeStyles API creates thousands of empty <style> tags synchronously makeStyles API creates thousands of <style> tags synchronously Sep 7, 2019
@tlambrou
Copy link

Well not only do we have all these mostly duplicate rules injected into the head, they also have uselessly unique classnames, which defeats the whole purpose style sharing via classes, and blows up the generated html as well…

@Swatinem did you ever get an explanation for this? Did you ultimately move to styled components to improve performance?

@Swatinem
Copy link
Author

I have moved on to different things by now, maybe @treylon can give you an answer.

@treylon
Copy link

treylon commented Sep 20, 2020

@tlambrou We ultimately moved back to styled components and stopped investigating further.

@oliviertassinari
Copy link
Member

Duplicate of #17370

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. performance
Projects
None yet
Development

No branches or pull requests

6 participants