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

[system][perf] Remove system allocations #43306

Merged
merged 11 commits into from
Aug 19, 2024
Merged

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Aug 15, 2024

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

number of runs: 20

before: 185.31ms +- 20.1
after:  166.9ms  +- 15.6

Test case: mounting a bunch of different MUI components.

image

@romgrk romgrk requested a review from a team August 15, 2024 06:32
@mui-bot
Copy link

mui-bot commented Aug 15, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5f5c14a

Comment on lines +52 to +54
for (const key in variant.props) {
if (props[key] !== variant.props[key] && props.ownerState?.[key] !== variant.props[key]) {
continue variantLoop;
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'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.

Copy link
Member

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

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've found something but it needs to solve the style objects allocations first, details here: romgrk#1

@oliviertassinari oliviertassinari added the package: system Specific to @mui/system label Aug 18, 2024
function isObjectEmpty(object) {
// eslint-disable-next-line
for (const _ in object) {
return false;
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@mnajdova mnajdova left a 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.

@romgrk romgrk merged commit 9d34813 into mui:next Aug 19, 2024
18 of 19 checks passed
@oliviertassinari oliviertassinari changed the title [perf] Remove system allocations [system][perf] Remove system allocations Aug 19, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 19, 2024

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.

@romgrk
Copy link
Contributor Author

romgrk commented Aug 19, 2024

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 Button is substantially affected by both color and theme PRs (more than the other components in the test case). On the other hand, if it's a 100 Box components it might be as little as 10-20% improvement (I have no idea, I haven't benchmarked that component, just trying to find an example of a component with little styling/variants/subcomponents).

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants