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

Improve useRestyle performance #131

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Conversation

sbalay
Copy link
Contributor

@sbalay sbalay commented Feb 23, 2022

Summary

  1. Instead of running composeRestyleFunctions on every render cycle, we can do it only once when creating the restyle component.

  2. Build the stylesheet based only on restyle props and not all the props received by the restyled component

  3. Build map used to filter between restyle props and ordinary props only once, and not on every render cycle

  4. Do not apply all restyle transformations to obtain the stylesheet, only apply the transformations that are needed based on the props that are present on the restyled component

I'll add further comments on the PR diff trying to explain these changes in more detail

Benchmark results

(Find the actual benchmark explanation below)

Noop rerender

Rerenders without changing the component tree turned out to be ~36% faster after these changes

Mount/Unmount 100 list items

Mounting and unmounting a list of 100 items turned out to be ~24% faster after these changes

Benchmark

To somehow measure how/if these changes improved performance or not, I built a benchmark app: https://github.com/sbalay/restyle-benchmark

In that app, we render a list of 100 items. Each item is a bunch of Box and Text components from restyle createBox and createText.

Test 1 (unmount/mount)

Unmount and remount the entire list and output the render cost of each operation using React's profiler API. At the end of the video I output the average cost of each update.

Before

20 operations, average cost 102ms

toggle.display.before.mp4

After

20 operations, average cost 77ms

toggle.display.after.mp4

Test 2 (noop rerender)

Trigger a noop rerender of the whole list (by changing state in the parent component). Same as before, measure using React Profiler and output averages at the end

Before

40 operations, average cost 44ms

noop.rerender.before.mp4

After

40 operations, average cost 28ms

noop.rerender.after.mp4

1. Instead of running composeRestyleFunctions on every render cycle, we
can do it only once when creating the restyle component.

2. Build the stylesheet based only on restyle props and not all the props received by the restyled component

3. Build map used to filter between restyle props and ordinary props only once, and not on every render cycle

4. Do not apply all restyle transformations to obtain the stylesheet, only apply the transformations that are needed based on the props that are present on the restyled component
props: TProps,
) => {
const theme = useTheme<Theme>();

const dimensions = useDimensions();

const restyled = useMemo(() => {
const composedRestyleFunction = composeRestyleFunctions(restyleFunctions);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

restyleFunctions are static values, so we don't need to compose them on every render cycle.

We can do that only once when calling createRestyledComponent instead

@@ -46,15 +48,20 @@ const composeRestyleFunctions = <
dimensions: Dimensions;
},
): RNStyle => {
const styles = funcs.reduce((acc, func) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each style function in funcs takes all the props of the component, applies a transformation and returns a style attribute that then will be used to create a stylesheet. If there is no value to transform, each function returns an empty object instead.

Here, we were running every style function every time even though, not all transformations were needed. Example:

<Box marginTop="m" />

Here we only need to transform marginTop="m" to marginTop: 16, however we run all the transformations registered for the Box component here: https://github.com/sbalay/restyle-benchmark/blob/master/src/restyle/createBox.ts#L47

So we also check for backgroundColor and return an empty object, we also check for marginBottom and returns an empty object, we also check for paddingHorizontal and so on.

With the change below, we only run transformation functions for the props that are present. In the example above we would only run the function that transforms marginTop="m" to {marginTop: 16}

@@ -13,24 +12,20 @@ const filterRestyleProps = <
TProps extends Record<string, unknown> & TRestyleProps
>(
props: TProps,
omitList: (keyof TRestyleProps)[],
): Omit<TProps, keyof TRestyleProps> => {
const omittedProp = omitList.reduce<Record<keyof TRestyleProps, boolean>>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every time we were going to filter the props to distinguish between restyle props and non restyle props, we would build this map.

Now, we build the map only once when calling createRestyledComponent and use that here.

props,
composedRestyleFunction.propertiesMap,
);
const style = composedRestyleFunction.buildStyle(restyleProps, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were calling composedRestyleFunction.buildStyle with all the props of the components, but we only need to call it with the props that belong to restyle.

@sbalay sbalay marked this pull request as ready for review February 23, 2022 12:58
Copy link
Contributor

@jamesism jamesism left a comment

Choose a reason for hiding this comment

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

This looks great to me! Your comments were really helpful, and the code changes make sense to me. I was able to replicate your performance improvement results, and everything looked great in a pretty thorough tophat. Great work!!!

@sbalay sbalay merged commit 963f2d3 into master Feb 24, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to production February 24, 2022 10:39 Inactive
@gvarandas gvarandas deleted the chore/perf-restyle-functions-compose branch March 19, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants