[system] Pre-serialize & cache styles to improve performance #43412

romgrk · 2024-08-23T04:31:40Z

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/

romgrk · 2024-08-23T05:14:39Z

packages/mui-system/src/createStyled/createStyled.js

+  function styleAttachTheme(props) {
+    attachTheme(props, themeId, defaultTheme)
+  }


I have removed the various attachTheme calls in favor of adding a style function that runs once and attaches the theme directly on the props. It relies on running before anything else.

packages/mui-system/src/preprocessStyles.ts

romgrk · 2024-08-25T13:35:15Z

packages/mui-system/src/createStyled/createStyled.js

+
+    // If `tag` is already a styled component, filter out the `sx` style function
+    // to prevent unnecessary styles generated by the composite components.
+    processStyles(tag, (styles) => styles.filter(style => style !== styleFunctionSx));


Isn't this mutating tag though?

@siriwatknp Maybe you could confirm here, I think this line is mutating tag's style processors, but what we want is instead to skip adding the sx processor to the new styled component. Is that right?

Regardless of the question above, I think I'll rename this function to internal_mutateStyles to better express what it's doing.

I think this could be an (existing) issue/bug but I don't have the bandwidth to investigate it and I'm afraid of fixing it and possibly changing the behavior, so I've just renamed internal_processStyles to internal_mutateStyles to better highlight what is happening here.

I think we introduced this as an perf improvement as we were processing sx multiple times if we had a styled component invoked over another styled component. This is what I remember about it. Likely @siriwatknp could have more info, but +1 on not changing any behavior as part of this PR.

Yes, @mnajdova is right.

oliviertassinari · 2024-08-26T09:33:05Z

packages/mui-material/src/utils/memoTheme.ts

-    return value;
-  };
-}
+export default memoTheme;


It looks like it shouldn't even be exported #43440

Suggested change

export default memoTheme;

I was worried about the typings in the initial PR (memoTheme<T>), having it here made it easy to include the MaterialTheme, though material doesn't use much TS from what I can see. I could remove it altogether from material but it would make it less accessible to external users in case they want it.

