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

[perf] Remove theme allocations #1

Closed
wants to merge 58 commits into from

Conversation

romgrk
Copy link
Owner

@romgrk romgrk commented Aug 19, 2024

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:

const Button = styled('button', {
  name: 'Button',
})(({ theme }) => /* <----------- this function here */ ({
  color: theme.palette.fg,
  /* lots more styling */
  variants: [
    ...
  ],
}));

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).

siriwatknp and others added 24 commits August 15, 2024 13:21
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>
Comment on lines 37 to 38
function preprocessVariants(style) {
// Compiles predicate functions for each variant, to add `variant.matches(props)`.
Copy link
Owner Author

@romgrk romgrk Aug 19, 2024

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) => ({
Copy link
Owner Author

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.

renovate bot and others added 26 commits August 19, 2024 11:58
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 => {

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.

Copy link

@siriwatknp siriwatknp Aug 20, 2024

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

Copy link
Owner Author

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)

@romgrk romgrk closed this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.