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.

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

Jump to bottom

[system][perf] Remove system allocations #43306

Merged
merged 11 commits into from
Aug 19, 2024
Merged

[system][perf] Remove system allocations #43306

Changes from all 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
189 changes: 111 additions & 78 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,88 +6,88 @@ 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();
function resolveTheme(themeId, theme, defaultTheme) {
return isObjectEmpty(theme) ? defaultTheme : theme[themeId] || theme;
}

const lowercaseFirstLetter = (string) => {
if (!string) {
return string;
const PROCESSED_PROPS = Symbol('mui.processed_props');

function attachTheme(props, themeId, defaultTheme) {
if (PROCESSED_PROPS in props) {
return props[PROCESSED_PROPS];
}
return string.charAt(0).toLowerCase() + string.slice(1);
};

function resolveTheme({ defaultTheme, theme, themeId }) {
return isEmpty(theme) ? defaultTheme : theme[themeId] || theme;
const processedProps = {
...props,
theme: resolveTheme(themeId, props.theme, defaultTheme),
};

props[PROCESSED_PROPS] = processedProps;
processedProps[PROCESSED_PROPS] = processedProps;

return processedProps;
}

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

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

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

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

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];
}
if (typeof variant.style === 'function') {
mergedState ??= { ...props, ...props.ownerState, ownerState: props.ownerState };
result.push(variant.style(mergedState));
} else {
result.push(variant.style);
}
}
/* eslint-enable no-labels */

return result;
}
return resolvedStylesArg;

return resolvedStyle;
}

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

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

return (tag, inputOptions = {}) => {
const styled = (tag, inputOptions = {}) => {
// Filter out the `sx` style function from the previous styled component to prevent unnecessary styles generated by the composite components.
processStyles(tag, (styles) => styles.filter((style) => !style?.__mui_systemSx));

Expand Down Expand Up @@ -158,51 +158,56 @@ export default function createStyled(input = {}) {
...options,
});

const transformStyleArg = (stylesArg) => {
const transformStyleArg = (style) => {
// On the server Emotion doesn't use React.forwardRef for creating components, so the created
// component stays as a function. This condition makes sure that we do not interpolate functions
// which are basically components used as a selectors.
if (
(typeof stylesArg === 'function' && stylesArg.__emotion_real !== stylesArg) ||
isPlainObject(stylesArg)
) {
return (props) =>
processStyleArg(stylesArg, {
...props,
theme: resolveTheme({ theme: props.theme, defaultTheme, themeId }),
});
if ((typeof style === 'function' && style.__emotion_real !== style) || isPlainObject(style)) {
return (props) => processStyle(style, attachTheme(props, themeId, defaultTheme));
}
return stylesArg;
return style;
};
const muiStyledResolver = (styleArg, ...expressions) => {
let transformedStyleArg = transformStyleArg(styleArg);

const muiStyledResolver = (style, ...expressions) => {
let transformedStyle = transformStyleArg(style);
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 = attachTheme(props, themeId, defaultTheme);

// 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] = processStyle(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 });
if (!themeVariants) {
return null;
}
return processStyle(
{ variants: themeVariants },
attachTheme(props, themeId, defaultTheme),
);
});
}

Expand All @@ -212,13 +217,13 @@ export default function createStyled(input = {}) {

const numOfCustomFnsApplied = expressionsWithDefaultTheme.length - expressions.length;

if (Array.isArray(styleArg) && numOfCustomFnsApplied > 0) {
if (Array.isArray(style) && numOfCustomFnsApplied > 0) {
const placeholders = new Array(numOfCustomFnsApplied).fill('');
// If the type is array, than we need to add placeholders in the template for the overrides, variants and the sx styles.
transformedStyleArg = [...styleArg, ...placeholders];
transformedStyleArg.raw = [...styleArg.raw, ...placeholders];
transformedStyle = [...style, ...placeholders];
transformedStyle.raw = [...style.raw, ...placeholders];
}
const Component = defaultStyledResolver(transformedStyleArg, ...expressionsWithDefaultTheme);
const Component = defaultStyledResolver(transformedStyle, ...expressionsWithDefaultTheme);

if (process.env.NODE_ENV !== 'production') {
let displayName;
Expand All @@ -244,4 +249,32 @@ export default function createStyled(input = {}) {

return muiStyledResolver;
};

return styled;
}

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! :)

}
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);
}