I want us to move to a point where we force developers to provide their theme (so we can remove all @mui/system re-exports from @mui/material, so we need a great DX to not rely on a default theme, so this seems OK.

Ok. So should I remove this file and import unstable_memoTheme from @mui/system everywhere?

I think this is beyond the scope of this PR #43440.

Btw, I added the export from @mui/material because I wanted to use it in the lab, for the LoadingButton.

packages/mui-system/src/memoTheme.ts

mui-bot · 2024-08-27T08:09:08Z

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 🚫 dangerJS against 080d166

mnajdova

Looks good, sorry it took a bit longer to get this one in.

misantronic · 2024-10-26T01:54:39Z

I just updated to mui v6 and got problems with one of your changes:
7308dd0#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 · 2024-10-26T09:13:16Z

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 · 2024-10-26T11:03:03Z

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 · 2024-10-26T11:41:30Z

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.

draft: pre-serialize styles

9045d66

romgrk added the performance label Aug 23, 2024

romgrk added 3 commits August 23, 2024 00:46

lint

f7d75a6

lint

e19bcf7

lint

61f432e

romgrk commented Aug 23, 2024

View reviewed changes

lint

2cf0557

siriwatknp reviewed Aug 23, 2024

View reviewed changes

packages/mui-system/src/preprocessStyles.ts Outdated Show resolved Hide resolved

refactor: lints

bce2ac0

romgrk force-pushed the perf-serialize-styles branch from ea7b780 to bce2ac0 Compare August 23, 2024 13:10

lint

db0bf76

romgrk commented Aug 25, 2024

View reviewed changes

romgrk mentioned this pull request Aug 26, 2024

[system] Remove theme/styling allocations #43372

Merged

oliviertassinari reviewed Aug 26, 2024

View reviewed changes

packages/mui-system/src/memoTheme.ts Outdated Show resolved Hide resolved

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 26, 2024

romgrk added 2 commits August 27, 2024 04:04

refactor: add .isProcessed

c347735

lint

0944431

romgrk added 2 commits August 27, 2024 04:12

Merge branch 'next' into perf-serialize-styles

05d06da

Merge branch 'master' into perf-serialize-styles

6f3dfb5

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 27, 2024

romgrk added 5 commits August 27, 2024 04:16

lint

25cfd5c

fix: style types

1ef64d8

fix: build

bd770e2

fix: build

30c33ab

lint

c3ab49e

romgrk marked this pull request as ready for review August 27, 2024 15:06

romgrk requested a review from a team August 27, 2024 15:06

lint

4fb397d

Merge branch 'master' into perf-serialize-styles

c1003b3

github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 1, 2024

mnajdova approved these changes Oct 2, 2024

View reviewed changes

mnajdova mentioned this pull request Oct 2, 2024

Bump Pigment CSS to 0.0.24 #43917

Closed

1 task

o-alexandrov mentioned this pull request Oct 2, 2024

[RFC] Restructure icons exports #42704

Open

Merge branch 'master' into perf-serialize-styles

afd9442

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 2, 2024

Merge branch 'master' into perf-serialize-styles

0d220c4

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 2, 2024

Merge branch 'master' into perf-serialize-styles

080d166

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 2, 2024

romgrk merged commit 7308dd0 into mui:master Oct 2, 2024
19 checks passed

romgrk deleted the perf-serialize-styles branch October 26, 2024 09:10

This was referenced Oct 31, 2024

[Snyk] Upgrade @mui/icons-material from 6.1.2 to 6.1.3 sensasi-delight/simulasi-grading-sawit#29

Merged

[Snyk] Upgrade @mui/material from 6.1.2 to 6.1.3 sensasi-delight/simulasi-grading-sawit#30

Merged

Souviksamanta34 mentioned this pull request Oct 31, 2024

[Snyk] Upgrade @mui/icons-material from 6.1.2 to 6.1.3 Souviksamanta34/gruham#17

Open

bansikah22 mentioned this pull request Oct 31, 2024

[Snyk] Upgrade @mui/material from 6.1.2 to 6.1.3 bansikah22/user-fullstack#11

Open

This was referenced Oct 31, 2024

[Snyk] Upgrade @mui/material from 5.10.10 to 6.1.3 https-quantumblockchainai-atlassian-net/ossinsight#970

Open

[Snyk] Upgrade @mui/icons-material from 5.10.9 to 6.1.3 https-quantumblockchainai-atlassian-net/ossinsight#971

Open

This was referenced Nov 1, 2024

[Snyk] Upgrade @mui/icons-material from 6.1.2 to 6.1.3 SE310-Team-1/SE310_Fitness_Tracker#99

Open

[Snyk] Upgrade @mui/material from 6.1.2 to 6.1.3 SE310-Team-1/SE310_Fitness_Tracker#100

Open

josetutis mentioned this pull request Nov 2, 2024

[Snyk] Upgrade @mui/styled-engine-sc from 6.0.0-alpha.18 to 6.1.3 lazycloud0/KingsHeathClangersApp#160

Open

Hoang-Nguyen-Huy mentioned this pull request Nov 2, 2024

[Snyk] Upgrade @mui/icons-material from 6.1.2 to 6.1.3 Hoang-Nguyen-Huy/fer-lab1#19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[system] Pre-serialize & cache styles to improve performance #43412

[system] Pre-serialize & cache styles to improve performance #43412

romgrk commented Aug 23, 2024 •

edited by oliviertassinari

Loading

romgrk Aug 23, 2024 •

edited

Loading

romgrk Aug 25, 2024

romgrk Aug 27, 2024 •

edited

Loading

romgrk Aug 30, 2024

romgrk Aug 30, 2024

mnajdova Aug 30, 2024

siriwatknp Sep 17, 2024

oliviertassinari Aug 26, 2024

romgrk Aug 27, 2024

oliviertassinari Aug 27, 2024 •

edited

Loading

romgrk Aug 27, 2024

oliviertassinari Aug 28, 2024

mnajdova Aug 28, 2024

mui-bot commented Aug 27, 2024 •

edited

Loading

mnajdova left a comment

misantronic commented Oct 26, 2024

romgrk commented Oct 26, 2024

misantronic commented Oct 26, 2024

romgrk commented Oct 26, 2024

[system] Pre-serialize & cache styles to improve performance #43412

[system] Pre-serialize & cache styles to improve performance #43412

Conversation

romgrk commented Aug 23, 2024 • edited by oliviertassinari Loading

romgrk Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romgrk Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mui-bot commented Aug 27, 2024 • edited Loading

Netlify deploy preview

Bundle size report

mnajdova left a comment

Choose a reason for hiding this comment

misantronic commented Oct 26, 2024

romgrk commented Oct 26, 2024

misantronic commented Oct 26, 2024

romgrk commented Oct 26, 2024

romgrk commented Aug 23, 2024 •

edited by oliviertassinari

Loading

romgrk Aug 23, 2024 •

edited

Loading

romgrk Aug 27, 2024 •

edited

Loading

oliviertassinari Aug 27, 2024 •

edited

Loading

mui-bot commented Aug 27, 2024 •

edited

Loading