From b54257c628b1a174a2c41960e7fc4d2d719ad731 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Thu, 19 Mar 2020 22:55:46 -0700 Subject: [PATCH] Introduce early dispatch of ViewCommands in FabricUIManager Summary: Earlier this week I introduced a change in the old, non-Fabric renderer (D20378633 D20427803) that (gated behind a feature-flag) executes ViewCommands before all other types of commands, as a perf optimization and (I think) a potential fix for a category of race conditions. I've added more details in comments here. The Fabric renderer uses the same feature-flag that I introduced for the non-Fabric renderer. Changelog: [Internal] Fabric Reviewed By: mdvacca Differential Revision: D20449186 fbshipit-source-id: bb3649f565f32c417a6247369902333989a043aa --- .../react/fabric/FabricJSIModuleProvider.java | 4 +- .../react/fabric/FabricUIManager.java | 169 ++++++++++++++---- .../fabric/mounting/MountingManager.java | 29 ++- .../mountitems/DispatchCommandMountItem.java | 36 ++-- .../DispatchIntCommandMountItem.java | 37 ++++ .../DispatchStringCommandMountItem.java | 2 +- 6 files changed, 204 insertions(+), 73 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricJSIModuleProvider.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricJSIModuleProvider.java index 6dcaaf4de60..4d55cf236d6 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricJSIModuleProvider.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricJSIModuleProvider.java @@ -20,7 +20,7 @@ import com.facebook.react.fabric.mounting.LayoutMetricsConversions; import com.facebook.react.fabric.mounting.MountingManager; import com.facebook.react.fabric.mounting.mountitems.BatchMountItem; import com.facebook.react.fabric.mounting.mountitems.DeleteMountItem; -import com.facebook.react.fabric.mounting.mountitems.DispatchCommandMountItem; +import com.facebook.react.fabric.mounting.mountitems.DispatchIntCommandMountItem; import com.facebook.react.fabric.mounting.mountitems.DispatchStringCommandMountItem; import com.facebook.react.fabric.mounting.mountitems.InsertMountItem; import com.facebook.react.fabric.mounting.mountitems.MountItem; @@ -108,7 +108,7 @@ public class FabricJSIModuleProvider implements JSIModuleProvider { GuardedFrameCallback.class.getClass(); BatchMountItem.class.getClass(); DeleteMountItem.class.getClass(); - DispatchCommandMountItem.class.getClass(); + DispatchIntCommandMountItem.class.getClass(); DispatchStringCommandMountItem.class.getClass(); InsertMountItem.class.getClass(); MountItem.class.getClass(); 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 a6defbdfd63..eea6ba5abdd 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -39,6 +39,7 @@ import com.facebook.react.bridge.ReactNoCrashSoftException; import com.facebook.react.bridge.ReactSoftException; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; +import com.facebook.react.bridge.RetryableMountingLayerException; import com.facebook.react.bridge.UIManager; import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.bridge.WritableMap; @@ -51,6 +52,7 @@ import com.facebook.react.fabric.mounting.mountitems.BatchMountItem; import com.facebook.react.fabric.mounting.mountitems.CreateMountItem; import com.facebook.react.fabric.mounting.mountitems.DeleteMountItem; import com.facebook.react.fabric.mounting.mountitems.DispatchCommandMountItem; +import com.facebook.react.fabric.mounting.mountitems.DispatchIntCommandMountItem; import com.facebook.react.fabric.mounting.mountitems.DispatchStringCommandMountItem; import com.facebook.react.fabric.mounting.mountitems.InsertMountItem; import com.facebook.react.fabric.mounting.mountitems.MountItem; @@ -109,12 +111,17 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { new ConcurrentHashMap<>(); @NonNull private final EventBeatManager mEventBeatManager; + @NonNull private final Object mViewCommandMountItemsLock = new Object(); @NonNull private final Object mMountItemsLock = new Object(); @NonNull private final Object mPreMountItemsLock = new Object(); private boolean mInDispatch = false; private int mReDispatchCounter = 0; + @GuardedBy("mViewCommandMountItemsLock") + @NonNull + private List mViewCommandMountItems = new ArrayList<>(); + @GuardedBy("mMountItemsLock") @NonNull private List mMountItems = new ArrayList<>(); @@ -628,6 +635,47 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { mReDispatchCounter = 0; } + @UiThread + @ThreadConfined(UI) + private List getAndResetViewCommandMountItems() { + if (!ReactFeatureFlags.allowEarlyViewCommandExecution) { + return null; + } + + synchronized (mViewCommandMountItemsLock) { + List result = mViewCommandMountItems; + if (result.isEmpty()) { + return null; + } + mViewCommandMountItems = new ArrayList<>(); + return result; + } + } + + @UiThread + @ThreadConfined(UI) + private List getAndResetMountItems() { + synchronized (mMountItemsLock) { + List result = mMountItems; + if (result.isEmpty()) { + return null; + } + mMountItems = new ArrayList<>(); + return result; + } + } + + private ArrayDeque getAndResetPreMountItems() { + synchronized (mPreMountItemsLock) { + ArrayDeque result = mPreMountItems; + if (result.isEmpty()) { + return null; + } + mPreMountItems = new ArrayDeque<>(PRE_MOUNT_ITEMS_INITIAL_SIZE_ARRAY); + return result; + } + } + @UiThread @ThreadConfined(UI) /** Nothing should call this directly except for `tryDispatchMountItems`. */ @@ -638,23 +686,69 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { mRunStartTime = SystemClock.uptimeMillis(); - List mountItemsToDispatch; - synchronized (mMountItemsLock) { - if (mMountItems.isEmpty()) { - return false; + List viewCommandMountItemsToDispatch = + getAndResetViewCommandMountItems(); + List mountItemsToDispatch = getAndResetMountItems(); + + if (mountItemsToDispatch == null && viewCommandMountItemsToDispatch == null) { + return false; + } + + // As an optimization, execute all ViewCommands first + // This should be: + // 1) Performant: ViewCommands are often a replacement for SetNativeProps, which we've always + // wanted to be as "synchronous" as possible. + // 2) Safer: ViewCommands are inherently disconnected from the tree commit/diff/mount process. + // JS imperatively queues these commands. + // If JS has queued a command, it's reasonable to assume that the more time passes, the more + // likely it is that the view disappears. + // Thus, by executing ViewCommands early, we should actually avoid a category of + // errors/glitches. + if (viewCommandMountItemsToDispatch != null) { + Systrace.beginSection( + Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, + "FabricUIManager::mountViews viewCommandMountItems to execute: " + + viewCommandMountItemsToDispatch.size()); + for (DispatchCommandMountItem command : viewCommandMountItemsToDispatch) { + if (ENABLE_FABRIC_LOGS) { + FLog.d(TAG, "dispatchMountItems: Executing viewCommandMountItem: " + command.toString()); + } + try { + command.execute(mMountingManager); + } catch (RetryableMountingLayerException e) { + // If the exception is marked as Retryable, we retry the viewcommand exactly once, after + // the current batch of mount items has finished executing. + if (command.getRetries() == 0) { + command.incrementRetries(); + dispatchCommandMountItem(command); + } else { + // It's very common for commands to be executed on views that no longer exist - for + // example, a blur event on TextInput being fired because of a navigation event away + // from the current screen. If the exception is marked as Retryable, we log a soft + // exception but never crash in debug. + // It's not clear that logging this is even useful, because these events are very + // common, mundane, and there's not much we can do about them currently. + ReactSoftException.logSoftException( + TAG, + new ReactNoCrashSoftException( + "Caught exception executing ViewCommand: " + command.toString(), e)); + } + } catch (Throwable e) { + // Non-Retryable exceptions are logged as soft exceptions in prod, but crash in Debug. + ReactSoftException.logSoftException( + TAG, + new RuntimeException( + "Caught exception executing ViewCommand: " + command.toString(), e)); + } } - mountItemsToDispatch = mMountItems; - mMountItems = new ArrayList<>(); + + Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE); } // If there are MountItems to dispatch, we make sure all the "pre mount items" are executed - ArrayDeque mPreMountItemsToDispatch = null; - synchronized (mPreMountItemsLock) { - if (!mPreMountItems.isEmpty()) { - mPreMountItemsToDispatch = mPreMountItems; - mPreMountItems = new ArrayDeque<>(PRE_MOUNT_ITEMS_INITIAL_SIZE_ARRAY); - } - } + // first + ArrayDeque mPreMountItemsToDispatch = getAndResetPreMountItems(); + if (mPreMountItemsToDispatch != null) { Systrace.beginSection( Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, @@ -668,23 +762,26 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE); } - Systrace.beginSection( - Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, - "FabricUIManager::mountViews mountItems to execute: " + mountItemsToDispatch.size()); + if (mountItemsToDispatch != null) { + Systrace.beginSection( + Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, + "FabricUIManager::mountViews mountItems to execute: " + mountItemsToDispatch.size()); - long batchedExecutionStartTime = SystemClock.uptimeMillis(); - for (MountItem mountItem : mountItemsToDispatch) { - if (ENABLE_FABRIC_LOGS) { - // If a MountItem description is split across multiple lines, it's because it's a compound - // MountItem. Log each line separately. - String[] mountItemLines = mountItem.toString().split("\n"); - for (String m : mountItemLines) { - FLog.d(TAG, "dispatchMountItems: Executing mountItem: " + m); + long batchedExecutionStartTime = SystemClock.uptimeMillis(); + + for (MountItem mountItem : mountItemsToDispatch) { + if (ENABLE_FABRIC_LOGS) { + // If a MountItem description is split across multiple lines, it's because it's a compound + // MountItem. Log each line separately. + String[] mountItemLines = mountItem.toString().split("\n"); + for (String m : mountItemLines) { + FLog.d(TAG, "dispatchMountItems: Executing mountItem: " + m); + } } + mountItem.execute(mMountingManager); } - mountItem.execute(mMountingManager); + mBatchedExecutionTime += SystemClock.uptimeMillis() - batchedExecutionStartTime; } - mBatchedExecutionTime += SystemClock.uptimeMillis() - batchedExecutionStartTime; Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE); return true; @@ -787,9 +884,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @ThreadConfined(ANY) public void dispatchCommand( final int reactTag, final int commandId, @Nullable final ReadableArray commandArgs) { - synchronized (mMountItemsLock) { - mMountItems.add(new DispatchCommandMountItem(reactTag, commandId, commandArgs)); - } + dispatchCommandMountItem(new DispatchIntCommandMountItem(reactTag, commandId, commandArgs)); } @Override @@ -797,8 +892,20 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @ThreadConfined(ANY) public void dispatchCommand( final int reactTag, final String commandId, @Nullable final ReadableArray commandArgs) { - synchronized (mMountItemsLock) { - mMountItems.add(new DispatchStringCommandMountItem(reactTag, commandId, commandArgs)); + dispatchCommandMountItem(new DispatchStringCommandMountItem(reactTag, commandId, commandArgs)); + } + + @AnyThread + @ThreadConfined(ANY) + private void dispatchCommandMountItem(DispatchCommandMountItem command) { + if (ReactFeatureFlags.allowEarlyViewCommandExecution) { + synchronized (mViewCommandMountItemsLock) { + mViewCommandMountItems.add(command); + } + } else { + synchronized (mMountItemsLock) { + mMountItems.add(command); + } } } 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 84d0dfe6af0..79966769fd7 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 @@ -21,11 +21,11 @@ import androidx.annotation.UiThread; import com.facebook.common.logging.FLog; import com.facebook.infer.annotation.Assertions; import com.facebook.infer.annotation.ThreadConfined; -import com.facebook.react.bridge.ReactNoCrashSoftException; import com.facebook.react.bridge.ReactSoftException; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; import com.facebook.react.bridge.ReadableNativeMap; +import com.facebook.react.bridge.RetryableMountingLayerException; import com.facebook.react.bridge.SoftAssertions; import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.fabric.FabricUIManager; @@ -155,21 +155,18 @@ public class MountingManager { // view hierarchy. For example, TextInput may send a "blur" command in response to the view // disappearing. Throw `ReactNoCrashSoftException` so they're logged but don't crash in dev // for now. - // TODO T58653970: Crash in debug again and fix all the places that cause this to crash. if (viewState == null) { - ReactSoftException.logSoftException( - MountingManager.TAG, - new ReactNoCrashSoftException( - "Unable to find viewState for tag: " + reactTag + " for commandId: " + commandId)); - return; + throw new RetryableMountingLayerException( + "Unable to find viewState for tag: " + reactTag + " for commandId: " + commandId); } if (viewState.mViewManager == null) { - throw new IllegalStateException("Unable to find viewManager for tag " + reactTag); + throw new RetryableMountingLayerException("Unable to find viewManager for tag " + reactTag); } if (viewState.mView == null) { - throw new IllegalStateException("Unable to find viewState view for tag " + reactTag); + throw new RetryableMountingLayerException( + "Unable to find viewState view for tag " + reactTag); } viewState.mViewManager.receiveCommand(viewState.mView, commandId, commandArgs); @@ -183,21 +180,19 @@ public class MountingManager { // view hierarchy. For example, TextInput may send a "blur" command in response to the view // disappearing. Throw `ReactNoCrashSoftException` so they're logged but don't crash in dev // for now. - // TODO T58653970: Crash in debug again and fix all the places that cause this to crash. if (viewState == null) { - ReactSoftException.logSoftException( - MountingManager.TAG, - new ReactNoCrashSoftException( - "Unable to find viewState for tag: " + reactTag + " for commandId: " + commandId)); - return; + throw new RetryableMountingLayerException( + "Unable to find viewState for tag: " + reactTag + " for commandId: " + commandId); } if (viewState.mViewManager == null) { - throw new IllegalStateException("Unable to find viewState manager for tag " + reactTag); + throw new RetryableMountingLayerException( + "Unable to find viewState manager for tag " + reactTag); } if (viewState.mView == null) { - throw new IllegalStateException("Unable to find viewState view for tag " + reactTag); + throw new RetryableMountingLayerException( + "Unable to find viewState view for tag " + reactTag); } viewState.mViewManager.receiveCommand(viewState.mView, commandId, commandArgs); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchCommandMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchCommandMountItem.java index 89fc96ff1d3..29f3063cd46 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchCommandMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchCommandMountItem.java @@ -7,31 +7,23 @@ package com.facebook.react.fabric.mounting.mountitems; -import androidx.annotation.NonNull; -import androidx.annotation.Nullable; -import com.facebook.react.bridge.ReadableArray; -import com.facebook.react.fabric.mounting.MountingManager; +import androidx.annotation.UiThread; -public class DispatchCommandMountItem implements MountItem { +/** + * This is a common interface for View Command operations. Once we delete the deprecated {@link + * DispatchIntCommandMountItem}, we can delete this interface too. It provides a set of common + * operations to simplify generic operations on all types of ViewCommands. + */ +public abstract class DispatchCommandMountItem implements MountItem { + private int mNumRetries = 0; - private final int mReactTag; - private final int mCommandId; - private final @Nullable ReadableArray mCommandArgs; - - public DispatchCommandMountItem( - int reactTag, int commandId, @Nullable ReadableArray commandArgs) { - mReactTag = reactTag; - mCommandId = commandId; - mCommandArgs = commandArgs; + @UiThread + public void incrementRetries() { + mNumRetries++; } - @Override - public void execute(@NonNull MountingManager mountingManager) { - mountingManager.receiveCommand(mReactTag, mCommandId, mCommandArgs); - } - - @Override - public String toString() { - return "DispatchCommandMountItem [" + mReactTag + "] " + mCommandId; + @UiThread + public int getRetries() { + return mNumRetries; } } 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 new file mode 100644 index 00000000000..9d525755bfa --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java @@ -0,0 +1,37 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.fabric.mounting.mountitems; + +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; +import com.facebook.react.bridge.ReadableArray; +import com.facebook.react.fabric.mounting.MountingManager; + +public class DispatchIntCommandMountItem extends DispatchCommandMountItem { + + private final int mReactTag; + private final int mCommandId; + private final @Nullable ReadableArray mCommandArgs; + + public DispatchIntCommandMountItem( + int reactTag, int commandId, @Nullable ReadableArray commandArgs) { + mReactTag = reactTag; + mCommandId = commandId; + mCommandArgs = commandArgs; + } + + @Override + public void execute(@NonNull MountingManager mountingManager) { + mountingManager.receiveCommand(mReactTag, mCommandId, mCommandArgs); + } + + @Override + public String toString() { + return "DispatchIntCommandMountItem [" + mReactTag + "] " + mCommandId; + } +} 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 074466dd212..0b04e25f161 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 @@ -12,7 +12,7 @@ import androidx.annotation.Nullable; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.fabric.mounting.MountingManager; -public class DispatchStringCommandMountItem implements MountItem { +public class DispatchStringCommandMountItem extends DispatchCommandMountItem { private final int mReactTag; @NonNull private final String mCommandId;