Back out "Experiment to replace Fabric MountItem lists with concurren…

…t queues" Summary: Changelog: [Internal] Original commit changeset: fcbdeda51f91 Reviewed By: rubennorte Differential Revision: D24973616 fbshipit-source-id: 4d21211d329c77dba50972a26b1daeccfffad912
facebook · Nov 14, 2020 · 26787e2 · 26787e2
1 parent d85d5d2
commit 26787e2
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 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
@@ -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
@@ -89,12 +89,10 @@
 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<UIManagerListener> mListeners = new CopyOnWriteArrayList<>();
-
-  // Concurrent MountItem data-structures, experimental. TODO: T79662803
-  @NonNull
-  private final ConcurrentLinkedQueue<DispatchCommandMountItem> mViewCommandMountItemsConcurrent =
-      new ConcurrentLinkedQueue<>();
-
-  @NonNull
-  private final ConcurrentLinkedQueue<MountItem> mMountItemsConcurrent =
-      new ConcurrentLinkedQueue<>();
-
-  @NonNull
-  private final ConcurrentLinkedQueue<PreAllocateViewMountItem> 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<DispatchCommandMountItem> mViewCommandMountItems = new ArrayList<>();
 
+  @NonNull
+  private final CopyOnWriteArrayList<UIManagerListener> mListeners = new CopyOnWriteArrayList<>();
+
   @GuardedBy("mMountItemsLock")
   @NonNull
   private List<MountItem> mMountItems = new ArrayList<>();
@@ -356,15 +339,18 @@ private void preallocateView(
     // 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 void execute(@NonNull MountingManager mountingManager) {
     // 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 @@ private void scheduleMountItem(
     }
 
     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 @@ private void tryDispatchMountItems() {
     mLastExecutedMountItemSurfaceId = -1;
   }
 
-  @Nullable
-  private <E extends MountItem> List<E> drainConcurrentItemQueue(ConcurrentLinkedQueue<E> queue) {
-    List<E> 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<DispatchCommandMountItem> getAndResetViewCommandMountItems() {
-    if (ReactFeatureFlags.enableLockFreeMountInstructions) {
-      return drainConcurrentItemQueue(mViewCommandMountItemsConcurrent);
-    }
-
     synchronized (mViewCommandMountItemsLock) {
       List<DispatchCommandMountItem> result = mViewCommandMountItems;
       if (result.isEmpty()) {
@@ -822,10 +793,6 @@ private List<DispatchCommandMountItem> getAndResetViewCommandMountItems() {
   @UiThread
   @ThreadConfined(UI)
   private List<MountItem> getAndResetMountItems() {
-    if (ReactFeatureFlags.enableLockFreeMountInstructions) {
-      return drainConcurrentItemQueue(mMountItemsConcurrent);
-    }
-
     synchronized (mMountItemsLock) {
       List<MountItem> result = mMountItems;
       if (result.isEmpty()) {
@@ -836,11 +803,7 @@ private List<MountItem> getAndResetMountItems() {
     }
   }
 
-  private Collection<PreAllocateViewMountItem> getAndResetPreMountItems() {
-    if (ReactFeatureFlags.enableLockFreeMountInstructions) {
-      return drainConcurrentItemQueue(mPreMountItemsConcurrent);
-    }
-
+  private ArrayDeque<PreAllocateViewMountItem> getAndResetPreMountItems() {
     synchronized (mPreMountItemsLock) {
       ArrayDeque<PreAllocateViewMountItem> result = mPreMountItems;
       if (result.isEmpty()) {
@@ -963,18 +926,19 @@ private boolean dispatchMountItems() {
 
     // If there are MountItems to dispatch, we make sure all the "pre mount items" are executed
     // first
-    Collection<PreAllocateViewMountItem> preMountItemsToDispatch = getAndResetPreMountItems();
+    ArrayDeque<PreAllocateViewMountItem> 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 @@ private void dispatchPreMountItems(long frameTimeNanos) {
           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 void dispatchCommand(
   @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 void sendAccessibilityEvent(int reactTag, int eventType) {
   @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 void execute(MountingManager mountingManager) {
    */
   @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 Map<String, Long> getPerformanceCounters() {
     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;