diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java index e759b0269f7..b30edeeab20 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java @@ -88,6 +88,16 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec private abstract class UIThreadOperation { abstract void execute(NativeAnimatedNodesManager animatedNodesManager); + + long mFrameNumber = -1; + + public void setFrameNumber(long frameNumber) { + mFrameNumber = frameNumber; + } + + public long getFrameNumber() { + return mFrameNumber; + } } @NonNull private final GuardedFrameCallback mAnimatedFrameCallback; @@ -103,6 +113,8 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec private @Nullable NativeAnimatedNodesManager mNodesManager; + private volatile long mFrameNumber = 0; + private long mDispatchedFrameNumber = 0; private boolean mInitializedForFabric = false; private boolean mInitializedForNonFabric = false; private @UIManagerType int mUIManagerType = UIManagerType.DEFAULT; @@ -157,13 +169,25 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec } private void addOperation(UIThreadOperation operation) { + operation.setFrameNumber(mFrameNumber); mOperations.add(operation); } private void addPreOperation(UIThreadOperation operation) { + operation.setFrameNumber(mFrameNumber); mPreOperations.add(operation); } + // For FabricUIManager only + @Override + public void didScheduleMountItems(UIManager uiManager) { + if (mUIManagerType != UIManagerType.FABRIC) { + return; + } + + mFrameNumber++; + } + // For FabricUIManager only @Override @UiThread @@ -172,6 +196,22 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec return; } + // The problem we're trying to solve here: we could be in the middle of queueing + // a batch of related animation operations when Fabric flushes a batch of MountItems. + // It's visually bad if we execute half of the animation ops and then wait another frame + // (or more) to execute the rest. + // See mFrameNumber. If the dispatchedFrameNumber drifts too far - that + // is, if no MountItems are scheduled for a while, which can happen if a tree + // is committed but there are no changes - bring these counts back in sync and + // execute any queued operations. This number is arbitrary, but we want it low + // enough that the user shouldn't be able to see this delay in most cases. + mDispatchedFrameNumber++; + long currentFrameNo = mFrameNumber - 1; + if ((mDispatchedFrameNumber - mFrameNumber) > 2) { + mFrameNumber = mDispatchedFrameNumber; + currentFrameNo = mFrameNumber; + } + // This will execute all operations and preOperations queued // since the last time this was run, and will race with anything // being queued from the JS thread. That is, if the JS thread @@ -182,13 +222,35 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec // is that `scheduleMountItems` happens as close to the JS commit as // possible, whereas execution of those same items might happen sometime // later on the UI thread while the JS thread keeps plugging along. - executeAllOperations(mPreOperations); - executeAllOperations(mOperations); + executeAllOperations(mPreOperations, currentFrameNo); + executeAllOperations(mOperations, currentFrameNo); } - private void executeAllOperations(Queue operationQueue) { + private void executeAllOperations(Queue operationQueue, long maxFrameNumber) { NativeAnimatedNodesManager nodesManager = getNodesManager(); while (true) { + // There is a race condition where `peek` may return a non-null value and isEmpty() is false, + // but `poll` returns a null value - it's not clear why since we only peek and poll on the UI + // thread, but it might be something that happens during teardown or a crash. Regardless, the + // root cause is not currently known so we're extra cautious here. + // It happens equally in Fabric and non-Fabric. + UIThreadOperation peekedOperation = operationQueue.peek(); + + // This is the same as operationQueue.isEmpty() + if (peekedOperation == null) { + return; + } + // The rest of the operations are for the next frame. + if (peekedOperation.getFrameNumber() > maxFrameNumber) { + return; + } + + // Since we apparently can't guarantee that there is still an operation on the queue, + // much less the same operation, we do a poll and another null check. If this isn't + // the same operation as the peeked operation, we can't do anything about it - we still + // need to execute it, we have no mechanism to put it at the front of the queue, and it + // won't cause any errors to execute it earlier than expected (just a bit of UI jank at worst) + // so we just continue happily along. UIThreadOperation polledOperation = operationQueue.poll(); if (polledOperation == null) { return; @@ -208,11 +270,13 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec return; } + final long frameNo = mFrameNumber++; + UIBlock preOperationsUIBlock = new UIBlock() { @Override public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) { - executeAllOperations(mPreOperations); + executeAllOperations(mPreOperations, frameNo); } }; @@ -220,7 +284,7 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec new UIBlock() { @Override public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) { - executeAllOperations(mOperations); + executeAllOperations(mOperations, frameNo); } }; diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/UIManagerListener.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/UIManagerListener.java index d08ab69b62d..c190e0a8c02 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/UIManagerListener.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/UIManagerListener.java @@ -16,4 +16,6 @@ public interface UIManagerListener { void willDispatchViewUpdates(UIManager uiManager); /* Called right after view updates are dispatched for a frame. */ void didDispatchMountItems(UIManager uiManager); + /* Called right after scheduleMountItems is called in Fabric, after a new tree is committed. */ + void didScheduleMountItems(UIManager uiManager); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index df44999e0c7..31f95bf7cad 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -588,6 +588,12 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { boolean isBatchMountItem = mountItem instanceof BatchMountItem; boolean shouldSchedule = !(isBatchMountItem && ((BatchMountItem) mountItem).getSize() == 0); + // In case of sync rendering, this could be called on the UI thread. Otherwise, + // it should ~always be called on the JS thread. + for (UIManagerListener listener : mListeners) { + listener.didScheduleMountItems(this); + } + if (isBatchMountItem) { mCommitStartTime = commitStartTime; mLayoutTime = layoutEndTime - layoutStartTime;