Fix FlatUIImplementation.manageChildren() failing when moveFrom is not sorted

Summary:
Code in FlatUIImplementation.manageChildren() incorrectly assumed that moveFrom is sorted and moveTo is not, which is actually reverse: moveFrom is not sorted, and moveTo is. This means that we need to sort moveFrom before we can traverse it, and that we no longer need to sort moveTo (we did it by moving nodes to mNodesToMove first and then sorting it).

The sorting algorithm used is borrowed from Android implementation of insertion sort used in DualPivotQuicksort.doSort() when number of elements < INSERTION_SORT_THRESHOLD(32) which is 99.999% the case in UIImplementation.manageChildren() (most of the time this array is either empty or only contains 1 element).

Another (very rare) bug this is fixing is that the code only worked for FlatShadowNodes, but not all shadow nodes are FlatShadowNodes (there are rare exceptions, such as ARTShape, ARTGroup etc). New code works with all types of shadow nodes.

Reviewed By: ahmedre

Differential Revision: D2975787

ReactAndroid/src/main/java/com/facebook/react/flat/FlatShadowNode.java

@@ -49,7 +49,6 @@ import com.facebook.react.uimanager.annotations.ReactProp;
   private boolean mBackingViewIsCreated;
   private @Nullable DrawView mDrawView;
   private @Nullable DrawBackgroundColor mDrawBackground;
-  private int mMoveToIndexInParent;
   private boolean mClipToBounds = false;
   private boolean mIsUpdated = true;
   private float mClipLeft;
@@ -252,14 +251,6 @@ import com.facebook.react.uimanager.annotations.ReactProp;
     mNodeRegions = nodeRegion;
   }
 
