-
Notifications
You must be signed in to change notification settings - Fork 0
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
[perf] Remove theme allocations #1
Conversation
I got a bit confused looking at more of those diffs for MUI X support of Material UI v6.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…ue not equal to option (mui#43314) Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Co-authored-by: Aarón García Hervás <aaron.garcia.hervas@gmail.com> Co-authored-by: Michał Dudak <michal.dudak@gmail.com>
Co-authored-by: DiegoAndai <diego@mui.com> Co-authored-by: Aarón García Hervás <aaron@mui.com>
function preprocessVariants(style) { | ||
// Compiles predicate functions for each variant, to add `variant.matches(props)`. |
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.
This is an example of optimization that can be unlocked once we know the style
is static & shared across all instances of a component. We can speed up matching variant styles by eval-compiling predicate function for their props
.
This is independent of the core proposition (removing theme/style allocations on every render) though, it's just here to illustrate how a stable theme makes other things possible.
})(({ theme }) => ({ | ||
})(styled.pure((theme) => ({ |
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.
Not entirely sure how PigmentCSS works and if this would mess up its extraction logic though.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Co-authored-by: Aarón García Hervás <aaron.garcia.hervas@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Aarón García Hervás <aaron@mui.com>
@@ -86,7 +86,7 @@ const ButtonRoot = styled(ButtonBase, { | |||
ownerState.fullWidth && styles.fullWidth, | |||
]; | |||
}, | |||
})(({ theme }) => { | |||
})(styled.pure(theme => { |
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 think Pigment CSS needs to support this too.
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.
Let's wait @brijeshb42 to confirm if this is possible
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.
Let's continue the discussion here: mui#43372 (comment)
Draft to show how to avoid theme/styling allocations and apply optimizations on top of a stable theme. This is opened here instead of the core repo just to present the changes compared to the
perf-system-allocations
.The problem with theme allocations is:
That function there runs on every render for every button, and allocates the huge style object for each of them. It's an insane cost, and that style object is completely identical as long as the
theme
stays constant.There's a very simple option, it's to add a memoization wrapper around that function when we know it's pure and doesn't need access to
props
. I've implemented a draft of this approach here, it saves us another 10% runtime cost across the board (so 30% total with the other perf PRs).