Fix native animated event lag on Android #11994

janicduplessis · 2017-01-19T23:13:00Z

Native animated events sometimes end up lagging a frame behind on android because we perform the update in the normal animation loop instead of doing it immediately when we receive the event. We had the same issue on iOS and was fixed in a similar way.

Moved some code around to have a method that updates a list of node that we can use to update the node in the animated event handler and also use it in the animation update loop.

Test plan
Tested that it did fix sticky headers lagging a frame behind during momentum scrolling in my PR #11315 and also tested the native animations examples still work properly.

janicduplessis · 2017-01-20T05:49:51Z

cc @kmagiera @ide @ryangomba

janicduplessis · 2017-01-20T05:51:19Z

The only downside of separating the update loop into an extra method is that it does add an extra fixed size array allocation but does simplify the code a bit.

facebook-github-bot · 2017-01-31T18:28:53Z

@sahrens has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

sahrens · 2017-01-31T20:20:02Z

I patched this in along with #11315 and everything works great, but I don't have any context on the code - @kmagiera @ide @ryangomba @AaaChiuuu @achen1, can one of you guys give the go-ahead on this?

facebook-github-bot · 2017-02-01T06:48:11Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

mkonicek · 2017-02-01T11:44:43Z

Let's ship it since it works for you @sahrens. Test plan looks good.

mkonicek · 2017-02-01T11:45:10Z

@facebook-github-bot shipit

facebook-github-bot · 2017-02-01T11:46:23Z

@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-02-02T00:48:20Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

mkonicek · 2017-02-02T11:44:17Z

@facebook-github-bot shipit

facebook-github-bot · 2017-02-02T11:45:10Z

@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-02-03T00:48:22Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

facebook-github-bot · 2017-02-07T02:46:29Z

@sahrens has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

sahrens · 2017-02-07T02:47:42Z

@janicduplessis: this is having trouble importing - maybe there is a merge conflict? Can you rebase and update?

janicduplessis · 2017-02-07T03:26:56Z

@sahrens Weird, doesn't look like there was any conficts, updated to master.

sahrens · 2017-02-08T06:42:28Z

There were some internal flow errors...I think I fixed them all but this is still having trouble. I'll get it through soon, I promise!

janicduplessis · 2017-02-13T22:20:20Z

@sahrens Any updates on this?

sahrens · 2017-02-13T22:22:22Z

@astreet said he would take a look - any progress, Andy?

sahrens · 2017-02-13T22:22:59Z

