Skip to content

[system][perf] Remove system allocations #43306

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Aug 19, 2024
Merged

[system][perf] Remove system allocations #43306

Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
dab6804
perf: remove lots of waste
romgrk Aug 15, 2024
bd0610e
perf: more optimizations
romgrk Aug 15, 2024
5b0db71
fix: bug
romgrk Aug 15, 2024
479d91b
lint
romgrk Aug 15, 2024
a691b1a
lint
romgrk Aug 15, 2024
c0a4188
lint
romgrk Aug 15, 2024
0ebf9e3
lint
romgrk Aug 15, 2024
d75e91b
perf: remove more allocations
romgrk Aug 18, 2024
0f81fe2
refactor: import
romgrk Aug 19, 2024
f0c2ea2
lint
romgrk Aug 19, 2024
5f5c14a
lint
romgrk Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 80 additions & 60 deletions packages/mui-system/src/createStyled/createStyled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,87 +6,72 @@ import getDisplayName from '@mui/utils/getDisplayName';
import createTheme from '../createTheme';
import styleFunctionSx from '../styleFunctionSx';

function isEmpty(obj) {
return Object.keys(obj).length === 0;
}

// https://github.com/emotion-js/emotion/blob/26ded6109fcd8ca9875cc2ce4564fee678a3f3c5/packages/styled/src/utils.js#L40
function isStringTag(tag) {
return (
typeof tag === 'string' &&
// 96 is one less than the char code
// for "a" so this is checking that
// it's a lowercase character
tag.charCodeAt(0) > 96
);
}
export const systemDefaultTheme = createTheme();

// Update /system/styled/#api in case if this changes
export function shouldForwardProp(prop) {
return prop !== 'ownerState' && prop !== 'theme' && prop !== 'sx' && prop !== 'as';
}

export const systemDefaultTheme = createTheme();

const lowercaseFirstLetter = (string) => {
if (!string) {
return string;
}
return string.charAt(0).toLowerCase() + string.slice(1);
};

