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

[core] perf: sx #44254

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open

[core] perf: sx #44254

wants to merge 55 commits into from

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Oct 29, 2024

Improve sx performance by removing allocations.

Example user code:

Before: 23.4ms +-1.6
After:  15.2ms +-1.1

n=100

We get roughly 35% less rendering time for the example above which makes heavy use of the sx prop, so I guess user-code would see something in the 0-40% less rendering time depending on how much they use that prop.

We can still get a lot more improvement by eliminating more memory allocations (I think we can get that example to at least 50% less rendering time), but the remaining changes would require more substantial work. The format of the style({ [prop]: value, theme }) sx style handlers is expensive, we could instead use something like style(prop, value, theme), though IIUC the system props also use those so there's a bit of refactoring to do.

The logic to create an empty breakpoint object for each sx object/subobject is also expensive, I've tried to remove it by lazily initializing the breakpoints but handleBreakpoints is used too much in the codebase so that's another large refactor.

@romgrk romgrk added performance core Infrastructure work going on behind the scenes labels Oct 29, 2024
@mui-bot
Copy link

mui-bot commented Oct 29, 2024

Netlify deploy preview

https://deploy-preview-44254--material-ui.netlify.app/

@material-ui/system: parsed: +1.44% , gzip: +1.57%
PigmentHidden: parsed: +1.91% , gzip: +1.90%
Slide: parsed: +1.81% , gzip: +1.95%
Fade: parsed: +1.89% , gzip: +1.89%
Zoom: parsed: +1.89% , gzip: +1.89%
Box: parsed: +1.59% , gzip: +1.87%
Grow: parsed: +1.83% , gzip: +1.93%
@material-ui/lab: parsed: +0.36% , gzip: +0.52%
@material-ui/core/styles/createTheme: parsed: +2.60% , gzip: +2.90%
SpeedDialAction: parsed: +0.78% , gzip: +0.95%
ListItemText: parsed: +1.45% , gzip: +1.50%
TextField: parsed: +0.67% , gzip: +0.86%
CardHeader: parsed: +1.44% , gzip: +1.48%
Autocomplete: parsed: +0.67% , gzip: +0.82%
FilledInput: parsed: +1.15% , gzip: +1.42%
Fab: parsed: +1.16% , gzip: +1.37%
createBox: parsed: +2.71% , gzip: +2.97%
@material-ui/core: parsed: +0.20% , gzip: +0.26%
LoadingButton: parsed: +1.03% , gzip: +1.26%
@mui/joy/Box: parsed: +1.78% , gzip: +2.09%
and 191 more changes

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 3455a04

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mui/utils/deepmerge is incredibly unefficient but it's used all across the codebase and I don't have enough bandwidth to try to replace it. But it should be replaced.

@romgrk romgrk marked this pull request as ready for review November 4, 2024 16:10
@romgrk romgrk requested a review from a team November 4, 2024 16:10
@@ -606,6 +606,7 @@ export default function extendTheme(themeOptions?: CssVarsThemeOptions): Theme {
getCssVar,
spacing: getSpacingVal(spacing),
font: { ...prepareTypographyVars(mergedScales.typography), ...mergedScales.font },
internal_cache: {},
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 would have expected to have to add .internal_cache to the system's createTheme function only, but I ended up needing to add it at various locations. It would be neat to pass all our themes through that function to avoid repeating internal details in various places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants