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] Remove theme/styling allocations #43372

Merged
merged 20 commits into from
Aug 21, 2024
Merged

[system] Remove theme/styling allocations #43372

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 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
f34a771
perf: avoid theme allocations
romgrk Aug 19, 2024
0fdf5d5
fix
romgrk Aug 19, 2024
d007f38
Merge branch 'next' into perf-theme-allocations
romgrk Aug 20, 2024
d4c3f0e
refactor: rename pure => fromTheme
romgrk Aug 20, 2024
79137a5
lint
romgrk Aug 20, 2024
c19dd34
feat: do the thing
romgrk Aug 20, 2024
e2f758e
lint
romgrk Aug 20, 2024
ee66b56
Update packages/mui-material/src/utils/memoTheme.ts
romgrk Aug 20, 2024
3939cdb
lint
romgrk Aug 20, 2024
95e4802
Merge branch 'perf-theme-allocations' of github.com:romgrk/material-u…
romgrk Aug 20, 2024
8921455
Merge branch 'next' into perf-theme-allocations
romgrk Aug 20, 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
9 changes: 5 additions & 4 deletions packages/mui-material/src/Accordion/Accordion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import clsx from 'clsx';
import chainPropTypes from '@mui/utils/chainPropTypes';
import composeClasses from '@mui/utils/composeClasses';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import Collapse from '../Collapse';
import Paper from '../Paper';
Expand Down Expand Up @@ -46,7 +47,7 @@ const AccordionRoot = styled(Paper, {
];
},
})(
({ theme }) => {
memoTheme(({ theme }) => {
const transition = {
duration: theme.transitions.duration.shortest,
Comment on lines +50 to 52
Copy link
Contributor Author

@romgrk romgrk Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's a problem with prettier, it's going to forcefully increase the indentation level everywhere inside memoTheme(({ theme }) => /* here */), which is one of the reasons I dislike prettier. Anyway, I've left the PR without prettifying it to ease the review, but I'll run it as a last step before merging.

Comment on lines 47 to 52
Copy link
Member

@oliviertassinari oliviertassinari Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are we sure about the DX of this? As a developer looking at the source:https://github.com/romgrk/material-ui/blob/8921455731c32f13dd9d61fa49002feb21b673fa/packages/mui-material/src/Accordion/Accordion.js#L36, the first thing I ask myself: why is the source verbose like this? Couldn't this be a flag?

 const AccordionRoot = styled(Paper, {
   name: 'MuiAccordion',
   slot: 'Root',
+  onlyTheme: true,
   overridesResolver: (props, styles) => {
     const { ownerState } = props;

     return [
       { [`& .${accordionClasses.region}`]: styles.region },
       styles.root,
       !ownerState.square && styles.rounded,
       !ownerState.disableGutters && styles.gutters,
     ];
   },
 })(
  ({ theme }) => {

If we had a transpilation step, it could automatically detect those and add the flag 😄, this way, nobody will forget about it, or use it wrong.

Copy link
Contributor Author

@romgrk romgrk Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It could have been a flag, I didn't think about it, but I didn't think about it too much because it can be refactored anytime and I want to iterate on this further.

Emotion is serializing+hashing those style objects on every render of every component, even if we return the same styling object. For internal components we can know, either with the flag on the styled option or a flag on the memoTheme return value (e.g. make it return a function with a .memoized = true flag), that all the styling only needs to react to theme changes. This means we could use emotion's serializeStyles when we memoize the style value, and return the serialized object directly, which emotion will handle here and skip the createStringFromObject on every render/component.

The only thing I need to figure out is variants, we probably need to transfer them from our style value to emotion's serialized style object.

One thing though, the flag would need to affect typings, I hope it's not too complex to setup.

Copy link
Contributor Author

@romgrk romgrk Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I remember why I picked a function though, initially I also had memoProps that would memo the value based on which props were accessed (in addition to memoTheme), so it was possible to have for the same component style functions that accessed the props and some that didn't (like the Grid).

I didn't end up including memoProps because the implementation was a bit complex (used a proxy to cache which props values were accessed, like SignalJS props work), and most components didn't access the props so the performance gains would have been lower, though I didn't benchmark the difference. That could improve Grid though.

Copy link
Member

@oliviertassinari oliviertassinari Aug 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This means we could use emotion's serializeStyles when we memoize the style value, and return the serialized object directly, which emotion will handle here and skip the createStringFromObject on every render/component.

Oh, nice, like it used to be in Material UI v4 with JSS. Styles created once per component type.

The only thing I need to figure out is variants, we probably need to transfer them from our style value to emotion's serialized style object.

Maybe we can create custom class name for each variant and apply them conditionally, like Pigment CSS is doing or like we were doing in Material UI v4. The difference now is that we have CSS variables, we have fewer variants to create, e.g. only one for color and not one for color primary, color secondary, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up here: #43412

};
Expand Down Expand Up @@ -91,8 +92,8 @@ const AccordionRoot = styled(Paper, {
backgroundColor: (theme.vars || theme).palette.action.disabledBackground,
},
};
},
({ theme }) => ({
}),
memoTheme(({ theme }) => ({
variants: [
{
props: (props) => !props.square,
Expand Down Expand Up @@ -122,7 +123,7 @@ const AccordionRoot = styled(Paper, {
},
},
],
}),
})),
);

const AccordionHeading = styled('h3', {
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/AccordionDetails/AccordionDetails.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
import clsx from 'clsx';
import composeClasses from '@mui/utils/composeClasses';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import { getAccordionDetailsUtilityClass } from './accordionDetailsClasses';

Expand All @@ -21,9 +22,9 @@ const AccordionDetailsRoot = styled('div', {
name: 'MuiAccordionDetails',
slot: 'Root',
overridesResolver: (props, styles) => styles.root,
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
padding: theme.spacing(1, 2, 2),
}));
})));