function resolveTheme({ defaultTheme, theme, themeId }) {
return isEmpty(theme) ? defaultTheme : theme[themeId] || theme;
function resolveTheme(themeId, theme, defaultTheme) {
return isObjectEmpty(theme) ? defaultTheme : theme[themeId] || theme;
}

function defaultOverridesResolver(slot) {
if (!slot) {
return null;
}
return (props, styles) => styles[slot];
return (_props, styles) => styles[slot];
}

function processStyleArg(callableStyle, { ownerState, ...props }) {
function processStyleArg(callableStyle, props) {
const resolvedStylesArg =
typeof callableStyle === 'function' ? callableStyle({ ownerState, ...props }) : callableStyle;
typeof callableStyle === 'function' ? callableStyle(props) : callableStyle;

if (Array.isArray(resolvedStylesArg)) {
return resolvedStylesArg.flatMap((resolvedStyle) =>
processStyleArg(resolvedStyle, { ownerState, ...props }),
);
return resolvedStylesArg.flatMap((style) => processStyleArg(style, props));
}

const mergedState = { ...props, ...ownerState, ownerState };
if (Array.isArray(resolvedStylesArg?.variants)) {
const { variants, ...otherStyles } = resolvedStylesArg;

if (
!!resolvedStylesArg &&
typeof resolvedStylesArg === 'object' &&
Array.isArray(resolvedStylesArg.variants)
) {
const { variants = [], ...otherStyles } = resolvedStylesArg;
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.

👍


/* eslint-disable no-labels */
variantLoop: for (let i = 0; i < variants.length; i += 1) {
const variant = variants[i];

if (typeof variant.props === 'function') {
isMatch = variant.props(mergedState);
mergedState ??= { ...props, ...props.ownerState, ownerState: props.ownerState };
if (!variant.props(mergedState)) {
continue;
}
} else {
Object.keys(variant.props).forEach((key) => {
if (ownerState?.[key] !== variant.props[key] && props[key] !== variant.props[key]) {
isMatch = false;
for (const key in variant.props) {
if (props[key] !== variant.props[key] && props.ownerState?.[key] !== variant.props[key]) {
continue variantLoop;
Comment on lines +68 to +70
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

}
});
}
if (isMatch) {
if (!Array.isArray(result)) {
result = [result];
}
result.push(
typeof variant.style === 'function' ? variant.style(mergedState) : variant.style,
);
}
});

if (!Array.isArray(result)) {
result = [result];
}
let style;
if (typeof variant.style === 'function') {
mergedState ??= { ...props, ...props.ownerState, ownerState: props.ownerState };
style = variant.style(mergedState);
} else {
style = variant.style;
}
result.push(style);
}
/* eslint-enable no-labels */

return result;
}

return resolvedStylesArg;
}

Expand All @@ -99,7 +84,7 @@ export default function createStyled(input = {}) {
} = input;

const systemSx = (props) => {
return styleFunctionSx({ ...props, theme: resolveTheme({ ...props, defaultTheme, themeId }) });
return styleFunctionSx({ ...props, theme: resolveTheme(themeId, props.theme, defaultTheme) });
};
systemSx.__mui_systemSx = true;

Expand Down Expand Up @@ -169,38 +154,47 @@ export default function createStyled(input = {}) {
return (props) =>
processStyleArg(stylesArg, {
...props,
theme: resolveTheme({ theme: props.theme, defaultTheme, themeId }),
theme: resolveTheme(themeId, props.theme, defaultTheme),
});
}
return stylesArg;
};

const muiStyledResolver = (styleArg, ...expressions) => {
let transformedStyleArg = transformStyleArg(styleArg);
const expressionsWithDefaultTheme = expressions ? expressions.map(transformStyleArg) : [];

if (componentName && overridesResolver) {
expressionsWithDefaultTheme.push((props) => {
const theme = resolveTheme({ ...props, defaultTheme, themeId });
const theme = resolveTheme(themeId, props.theme, defaultTheme);
if (
!theme.components ||
!theme.components[componentName] ||
!theme.components[componentName].styleOverrides
) {
return null;
}

const styleOverrides = theme.components[componentName].styleOverrides;
const resolvedStyleOverrides = {};
const propsWithTheme = { ...props, theme };

// TODO: v7 remove iteration and use `resolveStyleArg(styleOverrides[slot])` directly
Object.entries(styleOverrides).forEach(([slotKey, slotStyle]) => {
resolvedStyleOverrides[slotKey] = processStyleArg(slotStyle, { ...props, theme });
});
// eslint-disable-next-line guard-for-in
for (const slotKey in styleOverrides) {
resolvedStyleOverrides[slotKey] = processStyleArg(
styleOverrides[slotKey],
propsWithTheme,
);
}

return overridesResolver(props, resolvedStyleOverrides);
});
}

if (componentName && !skipVariantsResolver) {
expressionsWithDefaultTheme.push((props) => {
const theme = resolveTheme({ ...props, defaultTheme, themeId });
const theme = resolveTheme(themeId, props.theme, defaultTheme);
const themeVariants = theme?.components?.[componentName]?.variants;
return processStyleArg({ variants: themeVariants }, { ...props, theme });
});
Expand Down Expand Up @@ -245,3 +239,29 @@ export default function createStyled(input = {}) {
return muiStyledResolver;
};
}

function isObjectEmpty(obj) {
// eslint-disable-next-line
for (const _ in obj) {
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! :)

}
return true;
}

// https://github.com/emotion-js/emotion/blob/26ded6109fcd8ca9875cc2ce4564fee678a3f3c5/packages/styled/src/utils.js#L40
function isStringTag(tag) {
return (
typeof tag === 'string' &&
// 96 is one less than the char code
// for "a" so this is checking that
// it's a lowercase character
tag.charCodeAt(0) > 96
);
}

function lowercaseFirstLetter(string) {
if (!string) {
return string;
}
return string.charAt(0).toLowerCase() + string.slice(1);
}
Loading