Skip flushing animation-frame callbacks if update was already run for a given frame #4099
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
reviewed
Feb 24, 2023
piaskowyk
approved these changes
Feb 24, 2023
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
… a given frame (software-mansion#4099) ## Summary This PR fixes a perf regression introduced in software-mansion#3722. The issue was that we'd schedule a native request animation frame callback several times in a single frame. As a result we'd run mapper updates more than once per frame which resulted in UI thread skipping frames. The bug would surface in examples where we had both timing (active) and gesture driven animations. In such scenario we'd enqueue native requestAnimationFrame callback twice – first time for the running timing animation, and the second time as a result of handling the touch event. These two callbacks would then run as long as there are update happening and would incur additional cost to all updates happening on the UI thread. Moreover, for each new gesture started afterwards we'd add a new call to requestAnimationFrame and yet another time the updates would be executed withing a single frame. After several times of launching new stream of events, we'd end up with a huge number of repeated work being done in a single frame which was causing significant frame drops. This PR addresses this problem by preventing requestAnimationFrame callbacks to be flushed more than once in a single frame. We do this by remembering if the previous flush was caused by the native rAF callback or was it forced by other activity (i.e. event handling). Then we only allow native rAF callback to perform a flush if it doesn't immediately follow a non-native flush. This way we don't perform additional work in a frame when the updates are already triggered by the event. ## Test plan Below is a link to a minimum repro of this issue. The app runs an infinite animation and renders a draggable circle. After running this app for a while and pulling the circle several times the performance starts to drop. https://gist.github.com/kmagiera/b2df85f9512951f5e6ceee7bc569f5f1
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 fixes a perf regression introduced in #3722. The issue was that we'd schedule a native request animation frame callback several times in a single frame. As a result we'd run mapper updates more than once per frame which resulted in UI thread skipping frames. The bug would surface in examples where we had both timing (active) and gesture driven animations. In such scenario we'd enqueue native requestAnimationFrame callback twice – first time for the running timing animation, and the second time as a result of handling the touch event. These two callbacks would then run as long as there are update happening and would incur additional cost to all updates happening on the UI thread. Moreover, for each new gesture started afterwards we'd add a new call to requestAnimationFrame and yet another time the updates would be executed withing a single frame. After several times of launching new stream of events, we'd end up with a huge number of repeated work being done in a single frame which was causing significant frame drops.
This PR addresses this problem by preventing requestAnimationFrame callbacks to be flushed more than once in a single frame. We do this by remembering if the previous flush was caused by the native rAF callback or was it forced by other activity (i.e. event handling). Then we only allow native rAF callback to perform a flush if it doesn't immediately follow a non-native flush. This way we don't perform additional work in a frame when the updates are already triggered by the event.
Test plan
Below is a link to a minimum repro of this issue. The app runs an infinite animation and renders a draggable circle. After running this app for a while and pulling the circle several times the performance starts to drop.
https://gist.github.com/kmagiera/b2df85f9512951f5e6ceee7bc569f5f1