const AccordionDetails = React.forwardRef(function AccordionDetails(inProps, ref) {
const props = useDefaultProps({ props: inProps, name: 'MuiAccordionDetails' });
Expand Down
13 changes: 7 additions & 6 deletions packages/mui-material/src/AccordionSummary/AccordionSummary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
import clsx from 'clsx';
import composeClasses from '@mui/utils/composeClasses';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import ButtonBase from '../ButtonBase';
import AccordionContext from '../Accordion/AccordionContext';
Expand All @@ -28,7 +29,7 @@ const AccordionSummaryRoot = styled(ButtonBase, {
name: 'MuiAccordionSummary',
slot: 'Root',
overridesResolver: (props, styles) => styles.root,
})(({ theme }) => {
})(memoTheme(({ theme }) => {
const transition = {
duration: theme.transitions.duration.shortest,
};
Expand Down Expand Up @@ -58,13 +59,13 @@ const AccordionSummaryRoot = styled(ButtonBase, {
},
],
};
});
}));

const AccordionSummaryContent = styled('div', {
name: 'MuiAccordionSummary',
slot: 'Content',
overridesResolver: (props, styles) => styles.content,
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
display: 'flex',
flexGrow: 1,
margin: '12px 0',
Expand All @@ -81,13 +82,13 @@ const AccordionSummaryContent = styled('div', {
},
},
],
}));
})));

const AccordionSummaryExpandIconWrapper = styled('div', {
name: 'MuiAccordionSummary',
slot: 'ExpandIconWrapper',
overridesResolver: (props, styles) => styles.expandIconWrapper,
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
display: 'flex',
color: (theme.vars || theme).palette.action.active,
transform: 'rotate(0deg)',
Expand All @@ -97,7 +98,7 @@ const AccordionSummaryExpandIconWrapper = styled('div', {
[`&.${accordionSummaryClasses.expanded}`]: {
transform: 'rotate(180deg)',
},
}));
})));

