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

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

Merged
merged 40 commits into from
Oct 2, 2024

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Aug 23, 2024

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/

Comment on lines 98 to 100
function styleAttachTheme(props) {
attachTheme(props, themeId, defaultTheme)
}
Copy link
Contributor Author

@romgrk romgrk Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this mutating tag though?

Copy link
Contributor Author

@romgrk romgrk Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, @mnajdova is right.

return value;
};
}
export default memoTheme;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
export default memoTheme;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@oliviertassinari oliviertassinari Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

mui-bot commented Aug 27, 2024

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

@github-actions 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 romgrk marked this pull request as ready for review August 27, 2024 15:06
@romgrk romgrk requested a review from a team August 27, 2024 15:06
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 17, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 23, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 26, 2024
@romgrk
Copy link
Contributor Author

romgrk commented Sep 26, 2024

The issues in PigmentCSS are solved, this PR is ready for review.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 28, 2024
@github-actions 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 Sep 30, 2024
@github-actions 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
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 2, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[system] Fix memoTheme() source location
7 participants