Differ: ensure ownership of all ShadowView/ShadowNode pairs, ensure consistency of nodes

Summary:
Previously, `ShadowViewNodePair::List` owned each `ShadowViewNodePair` but whenever we put `ShadowViewNodePair` into a TinyMap, those were unowned pointer references. This worked... 99% of the time. But in some marginal cases, it would cause dangling pointers, leading to difficult-to-track-down issues. So, I'm moving both of these to be unowned pointers and keeping a `std::deque` that owns all `ShadowViewNodePair`s and is itself owned by the main differ function. See comments for more implementation details. I'm moderately concerned about memory usage regressions, but practically speaking this will contain many items when a tree is created for the first time, and then very few items after that (space complexity should be similar to `O(n)` where `n` is the number of changed nodes after the last diff).

See comments as to why I believe `std::deque` is the right choice. Long-term there might be data-structures that are even more optimal, but std::deque has the right tradeoffs compared to other built-in STL structures like std::list and std::vector, and is probably better than std::forward_list too. Long-term we may want a custom data-structure that fits our needs exactly, but std::deque comes close and is possibly optimal.

Changelog: [Internal]

Reviewed By: sammy-SC

Differential Revision: D27730952

fbshipit-source-id: 2194b535439bd309803a221188da5db75242005a

ReactCommon/react/renderer/mounting/Differentiator.h

@@ -10,18 +10,42 @@
 #include <react/renderer/core/ShadowNode.h>
 #include <react/renderer/debug/flags.h>
 #include <react/renderer/mounting/ShadowViewMutation.h>
