-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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][perf] Remove system allocations #43306
Conversation
Netlify deploy previewhttps://deploy-preview-43306--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
for (const key in variant.props) { | ||
if (props[key] !== variant.props[key] && props.ownerState?.[key] !== variant.props[key]) { | ||
continue variantLoop; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed that variants are quite expensive. For each Button
rendered, each of its variants is iterated to find the matching ones. Been wondering how to remove/reduce that cost, but I haven't found anything. This is essentially replicating the logic that browsers do with classnames (e.g. .button-small.button-outlined
) but in JS instead of C++, so it's much slower. This cost is also present with PigmenCSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious to see if CVA approached this differently, the API looks very close https://cva.style/docs/getting-started/variants. But no, it seems to be the same implementation: https://github.com/joe-bell/cva/blob/main/packages/cva/src/index.ts#L177
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found something but it needs to solve the style objects allocations first, details here: romgrk#1
function isObjectEmpty(object) { | ||
// eslint-disable-next-line | ||
for (const _ in object) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! :)
let result = otherStyles; | ||
variants.forEach((variant) => { | ||
let isMatch = true; | ||
let mergedState; // We might not need it, initalized lazily |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing seems to be missed from the previous implementation. All tests passed, so I am 👌 with the change.
It will be interesting to rerun our performance benchmarks: https://github.com/mui/material-ui/tree/next/benchmark/browser#output, and see how we compare to Chakra UI, JSS, etc after this wave of performance-focused fixes 🙌. For example, Material UI Box was 37% slower than Chakra UI Box for about the same features, something people are not blind to: https://www.reddit.com/r/nextjs/comments/18e65wc/comment/lisbpxn/ "Everything about is bloated, slow, it's a monolithic behemoth" 🙃. If we don't do it for curiosity reasons, at minimum for marketing ones cc @alelthomas. |
Shared with Jun but I'll repeat this here: To add some nuance, the 30% number is variable and could be give or take 10%, depending on the case. For example, if the user is rendering a page with a 100 buttons and nothing else, it might be up to 40% improvement because If we want realistic improvement numbers, then we need real-world-like projects that could exist in the wild. I've been wanting to create a benchmark demo dashboard app to create such a representative case but haven't had the time yet. |
A few performance optimizations on the system styled functions. The
processStyleArg
function is at the top of the list on stack traces, I've mainly removed a few{ ...spread }
and object params. The optimizations seem to improve the runtime by about 5-10%.Test case: mounting a bunch of different MUI components.