Replace styled-components attrs to fix performance issue
#7304
Conversation
Signed-off-by: Jessica Filben <54560994+jcfilben@users.noreply.github.com>
|
Just to summarize for my own understanding -- If we are using v2.38.0 as a baseline of 587ms:
If that's correct, this looks like a good solution. |
There was a problem hiding this comment.
Looks good to me!
One idea for ease of changes in future would be to abstract some of the logic into a custom hook like we have for useThemeValue. Not sure if this logic is perfect, but just a concept-- and not sure if there's any performance downsides that come with this approach or not.
// in component files
const themeContext = useThemeContext(theme);
<Component
{...themeContext}
// in utils files
const useThemeContext = (theme) => {
const existingThemeContext = useContext(ThemeContext);
return existingThemeContext === undefined ? { theme } : {}
}
That said, the work is complete on the PR and functionality seems to be working as expected, so soft opinion if we do any cleanups or just keep it as it is.
Good idea, I agree this would be cleaner. For now I left as is, maybe we can just try and get this update out and then handle this step in a separate PR? |
I think it is also helpful to looks at the full timeline, |
src/js/components/Anchor/Anchor.js
Outdated
| @@ -105,6 +107,7 @@ const Anchor = forwardRef( | |||
| if (onBlur) onBlur(event); | |||
| }} | |||
| size={sizeProp || size} | |||
| {...(withinThemeContext === undefined ? { theme } : {})} | |||
There was a problem hiding this comment.
This pattern is repeated way too much and is awkward so it's intent is hard to understand.
Also, if a theme prop is passed in, doesn't it end up in rest and get overwritten by the theme from the context if there is one?
I think a pattern of something like:
const theme = useTheme(themeProp);
<StyledAnchor theme={theme} ... />Is much cleaner assuming the useTheme does the useContext(ThemeContext) and logic to reconcile the theme prop from the component and theme from the theme context if needed.
This also could be done to prevent the theme parameter from changing each render (your ternary create two new objects each render)
There was a problem hiding this comment.
Also, if a theme prop is passed in, doesn't it end up in rest and get overwritten by the theme from the context if there is one?
I tried to keep {...(withinThemeContext === undefined ? { theme } : {})} above {...rest} so that rest would override the theme, looks like I actually missed this on Anchor though.
I think a pattern of something like:
const theme = useTheme(themeProp);
<StyledAnchor theme={theme} ... />
Is much cleaner assuming the useTheme does the useContext(ThemeContext) and logic to reconcile the theme prop from the component and theme from the theme context if needed.
I wanted to avoid having an explicit themeProp on each component and passing through the theme on each styled component if we don't need it (if we are within a theme context already). I'm not sure performance wise if it matters or not if we pass a large object (the theme) as a prop for each styled component regardless of whether we are within a theme context or not.
This also could be done to prevent the theme parameter from changing each render (your ternary create two new objects each render)
This is a good point, I hadn't thought about that cost
I'll add that performance is an area I'm not super familiar with so definitely open to other perspectives and open to going this route, just wanted to share my thought process of why I approached this the way that I did.
There was a problem hiding this comment.
I'm not sure what you're concerned with as far as performance of passing a theme prop. It seems like you're already doing that in most of the cases via the value from useThemeValue and passing it to the styled component (like Anchor here.) I would think in most cases you'd get the benefit with no real cost if useThemeValue() accepted a themeProp arg and did the logic discussed. Components that use useThemeValue and others could easily just check their props for the theme prop and pass it along. I don't think passing a large object is expensive (ie. it's a reference unless you're doing something expensive like a merge.)
There was a problem hiding this comment.
Ok sounds good, I'll work on making this change
There was a problem hiding this comment.
I implemented this change but I'm seeing worse performance on the com repo compared to this branch as it currently is. Maybe if someone is willing they could run on their end as well just to validate that they are getting the similar results as I am. I'll push up the changes in a new branch so it is easier to compare.
Here is what I'm seeing with this approach:
Com sample 1 with alt branch (#7306)
Com sample 2 with alt branch (#7306)
Com sample 3 with alt branch (#7306)
There was a problem hiding this comment.
I just tested this current branch again to see if me being in a different location on different internet was causing a problem, it looks like it is not. Here is what I got
Com sample 1 with this branch:
Com sample 2 with this branch
There was a problem hiding this comment.
To summarize the results that I am getting:
This branch with withinThemeContext approach = ~1500ms
Alternate branch #7306 with themeProp approach = ~2500ms
Again would be good to have someone else validate these results just to make sure we are seeing the same thing
There was a problem hiding this comment.
I just tested this current branch again to see if me being in a different location on different internet was causing a problem, it looks like it is not. Here is what I got
Can you please label these images with product + approach/version so that we know what we are comparing? Also, we need the equivalent for v2.38 so that we have our baseline.
There was a problem hiding this comment.
Yeah I'll add more detailed labels, baseline can be found in the PR description
There was a problem hiding this comment.
Com, grommet 2.38.0 tested at new location
v2.38.0 (forgot to grab screenshots)
Mean - 766 PR #7304 (This PR)
Mean - 703.3 PR #7306
Mean - 658.7 TakeawaysBoth PR branches performed comparably to v2.38.0 and better than v2.39.0. |
|
I'm back at home today and I tested again and now I'm getting similar results to Taylor. I can publish my results soon. Yesterday my internet was definitely slower. I wonder if the speed discrepancy between approaches I was seeing is only apparent on slower internet speeds? Or maybe my results yesterday were a fluke, not sure. |
v2.38.0coming soon... PR #7304 (This PR)
Mean 1706.4 PR #7306
Mean - 1675.8 |
|
Update 9/9/24 I chatted with the rest of the dev team, some notes from that discussion:
|
Performance snapshots on com after 'simplify code' commit 9/10/24
Mean - 2550.0 2.38.0 snapshots on com 9/10/24
Mean - 2786.5 |
| .withConfig(styledComponentsConfig) | ||
| .attrs((props) => ({ | ||
| ...withTheme(props), | ||
| 'aria-label': props.a11yTitleProp, |
There was a problem hiding this comment.
do we need to add this or its fine being removed? not sure why it was pulled out but we no longer have aria-label in this file just making sure that is intentional?
There was a problem hiding this comment.
This change is intentional, in the Grid.js file I renamed a11yTitleProp to aria-label so that it will pass through correctly without needing 'aria-label': props.a11yTitleProp in attrs.
What does this PR do?
Remove
attrsand instead pass through the theme whe we are outside of a ThemeContext via props.Where should the reviewer start?
What testing has been done on this PR?
Performance on COM with this branch
Performance on COm with grommet 2.38.0 (pre attrs)
Performance on COM with grommet 2.39.0 (attrs)
How should this be manually tested?
Do Jest tests follow these best practices?
screenis used for querying.asFragment()is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Closes grommet/hpe-design-system#4135
Screenshots (if appropriate)
Do the grommet docs need to be updated?
no
Should this PR be mentioned in the release notes?
yes
Is this change backwards compatible or is it a breaking change?
backwards compatible