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

Use commit hook to update props on Fabric #4075

Merged
merged 34 commits into from
Jun 21, 2023

Conversation

tomekzaw
Copy link
Member

@tomekzaw tomekzaw commented Feb 20, 2023

Summary

This draft PR introduces a new approach towards updating layout props on Fabric that uses UIManagerCommitHook. Previously, we would keep a registry of most recent ShadowNodes. Each time RN or Reanimated cloned a ShadowNode, we would retrieve the most recent version of it from the registry as well as store the new copy. Obviously, this required synchronization at the level of each cloneNode call so rendering large trees was slow. The new approach stores all most recent props of animated components in PropsRegistry and applies them on each RN render using ReanimatedCommitHook all at once which improves the performance significantly.

Changes:

  • Removed ReanimatedUIManagerBinding
  • Added ReanimatedCommitHook
  • Converted NewestShadowNodesRegistry into PropsRegistry
  • Converted _removeShadowNodeFromRegistry into _removeFromPropsRegistry
  • Updated initialization flow

Requires:

Things to consider:

  • Commit an identical tree and let ReanimatedCommitHook handle all updates instead of calling isThereAnyLayoutProp, enqueuing update in operationsInBatch_ and updating lastReanimatedRoot but modifying only necessary ShadowNodes?
  • Copy propsRegistry.map_ in ReanimatedCommitHook to prevent locking the UI thread while the background thread runs the commit hook?

Detected problems:

  • If calculating layout of RN tree takes more than one frame, Reanimated will skip the commit before RN can commit its own tree and the problem still persists
  • If calculating layout of RN tree takes more than one frame, Reanimated will run the animations in the meantime and finally RN will mount outdated tree
  • When there's just a few layout props changes and many non-layout props changes (e.g. emoji shower and progress bar), we still apply all changes via commit (this is to enforce UI consistency).
  • On each RN render (even if only one component is changed) we need to apply all changes from the whole PropsRegistry (which contains all currently mounted animated components).

Review

Since this PR affects 31 files, it is recommended to review the commits one-by-one.

Test plan

Launch FabricExample and open the following examples: chessboard, width, ref

@tomekzaw tomekzaw marked this pull request as ready for review April 7, 2023 16:34
Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

Looks good. Would be even better if you could write 2-3 sentences in the description to explain how this new approach works.

@@ -43,7 +42,8 @@ - (void)_tryAndHandleError:(dispatch_block_t)block;
@implementation REAModule {
#ifdef RCT_NEW_ARCH_ENABLED
__weak RCTSurfacePresenter *_surfacePresenter;
std::shared_ptr<NewestShadowNodesRegistry> newestShadowNodesRegistry;
std::shared_ptr<PropsRegistry> propsRegistry_;
std::shared_ptr<ReanimatedCommitHook> commitHook_;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to retain commitHook as a class member here? Looks like it is not being used except from passing it to uimanager. Same applies in NativeProxy implementation for Android

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to store it because React Native's UIManager keeps a vector of raw pointers (see here).

@tomekzaw tomekzaw added this pull request to the merge queue Jun 21, 2023
Merged via the queue into main with commit 7b43e32 Jun 21, 2023
16 checks passed
@tomekzaw tomekzaw deleted the @tomekzaw/fabric-layout-props-v2 branch June 21, 2023 14:50
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.

None yet

2 participants