From 26787e2260412d9d2fe831e68a8616505d3cab36 Mon Sep 17 00:00:00 2001 From: Andrei Shikov Date: Sat, 14 Nov 2020 08:56:58 -0800 Subject: [PATCH] Back out "Experiment to replace Fabric MountItem lists with concurrent queues" Summary: Changelog: [Internal] Original commit changeset: fcbdeda51f91 Reviewed By: rubennorte Differential Revision: D24973616 fbshipit-source-id: 4d21211d329c77dba50972a26b1daeccfffad912 --- .../react/config/ReactFeatureFlags.java | 3 - .../react/fabric/FabricUIManager.java | 202 ++++++------------ 2 files changed, 61 insertions(+), 144 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index d972c83330e..5a5cfeee2f7 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -88,7 +88,4 @@ public class ReactFeatureFlags { /** Potential bugfix for crashes caused by mutating the view hierarchy during onDraw. */ public static boolean enableDrawMutationFix = true; - - /** Use lock-free data structures for Fabric MountItems. */ - public static boolean enableLockFreeMountInstructions = false; } 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 05ca95c13f2..f769f708f43 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -89,12 +89,10 @@ import com.facebook.react.views.text.TextLayoutManager; import com.facebook.systrace.Systrace; import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.CopyOnWriteArrayList; @SuppressLint("MissingNativeLoadLibrary") @@ -126,35 +124,20 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { new ConcurrentHashMap<>(); @NonNull private final EventBeatManager mEventBeatManager; - - private boolean mInDispatch = false; - private int mReDispatchCounter = 0; - - @NonNull - private final CopyOnWriteArrayList mListeners = new CopyOnWriteArrayList<>(); - - // Concurrent MountItem data-structures, experimental. TODO: T79662803 - @NonNull - private final ConcurrentLinkedQueue mViewCommandMountItemsConcurrent = - new ConcurrentLinkedQueue<>(); - - @NonNull - private final ConcurrentLinkedQueue mMountItemsConcurrent = - new ConcurrentLinkedQueue<>(); - - @NonNull - private final ConcurrentLinkedQueue mPreMountItemsConcurrent = - new ConcurrentLinkedQueue<>(); - - // Non-concurrent MountItem data-structures @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<>(); + @NonNull + private final CopyOnWriteArrayList mListeners = new CopyOnWriteArrayList<>(); + @GuardedBy("mMountItemsLock") @NonNull private List mMountItems = new ArrayList<>(); @@ -356,15 +339,18 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { // possible at teardown, but this race should *never* happen at startup. @Nullable ThemedReactContext context = mReactContextForRootTag.get(rootTag); - addPreAllocateMountItem( - new PreAllocateViewMountItem( - context, - rootTag, - reactTag, - getFabricComponentName(componentName), - props, - (StateWrapper) stateWrapper, - isLayoutable)); + String component = getFabricComponentName(componentName); + synchronized (mPreMountItemsLock) { + mPreMountItems.add( + new PreAllocateViewMountItem( + context, + rootTag, + reactTag, + component, + props, + (StateWrapper) stateWrapper, + isLayoutable)); + } } @DoNotStrip @@ -630,7 +616,9 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { // If the reactTag exists, we assume that it might at the end of the next // batch of MountItems. Otherwise, we try to execute immediately. if (!mMountingManager.getViewExists(reactTag)) { - addMountItem(synchronousMountItem); + synchronized (mMountItemsLock) { + mMountItems.add(synchronousMountItem); + } return; } @@ -704,7 +692,9 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { } if (shouldSchedule && mountItem != null) { - addMountItem(mountItem); + synchronized (mMountItemsLock) { + mMountItems.add(mountItem); + } if (UiThreadUtil.isOnUiThread()) { // We only read these flags on the UI thread. tryDispatchMountItems(); @@ -787,28 +777,9 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { mLastExecutedMountItemSurfaceId = -1; } - @Nullable - private List drainConcurrentItemQueue(ConcurrentLinkedQueue queue) { - List result = new ArrayList<>(); - while (!queue.isEmpty()) { - E item = queue.poll(); - if (item != null) { - result.add(item); - } - } - if (result.size() == 0) { - return null; - } - return result; - } - @UiThread @ThreadConfined(UI) private List getAndResetViewCommandMountItems() { - if (ReactFeatureFlags.enableLockFreeMountInstructions) { - return drainConcurrentItemQueue(mViewCommandMountItemsConcurrent); - } - synchronized (mViewCommandMountItemsLock) { List result = mViewCommandMountItems; if (result.isEmpty()) { @@ -822,10 +793,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @UiThread @ThreadConfined(UI) private List getAndResetMountItems() { - if (ReactFeatureFlags.enableLockFreeMountInstructions) { - return drainConcurrentItemQueue(mMountItemsConcurrent); - } - synchronized (mMountItemsLock) { List result = mMountItems; if (result.isEmpty()) { @@ -836,11 +803,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { } } - private Collection getAndResetPreMountItems() { - if (ReactFeatureFlags.enableLockFreeMountInstructions) { - return drainConcurrentItemQueue(mPreMountItemsConcurrent); - } - + private ArrayDeque getAndResetPreMountItems() { synchronized (mPreMountItemsLock) { ArrayDeque result = mPreMountItems; if (result.isEmpty()) { @@ -963,18 +926,19 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { // If there are MountItems to dispatch, we make sure all the "pre mount items" are executed // first - Collection preMountItemsToDispatch = getAndResetPreMountItems(); + ArrayDeque mPreMountItemsToDispatch = getAndResetPreMountItems(); - if (preMountItemsToDispatch != null) { + if (mPreMountItemsToDispatch != null) { Systrace.beginSection( Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "FabricUIManager::mountViews preMountItems to execute: " - + preMountItemsToDispatch.size()); + + mPreMountItemsToDispatch.size()); - for (PreAllocateViewMountItem preMountItem : preMountItemsToDispatch) { + while (!mPreMountItemsToDispatch.isEmpty()) { + PreAllocateViewMountItem mountItem = mPreMountItemsToDispatch.pollFirst(); if (surfaceActiveForExecution( - preMountItem.getRootTag(), "dispatchMountItems PreAllocateViewMountItem")) { - preMountItem.execute(mMountingManager); + mountItem.getRootTag(), "dispatchMountItems PreAllocateViewMountItem")) { + mountItem.execute(mMountingManager); } } @@ -1052,19 +1016,12 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { break; } - PreAllocateViewMountItem preMountItemToDispatch = null; - if (ReactFeatureFlags.enableLockFreeMountInstructions) { - preMountItemToDispatch = mPreMountItemsConcurrent.poll(); - } else { - synchronized (mPreMountItemsLock) { - if (!mPreMountItems.isEmpty()) { - preMountItemToDispatch = mPreMountItems.pollFirst(); - } + PreAllocateViewMountItem preMountItemToDispatch; + synchronized (mPreMountItemsLock) { + if (mPreMountItems.isEmpty()) { + break; } - } - // If list is empty, `poll` will return null, or var will never be set - if (preMountItemToDispatch == null) { - break; + preMountItemToDispatch = mPreMountItems.pollFirst(); } if (surfaceActiveForExecution( @@ -1175,14 +1132,18 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @AnyThread @ThreadConfined(ANY) private void dispatchCommandMountItem(DispatchCommandMountItem command) { - addViewCommandMountItem(command); + synchronized (mViewCommandMountItemsLock) { + mViewCommandMountItems.add(command); + } } @Override @AnyThread @ThreadConfined(ANY) public void sendAccessibilityEvent(int reactTag, int eventType) { - addMountItem(new SendAccessibilityEvent(reactTag, eventType)); + synchronized (mMountItemsLock) { + mMountItems.add(new SendAccessibilityEvent(reactTag, eventType)); + } } /** @@ -1195,13 +1156,15 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @DoNotStrip public void setJSResponder( final int reactTag, final int initialReactTag, final boolean blockNativeResponder) { - addMountItem( - new MountItem() { - @Override - public void execute(MountingManager mountingManager) { - mountingManager.setJSResponder(reactTag, initialReactTag, blockNativeResponder); - } - }); + synchronized (mMountItemsLock) { + mMountItems.add( + new MountItem() { + @Override + public void execute(MountingManager mountingManager) { + mountingManager.setJSResponder(reactTag, initialReactTag, blockNativeResponder); + } + }); + } } /** @@ -1210,13 +1173,15 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { */ @DoNotStrip public void clearJSResponder() { - addMountItem( - new MountItem() { - @Override - public void execute(MountingManager mountingManager) { - mountingManager.clearJSResponder(); - } - }); + synchronized (mMountItemsLock) { + mMountItems.add( + new MountItem() { + @Override + public void execute(MountingManager mountingManager) { + mountingManager.clearJSResponder(); + } + }); + } } @Override @@ -1264,51 +1229,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { return performanceCounters; } - /** - * Abstraction between concurrent and non-concurrent MountItem list. - * - * @param mountItem - */ - private void addMountItem(MountItem mountItem) { - if (ReactFeatureFlags.enableLockFreeMountInstructions) { - mMountItemsConcurrent.add(mountItem); - } else { - synchronized (mMountItemsLock) { - mMountItems.add(mountItem); - } - } - } - - /** - * Abstraction between concurrent and non-concurrent PreAllocateViewMountItem list. - * - * @param mountItem - */ - private void addPreAllocateMountItem(PreAllocateViewMountItem mountItem) { - if (ReactFeatureFlags.enableLockFreeMountInstructions) { - mPreMountItemsConcurrent.add(mountItem); - } else { - synchronized (mPreMountItemsLock) { - mPreMountItemsConcurrent.add(mountItem); - } - } - } - - /** - * Abstraction between concurrent and non-concurrent DispatchCommandMountItem list. - * - * @param mountItem - */ - private void addViewCommandMountItem(DispatchCommandMountItem mountItem) { - if (ReactFeatureFlags.enableLockFreeMountInstructions) { - mViewCommandMountItemsConcurrent.add(mountItem); - } else { - synchronized (mViewCommandMountItemsLock) { - mViewCommandMountItems.add(mountItem); - } - } - } - private class DispatchUIFrameCallback extends GuardedFrameCallback { private volatile boolean mIsMountingEnabled = true;