react-native-reanimated icon indicating copy to clipboard operation
react-native-reanimated copied to clipboard

Make sure style does not regenerate between renders if it is the same.

Open larkox opened this issue 2 years ago • 3 comments

Description

This PR deals with one performance issue generated by creating a new style object on every render.

Any re-rendering of the parent component may make this component to re-render, and it will generate a brand new style property. This calls the re-rendering from the whole tree below this. This can be specially troublesome with components like VirtualizedLists (which have right now performance problems of their own https://github.com/facebook/react-native/pull/31327 ).

Changes

  • Added a new property to store the last sent style of the component.
  • Added checks to see if the new resulting style has changed from the last style sent.
  • Added another check to make sure any removed variable from the style is also not present in the last sent style.

Test code and steps to reproduce

Flipper can be used to track the re-renders and the reasons. Any simple three level design should be able to reproduce this error. For example:

const Level3 = React.memo(({style}) => (<View style={style} />))
const Level2 = ({style}) => (<Level3 style={style} />)
const AnimatedLevel2 = createAnimatedComponent(View)
const Level1 = () => (<AnimatedLevel2 style={{height:100}} />)

If we re-render the Level1, Level3 should not be re-rendered.

Checklist

  • [ ] Included code example that can be used to test this change
  • [ ] Updated TS types
  • [ ] Added TS types tests
  • [ ] Added unit / integration tests
  • [ ] Updated documentation
  • [ ] Ensured that CI passes

larkox avatar Apr 01 '22 10:04 larkox

@piaskowyk Would you consider this PR for the next release? I think the changes will be quite helpful to improve app performance.

AlexanderEggers avatar Apr 11 '22 15:04 AlexanderEggers

@AlexanderEggers I will try, but I see here a potential issue with the race condition of the update on the UI thread (mappers). I need to prepare some tests for this PR.

piaskowyk avatar Apr 11 '22 19:04 piaskowyk

Just stumbled upon this issue myself. This is generating significant re-renders on my app since I started using Layout Animations in various core components. I am happy to have found this PR so I can at least patch locally :)

derekstavis avatar Oct 07 '22 06:10 derekstavis

@terrysahaidak Any news here?

larkox avatar Oct 24 '22 09:10 larkox

@piaskowyk Any news regarding this PR?

larkox avatar Dec 07 '22 10:12 larkox

Hey, I see problem that you don't handle nested properties for example transform: [{}, {}]. Did you test what happens if a render is triggered during animation running?

piaskowyk avatar Dec 19 '22 13:12 piaskowyk

After the changes on createAnimatedComponent, the changes on this PR no longer make sense. I am not sure if they are still needed (I think the style is being recreated in _filterNonAnimatedProps, but I would like to check this version working in flipper before trying to fix it).

larkox avatar Jan 20 '23 17:01 larkox