Fix issues with skipping reanimated commit #6498
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
tomekzaw
requested changes
Sep 12, 2024
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/PropsRegistry.h
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/PropsRegistry.h
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/PropsRegistry.h
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ReanimatedMountHook.cpp
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ReanimatedMountHook.h
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ReanimatedMountHook.h
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ReanimatedMountHook.h
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ReanimatedMountHook.cpp
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ReanimatedMountHook.cpp
Outdated
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/PropsRegistry.h
Outdated
Show resolved
Hide resolved
tomekzaw
approved these changes
Sep 13, 2024
piaskowyk
approved these changes
Sep 13, 2024
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/PropsRegistry.h
Show resolved
Hide resolved
packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ReanimatedMountHook.cpp
Outdated
Show resolved
Hide resolved
bartlomiejbloniarz
deleted the
@bartlomiejbloniarz/fix-skip-reanimated-commit
branch
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
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
When working on #6330 I missed the fact that the
pleaseSkipReanimatedCommitmechanism 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 throughupdatePropsafter 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 theReanimatedMountHook(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 theReanimatedMountHookTest plan
Check if the example app works fine