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 51fa1b80c1e..42a5dfa7a86 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -95,7 +95,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { // The IS_DEVELOPMENT_ENVIRONMENT variable is used to log extra data when running fabric in a // development environment. DO NOT ENABLE THIS ON PRODUCTION OR YOU WILL BE FIRED! - public static final boolean IS_DEVELOPMENT_ENVIRONMENT = false; + public static final boolean IS_DEVELOPMENT_ENVIRONMENT = false && ReactBuildConfig.DEBUG; public static final boolean ENABLE_FABRIC_LOGS = ReactFeatureFlags.enableFabricLogs || PrinterHolder.getPrinter() @@ -483,6 +483,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { "Caught exception in synchronouslyUpdateViewOnUIThread", ex)); } } + + @Override + public int getSurfaceId() { + return View.NO_ID; + } }; // If the reactTag exists, we assume that it might at the end of the next @@ -905,13 +910,13 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @Override public void receiveEvent(int reactTag, String eventName, @Nullable WritableMap params) { - receiveEvent(-1, reactTag, eventName, params); + receiveEvent(View.NO_ID, reactTag, eventName, params); } @Override public void receiveEvent( int surfaceId, int reactTag, String eventName, @Nullable WritableMap params) { - if (ReactBuildConfig.DEBUG && surfaceId == -1) { + if (ReactBuildConfig.DEBUG && surfaceId == View.NO_ID) { FLog.d(TAG, "Emitted event without surfaceId: [%d] %s", reactTag, eventName); } @@ -1004,7 +1009,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { public void sendAccessibilityEvent(int reactTag, int eventType) { // Can be called from native, not just JS - we need to migrate the native callsites // before removing this entirely. - addMountItem(new SendAccessibilityEvent(-1, reactTag, eventType)); + addMountItem(new SendAccessibilityEvent(View.NO_ID, reactTag, eventType)); } @DoNotStrip @@ -1045,6 +1050,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { mountingManager.setJSResponder( surfaceId, reactTag, initialReactTag, blockNativeResponder); } + + @Override + public int getSurfaceId() { + return surfaceId; + } }); } @@ -1060,6 +1070,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { public void execute(MountingManager mountingManager) { mountingManager.clearJSResponder(); } + + @Override + public int getSurfaceId() { + return View.NO_ID; + } }); } @@ -1108,29 +1123,25 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { return performanceCounters; } - /** - * Abstraction between concurrent and non-concurrent MountItem list. - * - * @param mountItem - */ private void addMountItem(MountItem mountItem) { mMountItemsConcurrent.add(mountItem); } - /** - * Abstraction between concurrent and non-concurrent PreAllocateViewMountItem list. - * - * @param mountItem - */ private void addPreAllocateMountItem(PreAllocateViewMountItem mountItem) { - mPreMountItemsConcurrent.add(mountItem); + // We do this check only for PreAllocateViewMountItem - and not DispatchMountItem or regular + // MountItem - because PreAllocateViewMountItem is not batched, and is relatively more expensive + // both to queue, to drain, and to execute. + if (!mMountingManager.surfaceIsStopped(mountItem.getSurfaceId())) { + mPreMountItemsConcurrent.add(mountItem); + } else if (IS_DEVELOPMENT_ENVIRONMENT) { + FLog.e( + TAG, + "Not queueing PreAllocateMountItem: surfaceId stopped: [%d] - %s", + mountItem.getSurfaceId(), + mountItem.toString()); + } } - /** - * Abstraction between concurrent and non-concurrent DispatchCommandMountItem list. - * - * @param mountItem - */ private void addViewCommandMountItem(DispatchCommandMountItem mountItem) { mViewCommandMountItemsConcurrent.add(mountItem); } @@ -1167,13 +1178,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { try { dispatchPreMountItems(frameTimeNanos); - boolean dispatchedMountItems = tryDispatchMountItems(); - - // Only if we did no work (besides preallocation) and have time left, evict stale - // SurfaceMountingManagers - if (!dispatchedMountItems && !haveExceededNonBatchedFrameTime(frameTimeNanos)) { - mMountingManager.evictStaleSurfaces(); - } + tryDispatchMountItems(); } catch (Exception ex) { FLog.e(TAG, "Exception thrown when executing UIFrameGuarded", ex); stop(); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java index c31cf4db49a..bf3ddbeaaeb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java @@ -33,6 +33,7 @@ import com.facebook.react.uimanager.ViewManagerRegistry; import com.facebook.yoga.YogaMeasureMode; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; /** * Class responsible for actually dispatching view updates enqueued via {@link @@ -40,12 +41,13 @@ import java.util.concurrent.ConcurrentHashMap; */ public class MountingManager { public static final String TAG = MountingManager.class.getSimpleName(); + private static final int MAX_STOPPED_SURFACE_IDS_LENGTH = 15; @NonNull private final ConcurrentHashMap mSurfaceIdToManager = new ConcurrentHashMap<>(); // any thread - private volatile int mNumStaleSurfaces = 0; + private final CopyOnWriteArrayList mStoppedSurfaceIds = new CopyOnWriteArrayList<>(); @Nullable private SurfaceMountingManager mMostRecentSurfaceMountingManager; @@ -53,52 +55,13 @@ public class MountingManager { @NonNull private final ViewManagerRegistry mViewManagerRegistry; @NonNull private final RootViewManager mRootViewManager = new RootViewManager(); + private volatile int mStoppedSurfaceCacheLastId = View.NO_ID; + private volatile boolean mStoppedSurfaceCacheLastResult = false; + public MountingManager(@NonNull ViewManagerRegistry viewManagerRegistry) { mViewManagerRegistry = viewManagerRegistry; } - /** - * Evict stale SurfaceManagers. - * - *

The reasoning here is that we want SurfaceManagers to stay around for a little while after - * the Surface is stopped, to gracefully handle race conditions with (1) native libraries like - * NativeAnimatedModule, (2) events emitted to nodes on the surface, (3) queued imperative calls - * like dispatchCommand or sendAccessibilityEvent. - * - *

Without keeping the SurfaceManager around, those race conditions would result in us not - * being able to resolve a tag at all, meaning some operation is happening with a totally invalid, - * unknown tag. However, we want to fail gracefully since it's common for operations to be queued - * up and races to happen with StopSurface. This way, we can distinguish between those race - * conditions and other totally invalid operations on non-existing nodes. - */ - @UiThread - public void evictStaleSurfaces() { - UiThreadUtil.assertOnUiThread(); - - if (mNumStaleSurfaces == 0) { - return; - } - - mNumStaleSurfaces = 0; - - for (Map.Entry entry : mSurfaceIdToManager.entrySet()) { - SurfaceMountingManager surfaceMountingManager = entry.getValue(); - int surfacedId = entry.getKey(); - if (surfaceMountingManager.isStopped()) { - if (surfaceMountingManager.shouldKeepAliveStoppedSurface()) { - mNumStaleSurfaces++; - } else { - FLog.e(TAG, "Evicting stale SurfaceMountingManager: [%d]", surfacedId); - mSurfaceIdToManager.remove(surfacedId); - - if (surfaceMountingManager == mMostRecentSurfaceMountingManager) { - mMostRecentSurfaceMountingManager = null; - } - } - } - } - } - /** * This mutates the rootView, which is an Android View, so this should only be called on the UI * thread. @@ -136,10 +99,20 @@ public class MountingManager { @AnyThread public void stopSurface(final int surfaceId) { + mStoppedSurfaceCacheLastId = View.NO_ID; + SurfaceMountingManager surfaceMountingManager = mSurfaceIdToManager.get(surfaceId); if (surfaceMountingManager != null) { + // Maximum number of stopped surfaces to keep track of + while (mStoppedSurfaceIds.size() >= MAX_STOPPED_SURFACE_IDS_LENGTH) { + Integer staleStoppedId = mStoppedSurfaceIds.get(0); + mSurfaceIdToManager.remove(staleStoppedId.intValue()); + mStoppedSurfaceIds.remove(staleStoppedId); + FLog.d(TAG, "Removing stale SurfaceMountingManager: [%d]", staleStoppedId.intValue()); + } + mStoppedSurfaceIds.add(surfaceId); + surfaceMountingManager.stopSurface(); - mNumStaleSurfaces++; if (surfaceMountingManager == mMostRecentSurfaceMountingManager) { mMostRecentSurfaceMountingManager = null; @@ -150,10 +123,6 @@ public class MountingManager { new IllegalViewOperationException( "Cannot call StopSurface on non-existent surface: [" + surfaceId + "]")); } - - // We do not evict surfaces right away; the SurfaceMountingManager will stay in memory for a bit - // longer. See SurfaceMountingManager.stopSurface and - // evictStaleSurfaces for more details. } @Nullable @@ -176,6 +145,33 @@ public class MountingManager { return surfaceMountingManager; } + public boolean surfaceIsStopped(int surfaceId) { + if (surfaceId == View.NO_ID) { + return false; + } + if (surfaceId == mStoppedSurfaceCacheLastId) { + return mStoppedSurfaceCacheLastResult; + } + + boolean res = surfaceIsStoppedImpl(surfaceId); + mStoppedSurfaceCacheLastResult = res; + mStoppedSurfaceCacheLastId = surfaceId; + return res; + } + + private boolean surfaceIsStoppedImpl(int surfaceId) { + if (mStoppedSurfaceIds.contains(surfaceId)) { + return true; + } + + SurfaceMountingManager surfaceMountingManager = mSurfaceIdToManager.get(surfaceId); + if (surfaceMountingManager != null && surfaceMountingManager.isStopped()) { + return true; + } + + return false; + } + /** * Get SurfaceMountingManager associated with a ReactTag. Unfortunately, this requires lookups * over N maps, where N is the number of active or recently-stopped Surfaces. Each lookup will diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java index 3cbc20118e0..5c3686e7787 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java @@ -45,7 +45,6 @@ public class SurfaceMountingManager { public static final String TAG = SurfaceMountingManager.class.getSimpleName(); private static final boolean SHOW_CHANGED_VIEW_HIERARCHIES = ReactBuildConfig.DEBUG && false; - private static final long KEEPALIVE_MILLISECONDS = 1000; private volatile boolean mIsStopped = false; @@ -84,11 +83,6 @@ public class SurfaceMountingManager { return mIsStopped; } - public boolean shouldKeepAliveStoppedSurface() { - assert mIsStopped; - return (System.currentTimeMillis() - mLastSuccessfulQueryTime) < KEEPALIVE_MILLISECONDS; - } - public ThemedReactContext getContext() { return mThemedReactContext; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java index 176abe1f270..6f23f9b8779 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java @@ -27,6 +27,11 @@ public class DispatchIntCommandMountItem extends DispatchCommandMountItem { mCommandArgs = commandArgs; } + @Override + public int getSurfaceId() { + return mSurfaceId; + } + @Override public void execute(@NonNull MountingManager mountingManager) { mountingManager.receiveCommand(mSurfaceId, mReactTag, mCommandId, mCommandArgs); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchStringCommandMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchStringCommandMountItem.java index 361f92ad619..1d76091b6ae 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchStringCommandMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchStringCommandMountItem.java @@ -27,6 +27,11 @@ public class DispatchStringCommandMountItem extends DispatchCommandMountItem { mCommandArgs = commandArgs; } + @Override + public int getSurfaceId() { + return mSurfaceId; + } + @Override public void execute(@NonNull MountingManager mountingManager) { mountingManager.receiveCommand(mSurfaceId, mReactTag, mCommandId, mCommandArgs); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java index 02106de603d..311a08143ea 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java @@ -182,7 +182,8 @@ public class IntBufferBatchMountItem implements MountItem { endMarkers(); } - public int getRootTag() { + @Override + public int getSurfaceId() { return mSurfaceId; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/MountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/MountItem.java index dc053625c2a..5ec3aeea571 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/MountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/MountItem.java @@ -7,6 +7,7 @@ package com.facebook.react.fabric.mounting.mountitems; +import androidx.annotation.AnyThread; import androidx.annotation.NonNull; import androidx.annotation.UiThread; import com.facebook.react.fabric.mounting.MountingManager; @@ -16,4 +17,7 @@ public interface MountItem { /** Execute this {@link MountItem} into the operation queue received by parameter. */ @UiThread void execute(@NonNull MountingManager mountingManager); + + @AnyThread + int getSurfaceId(); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java index a39d5ff31c0..7764bd58a3c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java @@ -43,7 +43,8 @@ public class PreAllocateViewMountItem implements MountItem { mIsLayoutable = isLayoutable; } - public int getRootTag() { + @Override + public int getSurfaceId() { return mSurfaceId; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEvent.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEvent.java index 9506857f195..a52ed29042d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEvent.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEvent.java @@ -42,6 +42,11 @@ public class SendAccessibilityEvent implements MountItem { } } + @Override + public int getSurfaceId() { + return mSurfaceId; + } + @Override public String toString() { return "SendAccessibilityEvent [" + mReactTag + "] " + mEventType;