-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[PoC] perf: Sync state back to JS + clear prop registry #7601
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
base: main
Are you sure you want to change the base?
[PoC] perf: Sync state back to JS + clear prop registry #7601
Conversation
This PoC commit shows an example for syncing our state from the UI state back to JS. This enables us to remove nodes from the prop registry. This in turn is a performance improvement as for nodes that are not animating right now we don't need to necessarily clone those
|
Hey @hannojg! Thanks for your proposal. I appreciate your effort and all the suggestions you made. Although your suggested changes look promising, I feel that they make the current already complex approach even more complicated. We are currently working on the new approach in which we are trying to get rid of all these registries and (if possible) remove or simplify the commit and the mount hook. You can see the current work on the I hope that the new approach we are trying to implement would solve many issues and reduce the code complexity, which becomes unmaintainable with more registries being added. Anyways, thanks a lot for your effort. We will consider changes you proposed and maybe implement them in this or a similar form if we don't succeed implementing the new approach (from the |
|
@hannojg I'd like to revisit this draft PR. I've merged current main but wasn't able to push commits to this PR. Could you please merge https://github.com/software-mansion/react-native-reanimated/commits/sync-state/ branch? Also, looks like this draft PR works only on Android because it's not possible to obtain |
|
@hannojg I've managed to revisit this draft PR. Here are my main concerns regarding the proposed changes:
This means we can't merge the PR in its current state. However, I'd like to confirm that the core idea of this draft PR (which is how to synchronize the animated styles back to React once the animations have settled using We already tested this draft PR on a large production app and managed to increase JS FPS from ~17 fps to ~56 fps while keeping stable 60 fps on the UI thread so we have really high hopes for this one. We also have some draft ideas on how to achieve the same effect without requiring a full React commit using the ShadowNode runtime reference update mechanism but this is yet to come as we want to land the draft PR in its current form as soon as possible. |
Summary
Addresses: #7480
This PR shows a PoC of how we can sync our state from the UI thread back to JS thread / react reconciler.
Consider the following example:
The more nodes we keep and clone the slower the function gets our performance profilings have shown, which is a major performance bottleneck.
We keep all props in the props registry so that when react is performing a commit from the JS thread we don't "drop" the changes we made on the UI thread.
This mechanism now allows us to update the views/nodes on JS, so we don't need to keep all props.
As updates in fabric are batched, even if we update a lot of nodes it should result only in one react commit.
To provide a performance measurement. We have a huge list where each item is animated in, so they are added to the prop registry. Scrolling became slow over time.
For this screen we had a 37.43% jank frame rate. With this change the jank frame rate dropped to 19.99%.
As a reference, when having completely disabling the commit/mount hooks the jank frame rate is 16.70%.
(I don't want to dilute the scope of this description too much, but we also added back the fast update path where we update none layout props directly instead of running through the commit pipeline, and landed on a jank frame rate of 12.86%)
Things to improve in this PR
ComponentRegistry.tsbut there might be existing registries / mechanisms that I could use?Test plan