+#include <deque>
 
 namespace facebook {
 namespace react {
 
 enum class ReparentMode { Flatten, Unflatten };
 
+/**
+ * During differ, we need to keep some `ShadowViewNodePair`s in memory.
+ * Some `ShadowViewNodePair`s are referenced from std::vectors returned
+ * by `sliceChildShadowNodeViewPairsV2`; some are referenced in TinyMaps
+ * for view (un)flattening especially; and it is not always clear which
+ * std::vectors will outlive which TinyMaps, and vice-versa, so it doesn't
+ * make sense for the std::vector or TinyMap to own any `ShadowViewNodePair`s.
+ *
+ * Thus, we introduce the concept of a scope.
+ *
+ * For the duration of some operation, we keep a ViewNodePairScope around, such
+ * that: (1) the ViewNodePairScope keeps each
+ * ShadowViewNodePair alive, (2) we have a stable pointer value that we can
+ * use to reference each ShadowViewNodePair (not guaranteed with std::vector,
+ * for example, which may have to resize and move values around).
+ *
+ * As long as we only manipulate the data-structure with push_back, std::deque
+ * both (1) ensures that pointers into the data-structure are never invalidated,
+ * and (2) tries to efficiently allocate storage such that as many objects as
+ * possible are close in memory, but does not guarantee adjacency.
+ */
+using ViewNodePairScope = std::deque<ShadowViewNodePair>;
+
 /*
  * Calculates a list of view mutations which describes how the old
  * `ShadowTree` can be transformed to the new one.
  * The list of mutations might be and might not be optimal.
  */
-ShadowViewMutationList calculateShadowViewMutations(
+ShadowViewMutation::List calculateShadowViewMutations(
     ShadowNode const &oldRootShadowNode,
     ShadowNode const &newRootShadowNode,
     bool useNewDiffer);
@@ -31,15 +55,16 @@ ShadowViewMutationList calculateShadowViewMutations(
  * flattened view hierarchy. The V2 version preserves nodes even if they do
  * not form views and their children are flattened.
  */
-ShadowViewNodePair::List sliceChildShadowNodeViewPairsV2(
+ShadowViewNodePair::NonOwningList sliceChildShadowNodeViewPairsV2(
     ShadowNode const &shadowNode,
+    ViewNodePairScope &viewNodePairScope,
     bool allowFlattened = false);
 
 /*
  * Generates a list of `ShadowViewNodePair`s that represents a layer of a
  * flattened view hierarchy. This is *only* used by unit tests currently.
  */
-ShadowViewNodePair::List sliceChildShadowNodeViewPairsLegacy(
+ShadowViewNodePair::OwningList sliceChildShadowNodeViewPairsLegacy(
     ShadowNode const &shadowNode);
 
 } // namespace react

ReactCommon/react/renderer/mounting/DifferentiatorFlatteningClassic.cpp

@@ -178,15 +178,16 @@ class TinyMap final {
  * Sorting comparator for `reorderInPlaceIfNeeded`.
  */
 static bool shouldFirstPairComesBeforeSecondOne(
-    ShadowViewNodePair const &lhs,
-    ShadowViewNodePair const &rhs) noexcept {
+    ShadowViewNodePairLegacy const &lhs,
+    ShadowViewNodePairLegacy const &rhs) noexcept {
   return lhs.shadowNode->getOrderIndex() < rhs.shadowNode->getOrderIndex();
 }
 
 /*
  * Reorders pairs in-place based on `orderIndex` using a stable sort algorithm.
  */
-static void reorderInPlaceIfNeeded(ShadowViewNodePair::List &pairs) noexcept {
+static void reorderInPlaceIfNeeded(
+    ShadowViewNodePairLegacy::OwningList &pairs) noexcept {
   if (pairs.size() < 2) {
     return;
   }
@@ -208,7 +209,7 @@ static void reorderInPlaceIfNeeded(ShadowViewNodePair::List &pairs) noexcept {
 }
 
 static void sliceChildShadowNodeViewPairsRecursivelyV2(
-    ShadowViewNodePair::List &pairList,
+    ShadowViewNodePairLegacy::OwningList &pairList,
     Point layoutOffset,
     ShadowNode const &shadowNode) {
   for (auto const &sharedChildShadowNode : shadowNode.getChildren()) {
@@ -247,10 +248,10 @@ static void sliceChildShadowNodeViewPairsRecursivelyV2(
   }
 }
 
-ShadowViewNodePair::List sliceChildShadowNodeViewPairsV2(
+ShadowViewNodePairLegacy::OwningList sliceChildShadowNodeViewPairsV2(
     ShadowNode const &shadowNode,
     bool allowFlattened) {
-  auto pairList = ShadowViewNodePair::List{};
+  auto pairList = ShadowViewNodePairLegacy::OwningList{};
 
   if (!shadowNode.getTraits().check(
           ShadowNodeTraits::Trait::FormsStackingContext) &&
@@ -284,11 +285,11 @@ static_assert(
     std::is_move_constructible<ShadowView>::value,
     "`ShadowView` must be `move constructible`.");
 static_assert(
-    std::is_move_constructible<ShadowViewNodePair>::value,
-    "`ShadowViewNodePair` must be `move constructible`.");
+    std::is_move_constructible<ShadowViewNodePairLegacy>::value,
+    "`ShadowViewNodePairLegacy` must be `move constructible`.");
 static_assert(
-    std::is_move_constructible<ShadowViewNodePair::List>::value,
-    "`ShadowViewNodePair::List` must be `move constructible`.");
+    std::is_move_constructible<ShadowViewNodePairLegacy::OwningList>::value,
+    "`ShadowViewNodePairLegacy::OwningList` must be `move constructible`.");
 
 static_assert(
     std::is_move_assignable<ShadowViewMutation>::value,
@@ -297,19 +298,19 @@ static_assert(
     std::is_move_assignable<ShadowView>::value,
     "`ShadowView` must be `move assignable`.");
 static_assert(
-    std::is_move_assignable<ShadowViewNodePair>::value,
-    "`ShadowViewNodePair` must be `move assignable`.");
+    std::is_move_assignable<ShadowViewNodePairLegacy>::value,
+    "`ShadowViewNodePairLegacy` must be `move assignable`.");
 static_assert(
-    std::is_move_assignable<ShadowViewNodePair::List>::value,
-    "`ShadowViewNodePair::List` must be `move assignable`.");
+    std::is_move_assignable<ShadowViewNodePairLegacy::OwningList>::value,
+    "`ShadowViewNodePairLegacy::OwningList` must be `move assignable`.");
 
 // Forward declaration
 static void calculateShadowViewMutationsV2(
     BREADCRUMB_TYPE breadcrumb,
     ShadowViewMutation::List &mutations,
     ShadowView const &parentShadowView,
-    ShadowViewNodePair::List &&oldChildPairs,
-    ShadowViewNodePair::List &&newChildPairs);
+    ShadowViewNodePairLegacy::OwningList &&oldChildPairs,
+    ShadowViewNodePairLegacy::OwningList &&newChildPairs);
 
 struct OrderedMutationInstructionContainer {
   ShadowViewMutation::List &createMutations;
@@ -326,10 +327,11 @@ static void calculateShadowViewMutationsFlattener(
     ReparentMode reparentMode,
     OrderedMutationInstructionContainer &mutationInstructionContainer,
     ShadowView const &parentShadowView,
-    TinyMap<Tag, ShadowViewNodePair *> &unvisitedFlattenedNodes,
-    ShadowViewNodePair const &node,
-    TinyMap<Tag, ShadowViewNodePair *> *parentSubVisitedOtherNewNodes = nullptr,
-    TinyMap<Tag, ShadowViewNodePair *> *parentSubVisitedOtherOldNodes =
+    TinyMap<Tag, ShadowViewNodePairLegacy *> &unvisitedFlattenedNodes,
+    ShadowViewNodePairLegacy const &node,
+    TinyMap<Tag, ShadowViewNodePairLegacy *> *parentSubVisitedOtherNewNodes =
+        nullptr,
+    TinyMap<Tag, ShadowViewNodePairLegacy *> *parentSubVisitedOtherOldNodes =
         nullptr);
 
 /**
@@ -371,10 +373,10 @@ static void calculateShadowViewMutationsFlattener(
     ReparentMode reparentMode,
     OrderedMutationInstructionContainer &mutationInstructionContainer,
     ShadowView const &parentShadowView,
-    TinyMap<Tag, ShadowViewNodePair *> &unvisitedOtherNodes,
-    ShadowViewNodePair const &node,
-    TinyMap<Tag, ShadowViewNodePair *> *parentSubVisitedOtherNewNodes,
-    TinyMap<Tag, ShadowViewNodePair *> *parentSubVisitedOtherOldNodes) {
+    TinyMap<Tag, ShadowViewNodePairLegacy *> &unvisitedOtherNodes,
+    ShadowViewNodePairLegacy const &node,
+    TinyMap<Tag, ShadowViewNodePairLegacy *> *parentSubVisitedOtherNewNodes,
+    TinyMap<Tag, ShadowViewNodePairLegacy *> *parentSubVisitedOtherOldNodes) {
   DEBUG_LOGS({
     LOG(ERROR) << "Differ Flattener 1: "
                << (reparentMode == ReparentMode::Unflatten ? "Unflattening"
@@ -383,7 +385,7 @@ static void calculateShadowViewMutationsFlattener(
   });
 
   // Step 1: iterate through entire tree
-  ShadowViewNodePair::List treeChildren =
+  ShadowViewNodePairLegacy::OwningList treeChildren =
       sliceChildShadowNodeViewPairsV2(*node.shadowNode);
 
   DEBUG_LOGS({
@@ -414,8 +416,8 @@ static void calculateShadowViewMutationsFlattener(
   });
 
   // Views in other tree that are visited by sub-flattening or sub-unflattening
-  TinyMap<Tag, ShadowViewNodePair *> subVisitedOtherNewNodes{};
-  TinyMap<Tag, ShadowViewNodePair *> subVisitedOtherOldNodes{};
+  TinyMap<Tag, ShadowViewNodePairLegacy *> subVisitedOtherNewNodes{};
+  TinyMap<Tag, ShadowViewNodePairLegacy *> subVisitedOtherOldNodes{};
   auto subVisitedNewMap =
       (parentSubVisitedOtherNewNodes != nullptr ? parentSubVisitedOtherNewNodes
                                                 : &subVisitedOtherNewNodes);
@@ -425,7 +427,7 @@ static void calculateShadowViewMutationsFlattener(
 
   // Candidates for full tree creation or deletion at the end of this function
   auto deletionCreationCandidatePairs =
-      TinyMap<Tag, ShadowViewNodePair const *>{};
+      TinyMap<Tag, ShadowViewNodePairLegacy const *>{};
 
   for (size_t index = 0;
        index < treeChildren.size() && index < treeChildren.size();
@@ -553,7 +555,8 @@ static void calculateShadowViewMutationsFlattener(
           // Unflatten parent, flatten child
           if (childReparentMode == ReparentMode::Flatten) {
             // Construct unvisited nodes map
-            auto unvisitedNewChildPairs = TinyMap<Tag, ShadowViewNodePair *>{};
+            auto unvisitedNewChildPairs =
+                TinyMap<Tag, ShadowViewNodePairLegacy *>{};
             // Memory note: these oldFlattenedNodes all disappear at the end of
             // this "else" block, including any annotations we put on them.
             auto newFlattenedNodes = sliceChildShadowNodeViewPairsV2(
@@ -612,7 +615,8 @@ static void calculateShadowViewMutationsFlattener(
           // Flatten parent, unflatten child
           else {
             // Construct unvisited nodes map
-            auto unvisitedOldChildPairs = TinyMap<Tag, ShadowViewNodePair *>{};
+            auto unvisitedOldChildPairs =
+                TinyMap<Tag, ShadowViewNodePairLegacy *>{};
             // Memory note: these oldFlattenedNodes all disappear at the end of
             // this "else" block, including any annotations we put on them.
             auto oldFlattenedNodes = sliceChildShadowNodeViewPairsV2(
@@ -664,7 +668,7 @@ static void calculateShadowViewMutationsFlattener(
                   auto oldRemainingChildInListIt = std::find_if(
                       treeChildren.begin(),
                       treeChildren.end(),
-                      [&tag](ShadowViewNodePair &nodePair) {
+                      [&tag](ShadowViewNodePairLegacy &nodePair) {
                         return nodePair.shadowView.tag == tag;
                       });
                   if (oldRemainingChildInListIt != treeChildren.end()) {
@@ -816,8 +820,8 @@ static void calculateShadowViewMutationsV2(
     BREADCRUMB_TYPE breadcrumb,
     ShadowViewMutation::List &mutations,
     ShadowView const &parentShadowView,
-    ShadowViewNodePair::List &&oldChildPairs,
-    ShadowViewNodePair::List &&newChildPairs) {
+    ShadowViewNodePairLegacy::OwningList &&oldChildPairs,
+    ShadowViewNodePairLegacy::OwningList &&newChildPairs) {
   if (oldChildPairs.empty() && newChildPairs.empty()) {
     return;
   }
@@ -995,9 +999,10 @@ static void calculateShadowViewMutationsV2(
     }
   } else {
     // Collect map of tags in the new list
-    auto newRemainingPairs = TinyMap<Tag, ShadowViewNodePair *>{};
-    auto newInsertedPairs = TinyMap<Tag, ShadowViewNodePair *>{};
-    auto deletionCandidatePairs = TinyMap<Tag, ShadowViewNodePair const *>{};
+    auto newRemainingPairs = TinyMap<Tag, ShadowViewNodePairLegacy *>{};
+    auto newInsertedPairs = TinyMap<Tag, ShadowViewNodePairLegacy *>{};
+    auto deletionCandidatePairs =
+        TinyMap<Tag, ShadowViewNodePairLegacy const *>{};
     for (; index < newChildPairs.size(); index++) {
       auto &newChildPair = newChildPairs[index];
       newRemainingPairs.insert({newChildPair.shadowView.tag, &newChildPair});
@@ -1106,7 +1111,7 @@ static void calculateShadowViewMutationsV2(
             else {
               // Construct unvisited nodes map
               auto unvisitedOldChildPairs =
-                  TinyMap<Tag, ShadowViewNodePair *>{};
+                  TinyMap<Tag, ShadowViewNodePairLegacy *>{};
               // We don't know where all the children of oldChildPair are within
               // oldChildPairs, but we know that they're in the same relative
               // order. The reason for this is because of flattening + zIndex:
@@ -1236,7 +1241,7 @@ static void calculateShadowViewMutationsV2(
             else {
               // Construct unvisited nodes map
               auto unvisitedOldChildPairs =
-                  TinyMap<Tag, ShadowViewNodePair *>{};
+                  TinyMap<Tag, ShadowViewNodePairLegacy *>{};
               // We don't know where all the children of oldChildPair are within
               // oldChildPairs, but we know that they're in the same relative
               // order. The reason for this is because of flattening + zIndex:
@@ -1524,64 +1529,6 @@ static void calculateShadowViewMutationsV2(
       std::back_inserter(mutations));
 }
 
-/**
- * Only used by unit tests currently.
- */
-static void sliceChildShadowNodeViewPairsRecursivelyLegacy(
-    ShadowViewNodePair::List &pairList,
-    Point layoutOffset,
-    ShadowNode const &shadowNode) {
-  for (auto const &sharedChildShadowNode : shadowNode.getChildren()) {
-    auto &childShadowNode = *sharedChildShadowNode;
-
-#ifndef ANDROID
-    // Temporary disabled on Android because the mounting infrastructure
-    // is not fully ready yet.
-    if (childShadowNode.getTraits().check(ShadowNodeTraits::Trait::Hidden)) {
-      continue;
-    }
-#endif
-
-    auto shadowView = ShadowView(childShadowNode);
-    auto origin = layoutOffset;
-    if (shadowView.layoutMetrics != EmptyLayoutMetrics) {
-      origin += shadowView.layoutMetrics.frame.origin;
-      shadowView.layoutMetrics.frame.origin += layoutOffset;
-    }
-
-    if (childShadowNode.getTraits().check(
-            ShadowNodeTraits::Trait::FormsStackingContext)) {
-      pairList.push_back({shadowView, &childShadowNode});
-    } else {
-      if (childShadowNode.getTraits().check(
-              ShadowNodeTraits::Trait::FormsView)) {
-        pairList.push_back({shadowView, &childShadowNode});
-      }
-
-      sliceChildShadowNodeViewPairsRecursivelyLegacy(
-          pairList, origin, childShadowNode);
-    }
-  }
-}
-
-/**
- * Only used by unit tests currently.
- */
-ShadowViewNodePair::List sliceChildShadowNodeViewPairsLegacy(
-    ShadowNode const &shadowNode) {
-  auto pairList = ShadowViewNodePair::List{};
-
-  if (!shadowNode.getTraits().check(
-          ShadowNodeTraits::Trait::FormsStackingContext) &&
-      shadowNode.getTraits().check(ShadowNodeTraits::Trait::FormsView)) {
-    return pairList;
-  }
-
-  sliceChildShadowNodeViewPairsRecursivelyLegacy(pairList, {0, 0}, shadowNode);
-
-  return pairList;
-}
-
 ShadowViewMutation::List calculateShadowViewMutations(
     ShadowNode const &oldRootShadowNode,
     ShadowNode const &newRootShadowNode) {

ReactCommon/react/renderer/mounting/DifferentiatorFlatteningClassic.h

@@ -36,17 +36,10 @@ ShadowViewMutationList calculateShadowViewMutations(
  * flattened view hierarchy. The V2 version preserves nodes even if they do
  * not form views and their children are flattened.
  */
-ShadowViewNodePair::List sliceChildShadowNodeViewPairsV2(
+ShadowViewNodePairLegacy::OwningList sliceChildShadowNodeViewPairsV2(
     ShadowNode const &shadowNode,
     bool allowFlattened = false);
 
-/*
- * Generates a list of `ShadowViewNodePair`s that represents a layer of a
- * flattened view hierarchy. This is *only* used by unit tests currently.
- */
-ShadowViewNodePair::List sliceChildShadowNodeViewPairsLegacy(
-    ShadowNode const &shadowNode);
-
 } // namespace DifferOld
 } // namespace react
 } // namespace facebook

ReactCommon/react/renderer/mounting/ShadowView.cpp

@@ -83,5 +83,15 @@ bool ShadowViewNodePair::operator!=(const ShadowViewNodePair &rhs) const {
   return !(*this == rhs);
 }
 
+bool ShadowViewNodePairLegacy::operator==(
+    const ShadowViewNodePairLegacy &rhs) const {
+  return this->shadowNode == rhs.shadowNode;
+}
+
+bool ShadowViewNodePairLegacy::operator!=(
+    const ShadowViewNodePairLegacy &rhs) const {
+  return !(*this == rhs);
+}
+
 } // namespace react
 } // namespace facebook

ReactCommon/react/renderer/mounting/ShadowView.h

@@ -64,7 +64,9 @@ std::vector<DebugStringConvertibleObject> getDebugProps(
  *
  */
 struct ShadowViewNodePair final {
-  using List = better::
+  using NonOwningList = better::
+      small_vector<ShadowViewNodePair *, kShadowNodeChildrenSmallVectorSize>;
+  using OwningList = better::
       small_vector<ShadowViewNodePair, kShadowNodeChildrenSmallVectorSize>;
 
   ShadowView shadowView;
@@ -74,21 +76,49 @@ struct ShadowViewNodePair final {
 
   size_t mountIndex{0};
 
+  /**
+   * This is nullptr unless `inOtherTree` is set to true.
+   * We rely on this only for marginal cases. TODO: could we
+   * rely on this more heavily to simplify the diffing algorithm
+   * overall?
+   */
+  ShadowViewNodePair const *otherTreePair{nullptr};
+
+  /*
+   * The stored pointer to `ShadowNode` represents an identity of the pair.
+   */
+  bool operator==(const ShadowViewNodePair &rhs) const;
+  bool operator!=(const ShadowViewNodePair &rhs) const;
+
+  bool inOtherTree() const {
+    return this->otherTreePair != nullptr;
+  }
+};
+
+/*
+ * Describes pair of a `ShadowView` and a `ShadowNode`.
+ * This is not exposed to the mounting layer.
+ *
+ */
+struct ShadowViewNodePairLegacy final {
+  using OwningList = better::small_vector<
+      ShadowViewNodePairLegacy,
+      kShadowNodeChildrenSmallVectorSize>;
+
+  ShadowView shadowView;
+  ShadowNode const *shadowNode;
+  bool flattened{false};
+  bool isConcreteView{true};
+
+  size_t mountIndex{0};
+
   bool inOtherTree{false};
 
-  /**
-   * This is nullptr unless `inOtherTree` is set to true.
-   * We rely on this only for marginal cases. TODO: could we
-   * rely on this more heavily to simplify the diffing algorithm
-   * overall?
-   */
-  ShadowNode const *otherTreeShadowNode{nullptr};
-
   /*
    * The stored pointer to `ShadowNode` represents an identity of the pair.
    */
-  bool operator==(const ShadowViewNodePair &rhs) const;
-  bool operator!=(const ShadowViewNodePair &rhs) const;
+  bool operator==(const ShadowViewNodePairLegacy &rhs) const;
+  bool operator!=(const ShadowViewNodePairLegacy &rhs) const;
 };
 
 } // namespace react

ReactCommon/react/renderer/mounting/stubs.cpp

@@ -26,7 +26,8 @@ static bool shouldFirstPairComesBeforeSecondOne(
 /*
  * Reorders pairs in-place based on `orderIndex` using a stable sort algorithm.
  */
-static void reorderInPlaceIfNeeded(ShadowViewNodePair::List &pairs) noexcept {
+static void reorderInPlaceIfNeeded(
+    ShadowViewNodePair::OwningList &pairs) noexcept {
   // This is a simplified version of the function intentionally copied from
   // `Differentiator.cpp`.
   std::stable_sort(
@@ -42,7 +43,7 @@ static void reorderInPlaceIfNeeded(ShadowViewNodePair::List &pairs) noexcept {
 static void calculateShadowViewMutationsForNewTree(
     ShadowViewMutation::List &mutations,
     ShadowView const &parentShadowView,
-    ShadowViewNodePair::List newChildPairs) {
+    ShadowViewNodePair::OwningList newChildPairs) {
   // Sorting pairs based on `orderIndex` if needed.
   reorderInPlaceIfNeeded(newChildPairs);