There were some internal test failures :(

astreet · 2017-02-14T11:39:23Z

Hmm? I haven't seen this PR before but I can take a look now

astreet · 2017-02-14T11:43:50Z

ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java

@@ -353,9 +355,47 @@ public void onEventDispatch(Event event) {
   */
  public void runUpdates(long frameTimeNanos) {
    UiThreadUtil.assertOnUiThread();
+    boolean hasFinishedAnimations = false;
+
+    List<AnimatedNode> nodes = new ArrayList<AnimatedNode>(mUpdatedNodes.size() + mActiveAnimations.size());


Re-allocating a new list on every frame isn't great: you can just keep a static one around to re-use.

astreet · 2017-02-14T11:46:16Z

ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java

+
+    for (int i = 0; i < mActiveAnimations.size(); i++) {
+      AnimationDriver animation = mActiveAnimations.valueAt(i);
+      animation.runAnimationStep(frameTimeNanos);


I'm not familiar with this code, but this seems odd: why are we running the animation and then updating all the nodes for the frame below on line L376? I.e., shouldn't we be updating first?

This code didn't change, I just moved it in it's own method. How it works is we run the animation drivers first, which update the value nodes then update all the other nodes that are connected to these updated value nodes, which will cause props nodes to update their connected view.

astreet · 2017-02-14T11:48:57Z

ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java

@@ -334,7 +335,8 @@ public void onEventDispatch(Event event) {
      EventAnimationDriver eventDriver = mEventDrivers.get(event.getViewTag() + eventName);
      if (eventDriver != null) {
        event.dispatch(eventDriver);
-        mUpdatedNodes.put(eventDriver.mValueNode.mTag, eventDriver.mValueNode);
+
+        updateNodes(Collections.singletonList((AnimatedNode) eventDriver.mValueNode));


You have an updateNodes here when you receive an event, and then in the same frame you could runUpdates and updateNodes again: is that correct?

There are cases where this won't happen in the same frame, like during momentum scrolling, this is why we need to update it immediatly instead of relying on runUpdates which is called on each frame by the Choreographer.

These functions only update nodes that are marked as needing an update so it won't update twice in a frame.

sahrens · 2017-02-14T16:34:04Z

Thanks @astreet. This is part of the stack to get sticky list headers working on Android by changing the impl to a shared `Animated` one with `useNativeDriver`. Several folks internal and external have requested it, but those internal-only Android tests were failing so I couldn't land it. On Feb 14, 2017, at 3:51 AM, Andy Street <[email protected]<mailto:[email protected]>> wrote: @astreet requested changes on this pull request.

________________________________ In ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java<#11994 (review)>:

@@ -353,9 +355,47 @@ public void onEventDispatch(Event event) {

*/ public void runUpdates(long frameTimeNanos) { UiThreadUtil.assertOnUiThread(); + boolean hasFinishedAnimations = false; + + List<AnimatedNode> nodes = new ArrayList<AnimatedNode>(mUpdatedNodes.size() + mActiveAnimations.size()); Re-allocating a new list on every frame isn't great: you can just keep a static one around to re-use.

________________________________ In ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java<#11994 (review)>:

@@ -353,9 +355,47 @@ public void onEventDispatch(Event event) {

*/ public void runUpdates(long frameTimeNanos) { UiThreadUtil.assertOnUiThread(); + boolean hasFinishedAnimations = false; + + List<AnimatedNode> nodes = new ArrayList<AnimatedNode>(mUpdatedNodes.size() + mActiveAnimations.size()); + for (int i = 0; i < mUpdatedNodes.size(); i++) { + AnimatedNode node = mUpdatedNodes.valueAt(i); + nodes.add(node); + } + + for (int i = 0; i < mActiveAnimations.size(); i++) { + AnimationDriver animation = mActiveAnimations.valueAt(i); + animation.runAnimationStep(frameTimeNanos); I'm not familiar with this code, but this seems odd: why are we running the animation and then updating all the nodes for the frame below on line L376? I.e., shouldn't we be updating first?

________________________________ In ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java<#11994 (review)>:

@@ -334,7 +335,8 @@ public void onEventDispatch(Event event) {

EventAnimationDriver eventDriver = mEventDrivers.get(event.getViewTag() + eventName); if (eventDriver != null) { event.dispatch(eventDriver); - mUpdatedNodes.put(eventDriver.mValueNode.mTag, eventDriver.mValueNode); + + updateNodes(Collections.singletonList((AnimatedNode) eventDriver.mValueNode)); You have an updateNodes here when you receive an event, and then in the same frame you could runUpdates and updateNodes again: is that correct? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#11994 (review)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABcJxxGR2t1MWZw7KHHl7yDt-Eo6KQilks5rcZUkgaJpZM4LoueJ>.

janicduplessis · 2017-02-17T23:40:48Z

@astreet Updated to use a static list.

ide · 2017-02-17T23:45:01Z

ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java

@@ -51,6 +52,9 @@
 */
 /*package*/ class NativeAnimatedNodesManager implements EventDispatcherListener {

+  // Used to avoid allocating a new array on every frame in runUpdates.
+  private final static List<AnimatedNode> sRunUpdateNodeList = new ArrayList<>();


can you make this an instance variable unless it truly needs to be static and shared across NodesManagers?

Both would work, having it as a static ivar avoids having multiple copies in case there are multiple instances of the animated module (like in exponent). The array gets cleaned up after each invocation of runUpdates and this always runs from the same thread so it is safe.

If you feel it should be a normal ivar I don't really mind, just saves a few bytes in rare situations.

Yeah, I'd prefer an ivar in case someone somewhere decides to have multiple RN instances running at the same time :)

astreet · 2017-02-20T12:03:21Z

ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java

@@ -51,6 +52,9 @@
 */
 /*package*/ class NativeAnimatedNodesManager implements EventDispatcherListener {

+  // Used to avoid allocating a new array on every frame in runUpdates.
+  private final static List<AnimatedNode> sRunUpdateNodeList = new ArrayList<>();


Yeah, I'd prefer an ivar in case someone somewhere decides to have multiple RN instances running at the same time :)

janicduplessis · 2017-02-20T12:23:09Z

@astreet Updated to an ivar

facebook-github-bot · 2017-02-20T13:14:13Z

@astreet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Native animated events sometimes end up lagging a frame behind on android because we perform the update in the normal animation loop instead of doing it immediately when we receive the event. We had the same issue on iOS and was fixed in a similar way. Moved some code around to have a method that updates a list of node that we can use to update the node in the animated event handler and also use it in the animation update loop. **Test plan** Tested that it did fix sticky headers lagging a frame behind during momentum scrolling in my PR facebook#11315 and also tested the native animations examples still work properly. Closes facebook#11994 Reviewed By: mkonicek Differential Revision: D4488977 Pulled By: sahrens fbshipit-source-id: 831a1565bc7b8fa88cadd5a8c1be876fbdefbf66

Fix native animated event lag on Android

5861677

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Jan 19, 2017

janicduplessis mentioned this pull request Jan 19, 2017

Implement sticky headers in JS using Native Animated #11315

Closed

janicduplessis added Android labels Jan 20, 2017

facebook-github-bot added Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jan 31, 2017

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 1, 2017

janicduplessis force-pushed the native-animed-event-fix branch from 1f44559 to 5861677 Compare February 1, 2017 07:12

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 1, 2017

facebook-github-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 2, 2017

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 2, 2017

facebook-github-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 3, 2017

Merge branch 'master' into native-animed-event-fix

ecf6558

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 7, 2017

Merge branch 'master' into native-animed-event-fix

46127a6

janicduplessis removed Import Failed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 7, 2017

Merge branch 'master' into native-animed-event-fix

d742c41

astreet suggested changes Feb 14, 2017

View reviewed changes

Use static list to avoid allocations

6acd26c

ide reviewed Feb 17, 2017

View reviewed changes

astreet suggested changes Feb 20, 2017

View reviewed changes

static -> ivar

790d88d

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 20, 2017

facebook-github-bot closed this in 61d3741 Feb 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix native animated event lag on Android #11994

Fix native animated event lag on Android #11994

janicduplessis commented Jan 19, 2017

janicduplessis commented Jan 20, 2017

janicduplessis commented Jan 20, 2017

facebook-github-bot commented Jan 31, 2017

sahrens commented Jan 31, 2017

facebook-github-bot commented Feb 1, 2017

mkonicek commented Feb 1, 2017 •

edited

Loading

mkonicek commented Feb 1, 2017

facebook-github-bot commented Feb 1, 2017

facebook-github-bot commented Feb 2, 2017

mkonicek commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

facebook-github-bot commented Feb 3, 2017

facebook-github-bot commented Feb 7, 2017

sahrens commented Feb 7, 2017

janicduplessis commented Feb 7, 2017

sahrens commented Feb 8, 2017

janicduplessis commented Feb 13, 2017

sahrens commented Feb 13, 2017

sahrens commented Feb 13, 2017

astreet commented Feb 14, 2017

astreet Feb 14, 2017

janicduplessis Feb 14, 2017

astreet Feb 14, 2017

janicduplessis Feb 14, 2017

astreet Feb 14, 2017

janicduplessis Feb 14, 2017

sahrens commented Feb 14, 2017 via email

janicduplessis commented Feb 17, 2017

ide Feb 17, 2017

janicduplessis Feb 17, 2017

astreet Feb 20, 2017

astreet Feb 20, 2017

janicduplessis commented Feb 20, 2017

facebook-github-bot commented Feb 20, 2017

Fix native animated event lag on Android #11994

Fix native animated event lag on Android #11994

Conversation

janicduplessis commented Jan 19, 2017

janicduplessis commented Jan 20, 2017

janicduplessis commented Jan 20, 2017

facebook-github-bot commented Jan 31, 2017

sahrens commented Jan 31, 2017

facebook-github-bot commented Feb 1, 2017

mkonicek commented Feb 1, 2017 • edited Loading

mkonicek commented Feb 1, 2017

facebook-github-bot commented Feb 1, 2017

facebook-github-bot commented Feb 2, 2017

mkonicek commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

facebook-github-bot commented Feb 3, 2017

facebook-github-bot commented Feb 7, 2017

sahrens commented Feb 7, 2017

janicduplessis commented Feb 7, 2017

sahrens commented Feb 8, 2017

janicduplessis commented Feb 13, 2017

sahrens commented Feb 13, 2017

sahrens commented Feb 13, 2017

astreet commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahrens commented Feb 14, 2017 via email

janicduplessis commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis commented Feb 20, 2017

facebook-github-bot commented Feb 20, 2017

mkonicek commented Feb 1, 2017 •

edited

Loading