const AccordionSummary = React.forwardRef(function AccordionSummary(inProps, ref) {
const props = useDefaultProps({ props: inProps, name: 'MuiAccordionSummary' });
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/Alert/Alert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import clsx from 'clsx';
import composeClasses from '@mui/utils/composeClasses';
import { darken, lighten } from '@mui/system/colorManipulator';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import useSlot from '../utils/useSlot';
import capitalize from '../utils/capitalize';
Expand Down Expand Up @@ -47,7 +48,7 @@ const AlertRoot = styled(Paper, {
styles[`${ownerState.variant}${capitalize(ownerState.color || ownerState.severity)}`],
];
},
})(({ theme }) => {
})(memoTheme(({ theme }) => {
const getColor = theme.palette.mode === 'light' ? darken : lighten;
const getBackgroundColor = theme.palette.mode === 'light' ? lighten : darken;
return {
Expand Down Expand Up @@ -112,7 +113,7 @@ const AlertRoot = styled(Paper, {
})),
],
};
});
}));

const AlertIcon = styled('div', {
name: 'MuiAlert',
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/AlertTitle/AlertTitle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
import clsx from 'clsx';
import composeClasses from '@mui/utils/composeClasses';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import Typography from '../Typography';
import { getAlertTitleUtilityClass } from './alertTitleClasses';
Expand All @@ -22,12 +23,12 @@ const AlertTitleRoot = styled(Typography, {
name: 'MuiAlertTitle',
slot: 'Root',
overridesResolver: (props, styles) => styles.root,
})(({ theme }) => {
})(memoTheme(({ theme }) => {
return {
fontWeight: theme.typography.fontWeightMedium,
marginTop: -2,
};
});
}));

const AlertTitle = React.forwardRef(function AlertTitle(inProps, ref) {
const props = useDefaultProps({
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/AppBar/AppBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
import clsx from 'clsx';
import composeClasses from '@mui/utils/composeClasses';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import capitalize from '../utils/capitalize';
import Paper from '../Paper';
Expand Down Expand Up @@ -35,7 +36,7 @@ const AppBarRoot = styled(Paper, {
styles[`color${capitalize(ownerState.color)}`],
];
},
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
display: 'flex',
flexDirection: 'column',
width: '100%',
Expand Down Expand Up @@ -159,7 +160,7 @@ const AppBarRoot = styled(Paper, {
},
},
],
}));
})));

const AppBar = React.forwardRef(function AppBar(inProps, ref) {
const props = useDefaultProps({ props: inProps, name: 'MuiAppBar' });
Expand Down
25 changes: 13 additions & 12 deletions packages/mui-material/src/Autocomplete/Autocomplete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import filledInputClasses from '../FilledInput/filledInputClasses';
import ClearIcon from '../internal/svg-icons/Close';
import ArrowDropDownIcon from '../internal/svg-icons/ArrowDropDown';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import autocompleteClasses, { getAutocompleteUtilityClass } from './autocompleteClasses';
import capitalize from '../utils/capitalize';
Expand Down Expand Up @@ -278,7 +279,7 @@ const AutocompletePopper = styled(Popper, {
ownerState.disablePortal && styles.popperDisablePortal,
];
},
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
zIndex: (theme.vars || theme).zIndex.modal,
variants: [
{
Expand All @@ -288,40 +289,40 @@ const AutocompletePopper = styled(Popper, {
},
},
],
}));
})));

const AutocompletePaper = styled(Paper, {
name: 'MuiAutocomplete',
slot: 'Paper',
overridesResolver: (props, styles) => styles.paper,
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
...theme.typography.body1,
overflow: 'auto',
}));
})));

const AutocompleteLoading = styled('div', {
name: 'MuiAutocomplete',
slot: 'Loading',
overridesResolver: (props, styles) => styles.loading,
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
color: (theme.vars || theme).palette.text.secondary,
padding: '14px 16px',
}));
})));

const AutocompleteNoOptions = styled('div', {
name: 'MuiAutocomplete',
slot: 'NoOptions',
overridesResolver: (props, styles) => styles.noOptions,
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
color: (theme.vars || theme).palette.text.secondary,
padding: '14px 16px',
}));
})));

const AutocompleteListbox = styled('div', {
name: 'MuiAutocomplete',
slot: 'Listbox',
overridesResolver: (props, styles) => styles.listbox,
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
listStyle: 'none',
margin: 0,
padding: '8px 0',
Expand Down Expand Up @@ -385,16 +386,16 @@ const AutocompleteListbox = styled('div', {
},
},
},
}));
})));

