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

[core] Refactor system theme props #43120

Merged
merged 7 commits into from
Aug 9, 2024
Merged

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Jul 30, 2024

Closes mui/mui-x#10033

When there are theme defaultProps, the return value of useThemeProps is not memoized and change on every render. The grid passes those props down to is subcomponents through context, so it re-renders everything on every render.

This PR makes the return props more stable.

@romgrk romgrk added the core Infrastructure work going on behind the scenes label Jul 30, 2024
@romgrk romgrk changed the base branch from next to master July 30, 2024 14:41
Copy link
Contributor Author

@romgrk romgrk Jul 30, 2024

Choose a reason for hiding this comment

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

As I was reading through the implementation, I noticed this function that is called in all our components via useThemeProps or other helpers had a few issues, I did a drive-by refactor here. As this is called in (I think) every render of every component we have when there are default props, I prioritized a leaner and more efficient implementation.

Related PR, for context: #35477

const defaultSlotProps = (defaultProps[propName] || {}) as T[keyof T];
const slotProps = props[propName] as {} as T[keyof T];
output[propName] = {} as T[keyof T];
for (const key in defaultProps) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched the Object.keys for a for ... in loop, which is more efficient.

@mui-bot
Copy link

mui-bot commented Jul 30, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against c0ac96e

if (Object.prototype.hasOwnProperty.call(defaultProps, key)) {
const propName = key as keyof T;

if (propName === 'components' || propName === 'slots') {
Copy link
Contributor Author

@romgrk romgrk Jul 30, 2024

Choose a reason for hiding this comment

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

Same for the regex name check vs string comparison. Regexes are quite efficient on modern engines but there is still some overhead: https://jsben.ch/iNyQn

I also find this a bit more readable.

Comment on lines -28 to -30
if (!slotProps || !Object.keys(slotProps)) {
// Reduce the iteration if the slot props is empty
output[propName] = defaultSlotProps;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

!Object.keys(anything) is always going to be false, because it's equivalent to ![]. I'm guessing this was meant to be !Object.key(x).length. Anyway, I've replaced with a simpler version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did one last change, removed the empty object check optimization. Avoiding iterating an empty object isn't an interesting optimization, and it's an unlikely case, and less code is always a win.

...(output[propName] as any),
};
} else if (propName.toString().match(/^(componentsProps|slotProps)$/)) {
const defaultSlotProps = (defaultProps[propName] || {}) as T[keyof T];
Copy link
Contributor Author

@romgrk romgrk Jul 30, 2024

Choose a reason for hiding this comment

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

Setting defaultSlotProps to an empty object by default (the || {} part) makes the check that follows on line 31 (else if (!defaultSlotProps || !Object.keys(defaultSlotProps))) useless. In addition to the !Object.keys(x) pattern that is also present on line 28.

} else if (propName.toString().match(/^(componentsProps|slotProps)$/)) {
const defaultSlotProps = (defaultProps[propName] || {}) as T[keyof T];
const slotProps = props[propName] as {} as T[keyof T];
output[propName] = {} as T[keyof T];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This property is redefined in all 3 branches below, which are exhaustive, so the empty object allocated here is never used.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Appreciate the perf improvement! Can you change the target branch to next?.

Please verify the change on MUI X before merging.

@siriwatknp siriwatknp changed the base branch from master to next July 31, 2024 02:44
@siriwatknp siriwatknp changed the base branch from next to master July 31, 2024 02:45
@romgrk romgrk changed the base branch from master to next July 31, 2024 03:00
@romgrk
Copy link
Contributor Author

romgrk commented Jul 31, 2024

Updated the branch. What do you mean by verifying on MUI X? I used the following test code to ensure the props are now stable: https://codesandbox.io/s/angry-cray-gwhs5t

And should I ignore the argos changes?

@@ -7,6 +8,5 @@ export default function useThemeProps({ props, name, defaultTheme, themeId }) {
if (themeId) {
theme = theme[themeId] || theme;
}
const mergedProps = getThemeProps({ theme, name, props });
return mergedProps;
return React.useMemo(() => getThemeProps({ theme, name, props }), [theme, name, props]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what's the stability of props itself here ?
Have you done any benchmark for this hook ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the grid props is stable because we React.memo (or more exactly fastMemo) most of our components including the root one, for non-memoized components it would be stable only if the component is re-rendering because one of its hooks changed, but it wouldn't be stable if it re-renders because its parent re-rendered (the most common case I think).

Now that I think, I guess it would be possible to add the React.useMemo outside useThemeProps (in the mui-x codebase), it would avoid the cost for the rest of the components. It does feel a bit cleaner to have a stable output, but it's more performant to add it only where needed. I could be convinced either way. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also assume that most of the times, parent will re-render making the memo useless in this case.
Only of it was using some specific keys from props instead of the whole object.

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 thought about it some more and I prefer to add the memoization only where it's needed, so I've removed this part. The only thing left is the refactor for resolveProps, but it's a good refactor as there were some issues with the code there, so I'll merge this PR anyway.

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'm going to proceed with the plan above, but might need to come back. Can't wrap useThemeProps with useMemo because it is itself a hook (calling useContext), and needs to be reactively bound to the theme context change.

So useMemo(() => useThemeProps({ name, props }), [name, props])) is incorrect. I have more thinking to do because for the design-system agnostic objective for the grid I need to get rid of the dependency on useThemeProps anyway, so I'll do the thinking and merge this small refactor as-is.

@siriwatknp
Copy link
Member

And should I ignore the argos changes?

Yes, looks like the changes comes from the Inter font which is external.

@siriwatknp
Copy link
Member

Updated the branch. What do you mean by verifying on MUI X? I used the following test code to ensure the props are now stable: https://codesandbox.io/s/angry-cray-gwhs5t

Thanks.

@mnajdova
Copy link
Member

mnajdova commented Aug 2, 2024

@romgrk I merged the next branch to see if the argos tests will pass.

@romgrk romgrk enabled auto-merge (squash) August 5, 2024 13:15
@romgrk romgrk disabled auto-merge August 8, 2024 13:05
@romgrk romgrk changed the title [core] Fix system theme props [core] Refactor system theme props Aug 8, 2024
Copy link
Contributor

@brijeshb42 brijeshb42 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] Default theme props degrade performance
5 participants