From 40b36a040f06aae03be1a09f14e48eb0c652de96 Mon Sep 17 00:00:00 2001
From: Joshua Gross <joshuagross@fb.com>
Date: Thu, 11 Jun 2020 20:43:29 -0700
Subject: [PATCH] For lifecycle correctness, call scheduleMountItems even with
 empty BatchMountItem

Summary:
In particular, NativeAnimatedModule relies on having some signal of when there's a commit from ReactJS. In Fabric, this is the only reliable signal. Failing to call scheduleMountItems even when there's no changes to the tree will result in certain animation operations being delayed way longer than necessary.

I pass nullptr instead of empty arrays in some cases to hopefully improve perf.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D22008981

fbshipit-source-id: 6bdeb46e03b5138dbfa5b077952ec0fa3dfe1eb3
---
 .../react/fabric/FabricUIManager.java         | 24 +++++++++++++------
 .../com/facebook/react/fabric/jni/Binding.cpp | 10 +++-----
 .../mounting/mountitems/BatchMountItem.java   | 12 ++++++----
 3 files changed, 27 insertions(+), 19 deletions(-)

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 a238799e185..f70b0fc5632 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java
@@ -542,7 +542,8 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
 
   /**
    * This method enqueues UI operations directly to the UI thread. This might change in the future
-   * to enforce execution order using {@link ReactChoreographer#CallbackType}.
+   * to enforce execution order using {@link ReactChoreographer#CallbackType}. This method should
+   * only be called as the result of a new tree being committed.
    */
   @DoNotStrip
   @SuppressWarnings("unused")
@@ -562,6 +563,13 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
     // a BatchMountItem. No other sites call into this with a BatchMountItem, and Binding.cpp only
     // calls scheduleMountItems with a BatchMountItem.
     boolean isBatchMountItem = mountItem instanceof BatchMountItem;
+    boolean shouldSchedule = !(isBatchMountItem && ((BatchMountItem) mountItem).getSize() == 0);
+
+    // In case of sync rendering, this could be called on the UI thread. Otherwise,
+    // it should ~always be called on the JS thread.
+    for (UIManagerListener listener : mListeners) {
+      listener.didScheduleMountItems(this);
+    }
 
     if (isBatchMountItem) {
       mCommitStartTime = commitStartTime;
@@ -571,13 +579,15 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
       mDispatchViewUpdatesTime = SystemClock.uptimeMillis();
     }
 
-    synchronized (mMountItemsLock) {
-      mMountItems.add(mountItem);
-    }
+    if (shouldSchedule) {
+      synchronized (mMountItemsLock) {
+        mMountItems.add(mountItem);
+      }
 
-    if (UiThreadUtil.isOnUiThread()) {
-      // We only read these flags on the UI thread.
-      tryDispatchMountItems();
+      if (UiThreadUtil.isOnUiThread()) {
+        // We only read these flags on the UI thread.
+        tryDispatchMountItems();
+      }
     }
 
     // Post markers outside of lock and after sync mounting finishes its execution
diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp
index 648272fffc1..156cac88d5b 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp
+++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp
@@ -842,22 +842,18 @@ void Binding::schedulerDidFinishTransaction(
     toRemove.clear();
   }
 
-  if (position <= 0) {
-    // If there are no mountItems to be sent to the platform, then it is not
-    // necessary to even call.
-    return;
-  }
-
   static auto createMountItemsBatchContainer =
       jni::findClassStatic(UIManagerJavaDescriptor)
           ->getMethod<alias_ref<JMountItem>(
               jint, jtypeArray<JMountItem::javaobject>, jint, jint)>(
               "createBatchMountItem");
 
+  // If there are no items, we pass a nullptr instead of passing the object
+  // through the JNI
   auto batch = createMountItemsBatchContainer(
       localJavaUIManager,
       surfaceId,
-      mountItemsArray.get(),
+      position == 0 ? nullptr : mountItemsArray.get(),
       position,
       commitNumber);
 
diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/BatchMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/BatchMountItem.java
index 3ab5b864cca..91e736d5335 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/BatchMountItem.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/BatchMountItem.java
@@ -34,12 +34,10 @@ public class BatchMountItem implements MountItem {
   private final int mCommitNumber;
 
   public BatchMountItem(int rootTag, MountItem[] items, int size, int commitNumber) {
-    if (items == null) {
-      throw new NullPointerException();
-    }
-    if (size < 0 || size > items.length) {
+    int itemsLength = (items == null ? 0 : items.length);
+    if (size < 0 || size > itemsLength) {
       throw new IllegalArgumentException(
-          "Invalid size received by parameter size: " + size + " items.size = " + items.length);
+          "Invalid size received by parameter size: " + size + " items.size = " + itemsLength);
     }
     mRootTag = rootTag;
     mMountItems = items;
@@ -74,6 +72,10 @@ public class BatchMountItem implements MountItem {
     return mRootTag;
   }
 
+  public int getSize() {
+    return mSize;
+  }
+
   @Override
   public String toString() {
     StringBuilder s = new StringBuilder();