const AutocompleteGroupLabel = styled(ListSubheader, {
name: 'MuiAutocomplete',
slot: 'GroupLabel',
overridesResolver: (props, styles) => styles.groupLabel,
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
backgroundColor: (theme.vars || theme).palette.background.paper,
top: -8,
}));
})));

const AutocompleteGroupUl = styled('ul', {
name: 'MuiAutocomplete',
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/Avatar/Avatar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
import clsx from 'clsx';
import composeClasses from '@mui/utils/composeClasses';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import Person from '../internal/svg-icons/Person';
import { getAvatarUtilityClass } from './avatarClasses';
Expand Down Expand Up @@ -33,7 +34,7 @@ const AvatarRoot = styled('div', {
ownerState.colorDefault && styles.colorDefault,
];
},
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
position: 'relative',
display: 'flex',
alignItems: 'center',
Expand Down Expand Up @@ -75,7 +76,7 @@ const AvatarRoot = styled('div', {
},
},
],
}));
})));

const AvatarImg = styled('img', {
name: 'MuiAvatar',
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/AvatarGroup/AvatarGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import clsx from 'clsx';
import chainPropTypes from '@mui/utils/chainPropTypes';
import composeClasses from '@mui/utils/composeClasses';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import Avatar, { avatarClasses } from '../Avatar';
import avatarGroupClasses, { getAvatarGroupUtilityClass } from './avatarGroupClasses';
Expand Down Expand Up @@ -34,7 +35,7 @@ const AvatarGroupRoot = styled('div', {
[`& .${avatarGroupClasses.avatar}`]: styles.avatar,
...styles.root,
}),
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
display: 'flex',
flexDirection: 'row-reverse',
[`& .${avatarClasses.root}`]: {
Expand All @@ -45,7 +46,7 @@ const AvatarGroupRoot = styled('div', {
marginLeft: 0,
},
},
}));
})));

const AvatarGroup = React.forwardRef(function AvatarGroup(inProps, ref) {
const props = useDefaultProps({
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/Badge/Badge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import composeClasses from '@mui/utils/composeClasses';
import useSlotProps from '@mui/utils/useSlotProps';
import useBadge from './useBadge';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import capitalize from '../utils/capitalize';
import badgeClasses, { getBadgeUtilityClass } from './badgeClasses';
Expand Down Expand Up @@ -65,7 +66,7 @@ const BadgeBadge = styled('span', {
ownerState.invisible && styles.invisible,
];
},
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
display: 'flex',
flexDirection: 'row',
flexWrap: 'wrap',
Expand Down Expand Up @@ -236,7 +237,7 @@ const BadgeBadge = styled('span', {
},
},
],
}));
})));

const Badge = React.forwardRef(function Badge(inProps, ref) {
const props = useDefaultProps({ props: inProps, name: 'MuiBadge' });
Expand Down
5 changes: 3 additions & 2 deletions packages/mui-material/src/BottomNavigation/BottomNavigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import PropTypes from 'prop-types';
import clsx from 'clsx';
import composeClasses from '@mui/utils/composeClasses';
import { styled } from '../zero-styled';
import memoTheme from '../utils/memoTheme';
import { useDefaultProps } from '../DefaultPropsProvider';
import { getBottomNavigationUtilityClass } from './bottomNavigationClasses';

Expand All @@ -22,12 +23,12 @@ const BottomNavigationRoot = styled('div', {
name: 'MuiBottomNavigation',
slot: 'Root',
overridesResolver: (props, styles) => styles.root,
})(({ theme }) => ({
})(memoTheme(({ theme }) => ({
display: 'flex',
justifyContent: 'center',
height: 56,
backgroundColor: (theme.vars || theme).palette.background.paper,
}));
})));

const BottomNavigation = React.forwardRef(function BottomNavigation(inProps, ref) {
const props = useDefaultProps({ props: inProps, name: 'MuiBottomNavigation' });
Expand Down
Loading