Modify reanimated props updates #6330
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
bartlomiejbloniarz
requested review from
piaskowyk,
tjzel and
tomekzaw
and removed request for
piaskowyk
tomekzaw
requested changes
Jul 29, 2024
packages/react-native-reanimated/Common/cpp/Fabric/ReanimatedMountHook.cpp
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/Fabric/ReanimatedCommitHook.cpp
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/Fabric/ReanimatedCommitShadowNode.h
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/Fabric/ReanimatedCommitShadowNode.h
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/Fabric/ReanimatedCommitShadowNode.h
Show resolved
Hide resolved
tomekzaw
approved these changes
Jul 30, 2024
packages/react-native-reanimated/Common/cpp/Fabric/ReanimatedCommitShadowNode.h
Outdated
Show resolved
Hide resolved
tomekzaw
approved these changes
Jul 30, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 16, 2024
## Summary When working on #6330 I missed the fact that the `pleaseSkipReanimatedCommit` mechanism is not only a performance optimization (preventing us from applying some changes twice a frame). It's main role is to actually prevent reanimated from starving RN out. It could happen that RN is trying to apply some heavy commit while an animation is running. If during that commit reanimated applies its own changes, then the RN commit will fail and another attempt will be made. Since reanimated runs a commit in every frame, then the RN commit might continue failing until the animation is over, which is very very very bad. In this PR I changed the "skip" name to "pause" since it better describes the intent behind it. I also reverted the change where the skip flag was reset in `performOperations`. This was done to ensure that if some changes are applied through `updateProps` after the commit hook has already gathered all updates, but before reanimated commits have been unpaused, then these changes are not lost. Unfortunately this would lead to reanimated blocking heavy RN commits from being applied. Now the skip flag is reset in the `ReanimatedMountHook` (as it was before #6330), and if there are any changes that would be lost by skipping reanimated commits, we run an additional commit in the `ReanimatedMountHook` ## Test plan Check if the example app works fine
bartlomiejbloniarz
added a commit
that referenced
this pull request
Sep 19, 2024
## Summary When working on #6330 I missed the fact that the `pleaseSkipReanimatedCommit` mechanism is not only a performance optimization (preventing us from applying some changes twice a frame). It's main role is to actually prevent reanimated from starving RN out. It could happen that RN is trying to apply some heavy commit while an animation is running. If during that commit reanimated applies its own changes, then the RN commit will fail and another attempt will be made. Since reanimated runs a commit in every frame, then the RN commit might continue failing until the animation is over, which is very very very bad. In this PR I changed the "skip" name to "pause" since it better describes the intent behind it. I also reverted the change where the skip flag was reset in `performOperations`. This was done to ensure that if some changes are applied through `updateProps` after the commit hook has already gathered all updates, but before reanimated commits have been unpaused, then these changes are not lost. Unfortunately this would lead to reanimated blocking heavy RN commits from being applied. Now the skip flag is reset in the `ReanimatedMountHook` (as it was before #6330), and if there are any changes that would be lost by skipping reanimated commits, we run an additional commit in the `ReanimatedMountHook` ## Test plan Check if the example app works fine
This was referenced Sep 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR changes the way
performOperationsandReanimatedCommitHookare synchronized. The current implementations faces 2 problems:Updates that come through
performOperationsafterpleaseSkipReanimatedCommitis called in the commit hook will be deferred until the next commit. Usually it's fine since the next commit will be triggered by the next animation frame. But if there was a singular update scheduled through Reanimated, we might not see the change for a long time. This issue is more thoroughly explained in this issue.In
ReanimatedCommitMarkerthere is an assumption that there can be at most one commit happening on a given thread (i.e. there can't be nested commits). This isn't true, since there can be an event listener that commits some changes in a reaction to a native mount/unmount (which is a part of the commit function). This exact scenario was observed in the New Expensify App withreact-native-keyboard-controller. At first I thought this is a mistake, but this PR in RN seems to allow for scenarios like that.Applied fixes:
ad 1. Now instead of resetting the skip flag in reanimated after a transaction is mounted (via MountHook), we reset the flag whenever a non-empty batch is read in
performOperations. This ensures that we don't make an unnecessary commit, but never skip any updates.ad 2. Now we mark reanimated commits through
ReanimatedCommitShadowNode. This is a class derived from ShadowNode that allows us to modify the root nodes traits_ (and add our custom trait). This ensures that even if the root node gets cloned it will retain the information. We couldn't derive fromRootShadowNodesince it is a final class.closes #6245
Test plan