-  /* package */ final void setMoveToIndexInParent(int moveToIndexInParent) {
-    mMoveToIndexInParent = moveToIndexInParent;
-  }
-
-  /* package */ final int getMoveToIndexInParent() {
-    return mMoveToIndexInParent;
-  }
-
   /* package */ void updateNodeRegion(float left, float top, float right, float bottom) {
     if (mNodeRegion.mLeft != left || mNodeRegion.mTop != top ||
         mNodeRegion.mRight != right || mNodeRegion.mBottom != bottom) {

ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIImplementation.java

@@ -12,8 +12,6 @@ package com.facebook.react.flat;
 import javax.annotation.Nullable;
 
 import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Comparator;
 import java.util.List;
 
 import com.facebook.infer.annotation.Assertions;
@@ -34,14 +32,6 @@ import com.facebook.react.views.image.ReactImageManager;
  * for faster drawing and interactions.
  */
 public class FlatUIImplementation extends UIImplementation {
-  /**
-   * This Comparator allows sorting FlatShadowNode by order in which they should be added.
-   */
-  private static final Comparator<FlatShadowNode> COMPARATOR = new Comparator<FlatShadowNode>() {
-    public int compare(FlatShadowNode lhs, FlatShadowNode rhs) {
-      return lhs.getMoveToIndexInParent() - rhs.getMoveToIndexInParent();
-    }
-  };
 
   public static FlatUIImplementation createInstance(
       ReactApplicationContext reactContext,
@@ -77,10 +67,10 @@ public class FlatUIImplementation extends UIImplementation {
   }
 
   /**
-   * Temporary storage for elements that need to be moved within a parent.
-   * Only used inside #manageChildren() and always empty outside of it.
+   * Helper class that sorts moveTo/moveFrom arrays passed to #manageChildren().
+   * Not used outside of the said method.
    */ 
-  private final ArrayList<FlatShadowNode> mNodesToMove = new ArrayList<>();
+  private final MoveProxy mMoveProxy = new MoveProxy();
   private final StateBuilder mStateBuilder;
   private @Nullable ReactImageManager mReactImageManager;
 
@@ -224,15 +214,10 @@ public class FlatUIImplementation extends UIImplementation {
 
     int prevIndex = Integer.MAX_VALUE;
 
-    int moveFromIndex;
-    int moveFromChildIndex;
-    if (moveFrom == null) {
-      moveFromIndex = -1;
-      moveFromChildIndex = -1;
-    } else {
-      moveFromIndex = moveFrom.size() - 1;
-      moveFromChildIndex = moveFrom.getInt(moveFromIndex);
-    }
+    mMoveProxy.setup(moveFrom, moveTo);
+
+    int moveFromIndex = mMoveProxy.size() - 1;
+    int moveFromChildIndex = (moveFromIndex == -1) ? -1 : mMoveProxy.getMoveFrom(moveFromIndex);
 
     int removeFromIndex;
     int removeFromChildIndex;
@@ -249,12 +234,11 @@ public class FlatUIImplementation extends UIImplementation {
 
     while (true) {
       if (moveFromChildIndex > removeFromChildIndex) {
-        int indexInParent = moveTo.getInt(moveFromIndex);
-        moveChild(removeChildAt(parentNode, moveFromChildIndex, prevIndex), indexInParent);
+        moveChild(removeChildAt(parentNode, moveFromChildIndex, prevIndex), moveFromIndex);
         prevIndex = moveFromChildIndex;
 
         --moveFromIndex;
-        moveFromChildIndex = (moveFromIndex == -1) ? -1 : moveFrom.getInt(moveFromIndex);
+        moveFromChildIndex = (moveFromIndex == -1) ? -1 : mMoveProxy.getMoveFrom(moveFromIndex);
       } else if (removeFromChildIndex > moveFromChildIndex) {
         removeChild(removeChildAt(parentNode, removeFromChildIndex, prevIndex));
         prevIndex = removeFromChildIndex;
@@ -273,29 +257,33 @@ public class FlatUIImplementation extends UIImplementation {
    * Unregisters given element and all of its children from ShadowNodeRegistry,
    * and drops all Views used by it and its children.
    */
-  private void removeChild(FlatShadowNode child) {
-    if (child.mountsToView() && child.isBackingViewCreated()) {
-      // this will recursively drop all subviews
-      mStateBuilder.dropView(child);
-    } else {
-      for (int i = 0, childCount = child.getChildCount(); i != childCount; ++i) {
-        removeChild((FlatShadowNode) child.getChildAt(i));
+  private void removeChild(ReactShadowNode child) {
+    if (child instanceof FlatShadowNode) {
+      FlatShadowNode node = (FlatShadowNode) child;
+      if (node.mountsToView() && node.isBackingViewCreated()) {
+        // this will recursively drop all subviews
+        mStateBuilder.dropView(node);
+        removeShadowNode(node);
+        return;
       }
     }
 
+    for (int i = 0, childCount = child.getChildCount(); i != childCount; ++i) {
+      removeChild(child.getChildAt(i));
+    }
+
     removeShadowNode(child);
   }
 
   /**
    * Prepares a given element to be moved to a new position.
    */
-  private void moveChild(FlatShadowNode child, int indexInParent) {
-    child.setMoveToIndexInParent(indexInParent);
-    mNodesToMove.add(child);
+  private void moveChild(ReactShadowNode child, int moveFromIndex) {
+    mMoveProxy.setChildMoveFrom(moveFromIndex, child);
   }
 
   /**
-   * Adds all children from addChildTags and mNodesToMove, populated by removeChildren.
+   * Adds all children from addChildTags and moveFrom/moveTo.
    */
   private void addChildren(
       ReactShadowNode parentNode,
@@ -304,22 +292,14 @@ public class FlatUIImplementation extends UIImplementation {
 
     int prevIndex = -1;
 
-    int numNodesToMove = mNodesToMove.size();
     int moveToIndex;
     int moveToChildIndex;
-    FlatShadowNode moveToChild;
-    if (numNodesToMove == 0) {
+    if (mMoveProxy.size() == 0) {
       moveToIndex = Integer.MAX_VALUE;
-      moveToChild = null;
       moveToChildIndex = Integer.MAX_VALUE;
     } else {
-      if (numNodesToMove > 1) {
-        // mNodesToMove is not sorted, so do it now.
-        Collections.sort(mNodesToMove, COMPARATOR);
-      }
       moveToIndex = 0;
-      moveToChild = mNodesToMove.get(0);
-      moveToChildIndex = moveToChild.getMoveToIndexInParent();
+      moveToChildIndex = mMoveProxy.getMoveTo(0);
     }
 
     int numNodesToAdd;
@@ -335,7 +315,7 @@ public class FlatUIImplementation extends UIImplementation {
       addToChildIndex = addAtIndices.getInt(0);
     }
 
-    // both mNodesToMove and addChildTags are already sorted, but combined order is not sorted. Use
+    // both mMoveProxy and addChildTags are already sorted, but combined order is not sorted. Use
     // a merge step from mergesort to walk over both arrays and extract elements in sorted order.
 
     while (true) {
@@ -351,15 +331,15 @@ public class FlatUIImplementation extends UIImplementation {
           addToChildIndex = addAtIndices.getInt(addToIndex);
         }
       } else if (moveToChildIndex < addToChildIndex) {
+        ReactShadowNode moveToChild = mMoveProxy.getChildMoveTo(moveToIndex);
         addChildAt(parentNode, moveToChild, moveToChildIndex, prevIndex);
         prevIndex = moveToChildIndex;
 
         ++moveToIndex;
-        if (moveToIndex == numNodesToMove) {
+        if (moveToIndex == mMoveProxy.size()) {
           moveToChildIndex = Integer.MAX_VALUE;
         } else {
-          moveToChild = mNodesToMove.get(moveToIndex);
-          moveToChildIndex = moveToChild.getMoveToIndexInParent();
+          moveToChildIndex = mMoveProxy.getMoveTo(moveToIndex);
         }
       } else {
         // moveToChildIndex == addToChildIndex can only be if both are equal to Integer.MAX_VALUE
@@ -367,14 +347,12 @@ public class FlatUIImplementation extends UIImplementation {
         break;
       }
     }
-
-    mNodesToMove.clear();
   }
 
   /**
    * Removes a child from parent, verifying that we are removing in descending order.
    */
-  private static FlatShadowNode removeChildAt(
+  private static ReactShadowNode removeChildAt(
       ReactShadowNode parentNode,
       int index,
       int prevIndex) {
@@ -383,7 +361,7 @@ public class FlatUIImplementation extends UIImplementation {
           "Invariant failure, needs sorting! " + index + " >= " + prevIndex);
     }
 
-    return (FlatShadowNode) parentNode.removeChildAt(index);
+    return parentNode.removeChildAt(index);
   }
 
   /**

ReactAndroid/src/main/java/com/facebook/react/flat/MoveProxy.java

@@ -0,0 +1,170 @@
+/**
+ * Copyright (c) 2015-present, Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package com.facebook.react.flat;
+
+import javax.annotation.Nullable;
+
+import com.facebook.infer.annotation.Assertions;
+import com.facebook.react.bridge.ReadableArray;
+import com.facebook.react.uimanager.ReactShadowNode;
+
+/**
+ * Helper class that sorts moveFrom/moveTo arrays in lockstep.
+ */
+/* package */ final class MoveProxy {
+
+  private @Nullable ReadableArray mMoveTo;
+  private int mSize;
+  private int[] mMapping = new int[8];
+  private ReactShadowNode[] mChildren = new ReactShadowNode[4];
+
+  /**
+   * Retuns size of underlying moveTo/moveFrom arrays
+   */
+  public int size() {
+    return mSize;
+  }
+
+  /**
+   * Assigns ith child that we want to move if moveFrom was sorted.
+   */
+  public void setChildMoveFrom(int moveFromIndex, ReactShadowNode node) {
+    mChildren[moveFromToIndex(moveFromIndex)] = node;
+  }
+
+  /**
+   * Returns ith child that we want to move if moveTo was sorted.
+   */
+  public ReactShadowNode getChildMoveTo(int moveToIndex) {
+    return mChildren[moveToToIndex(moveToIndex)];
+  }
+
+  /**
+   * Returns index of the ith child that we want to move if moveFrom was sorted
+   */
+  public int getMoveFrom(int moveFromIndex) {
+    return moveFromToValue(moveFromIndex);
+  }
+
+  /**
+   * Returns index of the ith child that we want to move to if moveTo was sorted
+   */
+  public int getMoveTo(int moveToIndex) {
+    return moveToToValue(moveToIndex);
+  }
+
+  /**
+   * Initialize MoveProxy with given moveFrom and moveTo arrays.
+   */
+  public void setup(ReadableArray moveFrom, ReadableArray moveTo) {
+    mMoveTo = moveTo;
+
+    if (moveFrom == null) {
+      setSize(0);
+      return;
+    }
+
+    int size = moveFrom.size();
+    int requiredSpace = size + size;
+    if (mMapping.length < requiredSpace) {
+      mMapping = new int[requiredSpace];
+      mChildren = new FlatShadowNode[size];
+    }
+
+    setSize(size);
+
+    // Array contains data in the following way:
+    // [ k0, v0, k1, v1, k2, v2, ... ]
+    //
+    // where vi = moveFrom.getInt(ki)
+
+    // We don't technically *need* to store vi, but they are accessed so often that it makes sense
+    // to cache it instead of calling ReadableArray.getInt() all the time.
+
+    // Sorting algorithm will reorder ki/vi pairs in such a way that vi < v(i+1)
+
+    // Code below is an insertion sort, adapted from DualPivotQuicksort.doSort()
+
+    // At each step i, we got the following data:
+
+    // [k0, v0, k1, v2, .. k(i-1), v(i-1), unused...]
+    // where v0 < v1 < v2 ... < v(i-1)
+    //
+    // This holds true for step i = 0 (array of size one is sorted)
+    // Again, k0 = 0, v0 = moveFrom.getInt(k0)
+    setKeyValue(0, 0, moveFrom.getInt(0));
+
+    // At each of the next steps, we grab a new key and walk back until we find first key that is
+    // less than current, shifting key/value pairs if they are larger than current key.
+    for (int i = 1; i < size; i++) {
+      // this is our next key
+      int current = moveFrom.getInt(i);
+
+      // this loop will find correct position for it
+      int j;
+
+      // At this point, array is like this: [ k0, v0, k1, v1, k2, v2, ..., k(i-1), v(i-1), ... ]
+      for (j = i - 1; j >= 0; j--) {
+        if (moveFromToValue(j) < current) {
+          break;
+        }
+
+        // value at index j is < current value, shift that value and its key
+        setKeyValue(j + 1, moveFromToIndex(j), moveFromToValue(j));
+      }
+
+      setKeyValue(j + 1, i, current);
+    }
+  }
+
+  /**
+   * Returns index of ith key in array.
+   */
+  private static int k(int i) {
+    return i * 2;
+  }
+
+  /**
+   * Returns index of ith value in array.
+   */
+  private static int v(int i) {
+    return i * 2 + 1;
+  }
+
+  private void setKeyValue(int index, int key, int value) {
+    mMapping[k(index)] = key;
+    mMapping[v(index)] = value;
+  }
+
+  private int moveFromToIndex(int index) {
+    return mMapping[k(index)];
+  }
+
+  private int moveFromToValue(int index) {
+    return mMapping[v(index)];
+  }
+
+  private static int moveToToIndex(int index) {
+    return index;
+  }
+
+  private int moveToToValue(int index) {
+    return Assertions.assumeNotNull(mMoveTo).getInt(index);
+  }
+
+  private void setSize(int newSize) {
+    // reset references to null when shrinking to avoid memory leaks
+    for (int i = newSize; i < mSize; ++i) {
+      mChildren[i] = null;
+    }
+
+    mSize = newSize;
+  }
+}