[core] perf: sx #44254
[core] perf: sx #44254
Conversation
Netlify deploy previewhttps://deploy-preview-44254--material-ui.netlify.app/ @material-ui/system: parsed: +1.44% , gzip: +1.57% Bundle size reportDetails of bundle changes (Toolpad) |
| @@ -606,6 +606,7 @@ export default function extendTheme(themeOptions?: CssVarsThemeOptions): Theme { | |||
| getCssVar, | |||
| spacing: getSpacingVal(spacing), | |||
| font: { ...prepareTypographyVars(mergedScales.typography), ...mergedScales.font }, | |||
| internal_cache: {}, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why couldn't we just add it in the createTheme function? What was the issue?
There was a problem hiding this comment.
Some created theme objects don't go through system's createTheme at all. I don't feel confident enough to pass them all through it at this point, I'm not sure what edge-cases I might find doing so.
| <Div color="white" bgcolor="palevioletred" p={1}> | ||
| Styled components | ||
| </Div> | ||
| <ThemeProvider theme={theme}> |
There was a problem hiding this comment.
Why did we need to change this demo?
| @@ -606,6 +606,7 @@ export default function extendTheme(themeOptions?: CssVarsThemeOptions): Theme { | |||
| getCssVar, | |||
| spacing: getSpacingVal(spacing), | |||
| font: { ...prepareTypographyVars(mergedScales.typography), ...mergedScales.font }, | |||
| internal_cache: {}, | |||
There was a problem hiding this comment.
Why couldn't we just add it in the createTheme function? What was the issue?
| @@ -1,7 +1,12 @@ | |||
| import PropTypes from 'prop-types'; | |||
| import isObjectEmpty from '@mui/utils/isObjectEmpty'; | |||
| import fastDeepAssign from '@mui/utils/fastDeepAssign'; | |||
There was a problem hiding this comment.
| import fastDeepAssign from '@mui/utils/fastDeepAssign'; | |
| import deepAssign from '@mui/utils/deepAssign'; |
There was a problem hiding this comment.
When I name something fast... it usually means there's a trade-off with correctness to focus on being "fast". Here it's that it doesn't handle exotic objects as other implementations will, as in, it will also traverse enumerable properties on the object's prototype. In the cases where we use this function (to create React props objects copies, or to create CSS style objects) we can safely assume that invariant is true. I should add this to the jsdoc, but I like having some sort of warning in the name.
There was a problem hiding this comment.
Adding this to the jsdom would definitely help. On the other hand, I would recommend maybe moving this util outside of @mui/utils, if it is only used in the mui-system, to avoid developers accidently using it in other places where it may not be the right thing. If you already plan to use it in other places, then probably we can keep it in @mui/utils.
|
|
||
| if (isPrimitive(source)) { | ||
| return source; | ||
| } else if (isPrimitiveOrBuiltIn(target)) { |
There was a problem hiding this comment.
Why not just check for built in here? We have the primitive check already on the previous condition.
There was a problem hiding this comment.
I didn't write it, I pulled it from the source listed at the top and made a few modifications to simplify it. I could simplify it further.
There was a problem hiding this comment.
Ah, I thought is a new code, we can update it later in that case, up to you.
Improve
sxperformance by removing allocations.Example user code:
We get roughly 35% less rendering time for the example above which makes heavy use of the
sxprop, 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 likestyle(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
sxobject/subobject is also expensive, I've tried to remove it by lazily initializing the breakpoints buthandleBreakpointsis used too much in the codebase so that's another large refactor.