From 7c5d9ccb46e14940de6c15766b186244ce712dc6 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Mon, 16 May 2022 14:06:58 -0700 Subject: [PATCH] Fix NativeAnimatedNodesManager registering event listener multiple times Summary: Noticed that (LockFree)EventDispatcherImpl had NativeAnimatedNodesManager as a listener 100+ times, as listener registration can happen multiple times from Animated node creation. Since listener management on event dispatcher is thread-safe, we can avoid the thread hop for this. Changelog: [Internal] Reviewed By: genkikondo Differential Revision: D36316102 fbshipit-source-id: f2f417b69885def87f88460d8b1e0b35b66726cb --- .../react/animated/NativeAnimatedModule.java | 7 ++-- .../animated/NativeAnimatedNodesManager.java | 38 +++++++------------ 2 files changed, 17 insertions(+), 28 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 a951694427e..d84f69d8b10 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java @@ -397,8 +397,8 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec /** * Given a viewTag, detect if we're running in Fabric or non-Fabric and attach an event listener - * to the correct UIManager, if necessary. This is expected to only be called from the JS thread, - * and not concurrently. + * to the correct UIManager, if necessary. This is expected to only be called from the native + * module thread, and not concurrently. * * @param viewTag */ @@ -421,8 +421,7 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec } // Subscribe to UIManager (Fabric or non-Fabric) lifecycle events if we haven't yet - if ((mInitializedForFabric && mUIManagerType == UIManagerType.FABRIC) - || (mInitializedForNonFabric && mUIManagerType == UIManagerType.DEFAULT)) { + if (mUIManagerType == UIManagerType.FABRIC ? mInitializedForFabric : mInitializedForNonFabric) { return; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java index 28d611f0b10..c4f2f8cdc20 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java @@ -76,36 +76,27 @@ import java.util.Queue; /** * Initialize event listeners for Fabric UIManager or non-Fabric UIManager, exactly once. Once - * Fabric is the only UIManager, this logic can be simplified. This is only called on the JS - * thread. + * Fabric is the only UIManager, this logic can be simplified. This is expected to only be called + * from the native module thread. * * @param uiManagerType */ - @UiThread public void initializeEventListenerForUIManagerType(@UIManagerType final int uiManagerType) { - if ((uiManagerType == UIManagerType.FABRIC && mEventListenerInitializedForFabric) - || (uiManagerType == UIManagerType.DEFAULT && mEventListenerInitializedForNonFabric)) { + if (uiManagerType == UIManagerType.FABRIC + ? mEventListenerInitializedForFabric + : mEventListenerInitializedForNonFabric) { return; } - final NativeAnimatedNodesManager self = this; - mReactApplicationContext.runOnUiQueueThread( - new Runnable() { - @Override - public void run() { - UIManager uiManager = - UIManagerHelper.getUIManager(mReactApplicationContext, uiManagerType); - if (uiManager != null) { - uiManager.getEventDispatcher().addListener(self); - - if (uiManagerType == UIManagerType.FABRIC) { - mEventListenerInitializedForFabric = true; - } else { - mEventListenerInitializedForNonFabric = true; - } - } - } - }); + UIManager uiManager = UIManagerHelper.getUIManager(mReactApplicationContext, uiManagerType); + if (uiManager != null) { + uiManager.getEventDispatcher().addListener(this); + if (uiManagerType == UIManagerType.FABRIC) { + mEventListenerInitializedForFabric = true; + } else { + mEventListenerInitializedForNonFabric = true; + } + } } /*package*/ @Nullable @@ -524,7 +515,6 @@ import java.util.Queue; } } - @UiThread @Override public void onEventDispatch(final Event event) { // Events can be dispatched from any thread so we have to make sure handleEvent is run from the