From d7f748a944a9a9324e485ccbe214098e6c8645fc Mon Sep 17 00:00:00 2001 From: Andrei Shikov Date: Sun, 20 Feb 2022 13:23:01 -0800 Subject: [PATCH] Drain animation operation queue before execution Summary: The concurrent queue seems to leak values on some Android 12 devices when interleaving instruction polling and execution. To help with that, we can drain the queue before executing operations, reducing the window when `add` and `poll` happen concurrently. Changelog: [Internal] - Drain the Animated queue before execution Reviewed By: JoshuaGross Differential Revision: D34370056 fbshipit-source-id: 781631dbf1a10e3f1ba9564db985403bd77a2c06 --- .../react/animated/NativeAnimatedModule.java | 128 ++++++++++-------- 1 file changed, 74 insertions(+), 54 deletions(-) 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 e419c57a164..ae447ce2031 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java @@ -7,6 +7,7 @@ package com.facebook.react.animated; +import androidx.annotation.AnyThread; import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.UiThread; @@ -33,6 +34,8 @@ import com.facebook.react.uimanager.UIManagerHelper; import com.facebook.react.uimanager.UIManagerModule; import com.facebook.react.uimanager.common.UIManagerType; import com.facebook.react.uimanager.common.ViewUtil; +import java.util.ArrayList; +import java.util.List; import java.util.Queue; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicReference; @@ -102,16 +105,76 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec } } + private class ConcurrentOperationQueue { + private final Queue mQueue = new ConcurrentLinkedQueue<>(); + + @AnyThread + boolean isEmpty() { + return mQueue.isEmpty(); + } + + @AnyThread + void add(UIThreadOperation operation) { + mQueue.add(operation); + } + + @UiThread + void executeBatch(long maxBatchNumber, NativeAnimatedNodesManager nodesManager) { + List operations = new ArrayList<>(); + 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 = mQueue.peek(); + + // This is the same as operationQueue.isEmpty() + if (peekedOperation == null) { + break; + } + // The rest of the operations are for the next frame. + if (peekedOperation.getBatchNumber() > maxBatchNumber) { + break; + } + + // 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 = mQueue.poll(); + if (peekedOperation != polledOperation) { + ReactSoftExceptionLogger.logSoftException( + NAME, + new RuntimeException( + "Inconsistency detected: peeked animation operation different from polled: " + + peekedOperation + + " / " + + polledOperation)); + } + if (polledOperation == null) { + break; + } + operations.add(polledOperation); + } + + for (UIThreadOperation operation : operations) { + operation.execute(nodesManager); + } + } + } + @NonNull private final GuardedFrameCallback mAnimatedFrameCallback; private final ReactChoreographer mReactChoreographer; - @NonNull - private final ConcurrentLinkedQueue mOperations = - new ConcurrentLinkedQueue<>(); - - @NonNull - private final ConcurrentLinkedQueue mPreOperations = - new ConcurrentLinkedQueue<>(); + @NonNull private final ConcurrentOperationQueue mOperations = new ConcurrentOperationQueue(); + @NonNull private final ConcurrentOperationQueue mPreOperations = new ConcurrentOperationQueue(); private final AtomicReference mNodesManager = new AtomicReference<>(); @@ -221,51 +284,8 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec } } - executeAllOperations(mPreOperations, batchNumber); - executeAllOperations(mOperations, batchNumber); - } - - @UiThread - private void executeAllOperations(Queue operationQueue, long maxBatchNumber) { - 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.getBatchNumber() > maxBatchNumber) { - 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 (peekedOperation != polledOperation) { - ReactSoftExceptionLogger.logSoftException( - NAME, - new RuntimeException( - "Inconsistency detected: peeked animation operation different from polled: " - + peekedOperation - + " / " - + polledOperation)); - } - if (polledOperation == null) { - return; - } - polledOperation.execute(nodesManager); - } + mPreOperations.executeBatch(batchNumber, getNodesManager()); + mOperations.executeBatch(batchNumber, getNodesManager()); } // For non-FabricUIManager only @@ -285,7 +305,7 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec new UIBlock() { @Override public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) { - executeAllOperations(mPreOperations, frameNo); + mPreOperations.executeBatch(frameNo, getNodesManager()); } }; @@ -293,7 +313,7 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec new UIBlock() { @Override public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) { - executeAllOperations(mOperations, frameNo); + mOperations.executeBatch(frameNo, getNodesManager()); } };