From 752e7ab1f9b787090abdc2f1b0debe0113efd04d Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Sat, 14 Nov 2020 12:18:05 -0800 Subject: [PATCH] Experiment to replace Fabric MountItem lists with concurrent queues (re-land) Summary: Currently, queuing any sort of MountItem or getting the list of MountItems requires synchronization. Since these can be queued up at any point from the JS thread, there will, in theory, be constant contention between JS and UI thread. To see if this is true, I'm creating an experiment instead of just switching over to concurrent structures. After seeing results, we can hopefully make a decision and delete the non-concurrent stuff or improve on it somehow. The original was unlanded in D24973616 (https://github.com/facebook/react-native/commit/26787e2260412d9d2fe831e68a8616505d3cab36) due to a typo, which has been fixed now. The typo was that in Blocking Mode, we queued up all PreMountItems to the concurrent PreMountItems queue. Changelog: [Internal] Reviewed By: ShikaSD Differential Revision: D24974851 fbshipit-source-id: d081c081bbd0de445bb92408f0ec822056b905a5 --- .../react/config/ReactFeatureFlags.java | 3 + .../react/fabric/FabricUIManager.java | 202 ++++++++++++------ 2 files changed, 144 insertions(+), 61 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 5a5cfeee2f7..d972c83330e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -88,4 +88,7 @@ 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 f769f708f43..76ab25a5463 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -89,10 +89,12 @@ 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") @@ -124,20 +126,35 @@ 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; + @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(); + @GuardedBy("mViewCommandMountItemsLock") @NonNull private List mViewCommandMountItems = new ArrayList<>(); - @NonNull - private final CopyOnWriteArrayList mListeners = new CopyOnWriteArrayList<>(); - @GuardedBy("mMountItemsLock") @NonNull private List mMountItems = new ArrayList<>(); @@ -339,18 +356,15 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { // possible at teardown, but this race should *never* happen at startup. @Nullable ThemedReactContext context = mReactContextForRootTag.get(rootTag); - String component = getFabricComponentName(componentName); - synchronized (mPreMountItemsLock) { - mPreMountItems.add( - new PreAllocateViewMountItem( - context, - rootTag, - reactTag, - component, - props, - (StateWrapper) stateWrapper, - isLayoutable)); - } + addPreAllocateMountItem( + new PreAllocateViewMountItem( + context, + rootTag, + reactTag, + getFabricComponentName(componentName), + props, + (StateWrapper) stateWrapper, + isLayoutable)); } @DoNotStrip @@ -616,9 +630,7 @@ 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)) { - synchronized (mMountItemsLock) { - mMountItems.add(synchronousMountItem); - } + addMountItem(synchronousMountItem); return; } @@ -692,9 +704,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { } if (shouldSchedule && mountItem != null) { - synchronized (mMountItemsLock) { - mMountItems.add(mountItem); - } + addMountItem(mountItem); if (UiThreadUtil.isOnUiThread()) { // We only read these flags on the UI thread. tryDispatchMountItems(); @@ -777,9 +787,28 @@ 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()) { @@ -793,6 +822,10 @@ 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()) { @@ -803,7 +836,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { } } - private ArrayDeque getAndResetPreMountItems() { + private Collection getAndResetPreMountItems() { + if (ReactFeatureFlags.enableLockFreeMountInstructions) { + return drainConcurrentItemQueue(mPreMountItemsConcurrent); + } + synchronized (mPreMountItemsLock) { ArrayDeque result = mPreMountItems; if (result.isEmpty()) { @@ -926,19 +963,18 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { // If there are MountItems to dispatch, we make sure all the "pre mount items" are executed // first - ArrayDeque mPreMountItemsToDispatch = getAndResetPreMountItems(); + Collection preMountItemsToDispatch = getAndResetPreMountItems(); - if (mPreMountItemsToDispatch != null) { + if (preMountItemsToDispatch != null) { Systrace.beginSection( Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "FabricUIManager::mountViews preMountItems to execute: " - + mPreMountItemsToDispatch.size()); + + preMountItemsToDispatch.size()); - while (!mPreMountItemsToDispatch.isEmpty()) { - PreAllocateViewMountItem mountItem = mPreMountItemsToDispatch.pollFirst(); + for (PreAllocateViewMountItem preMountItem : preMountItemsToDispatch) { if (surfaceActiveForExecution( - mountItem.getRootTag(), "dispatchMountItems PreAllocateViewMountItem")) { - mountItem.execute(mMountingManager); + preMountItem.getRootTag(), "dispatchMountItems PreAllocateViewMountItem")) { + preMountItem.execute(mMountingManager); } } @@ -1016,12 +1052,19 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { break; } - PreAllocateViewMountItem preMountItemToDispatch; - synchronized (mPreMountItemsLock) { - if (mPreMountItems.isEmpty()) { - break; + PreAllocateViewMountItem preMountItemToDispatch = null; + if (ReactFeatureFlags.enableLockFreeMountInstructions) { + preMountItemToDispatch = mPreMountItemsConcurrent.poll(); + } else { + synchronized (mPreMountItemsLock) { + if (!mPreMountItems.isEmpty()) { + preMountItemToDispatch = mPreMountItems.pollFirst(); + } } - preMountItemToDispatch = mPreMountItems.pollFirst(); + } + // If list is empty, `poll` will return null, or var will never be set + if (preMountItemToDispatch == null) { + break; } if (surfaceActiveForExecution( @@ -1132,18 +1175,14 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @AnyThread @ThreadConfined(ANY) private void dispatchCommandMountItem(DispatchCommandMountItem command) { - synchronized (mViewCommandMountItemsLock) { - mViewCommandMountItems.add(command); - } + addViewCommandMountItem(command); } @Override @AnyThread @ThreadConfined(ANY) public void sendAccessibilityEvent(int reactTag, int eventType) { - synchronized (mMountItemsLock) { - mMountItems.add(new SendAccessibilityEvent(reactTag, eventType)); - } + addMountItem(new SendAccessibilityEvent(reactTag, eventType)); } /** @@ -1156,15 +1195,13 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @DoNotStrip public void setJSResponder( final int reactTag, final int initialReactTag, final boolean blockNativeResponder) { - synchronized (mMountItemsLock) { - mMountItems.add( - new MountItem() { - @Override - public void execute(MountingManager mountingManager) { - mountingManager.setJSResponder(reactTag, initialReactTag, blockNativeResponder); - } - }); - } + addMountItem( + new MountItem() { + @Override + public void execute(MountingManager mountingManager) { + mountingManager.setJSResponder(reactTag, initialReactTag, blockNativeResponder); + } + }); } /** @@ -1173,15 +1210,13 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { */ @DoNotStrip public void clearJSResponder() { - synchronized (mMountItemsLock) { - mMountItems.add( - new MountItem() { - @Override - public void execute(MountingManager mountingManager) { - mountingManager.clearJSResponder(); - } - }); - } + addMountItem( + new MountItem() { + @Override + public void execute(MountingManager mountingManager) { + mountingManager.clearJSResponder(); + } + }); } @Override @@ -1229,6 +1264,51 @@ 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) { + mPreMountItems.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;