From afc3c97111c4897998227f9fc646fb420a494ca2 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Wed, 29 Apr 2020 20:53:15 -0700 Subject: [PATCH] Fabric: Changes in State reconciliation Summary: I spent the last several days thinking about state reconciliation issues, some crashes (T65586949) that suspiciously happen somewhere inside, and a bunch of issues that might be connected to that (possibly, some of T65516263 sub-task). I cannot see some obvious problems in the current state reconciliation algorithm that might cause the crash (because of some use-after-free or other pure C++ issues), but I suspect some of the problems we experience might be caused by some details of how we reconcile states. In the current approach, we rank all states based on the "hierarchical" history of their creation (state version is being calculated based on the version of the base tree). That's usually fine but in some cases when trees are being constructed concurrently, a logical version of a based tree does not correspond to the local version of a committed tree. In other words, the linear history of commits does not always correspond to the "hierarchical" history of trees generation that was done by different parties (e.g. React vs native state update pipeline). In this diff, I tried to change the approach to change the algorithm to follow this logic: If some state is `obsolete` (already been committed and then replaced with newer one), we replace that with the most recent one. This change does not introduce the `obsolete` flag; is already used by State infra to avoid cloning nodes with an outdated state. Interestingly, it fixes the issue with an empty BottomSheet on Android (T66177144). See the attached video. The hope is that it's also will This change theoretically might affect all things that use State, so it hard to predict what can break and how. So, if we don't see obvious problems here, I would set up a GK/QE and run the experiment in prod. Changelog: [Internal] Fabric-specific internal change. Reviewed By: JoshuaGross Differential Revision: D21295137 fbshipit-source-id: e5613218d3e11a56623cab9bbf2540495b2b24e8 --- .../core/shadownode/ShadowNodeFamily.cpp | 9 + .../fabric/core/shadownode/ShadowNodeFamily.h | 9 + ReactCommon/fabric/core/state/State.cpp | 9 + ReactCommon/fabric/core/state/State.h | 6 + ReactCommon/fabric/mounting/ShadowTree.cpp | 158 +++++++++++++++++- ReactCommon/fabric/mounting/ShadowTree.h | 1 + 6 files changed, 185 insertions(+), 7 deletions(-) diff --git a/ReactCommon/fabric/core/shadownode/ShadowNodeFamily.cpp b/ReactCommon/fabric/core/shadownode/ShadowNodeFamily.cpp index 2dd49e53065..fe2aaa84a75 100644 --- a/ReactCommon/fabric/core/shadownode/ShadowNodeFamily.cpp +++ b/ReactCommon/fabric/core/shadownode/ShadowNodeFamily.cpp @@ -119,6 +119,15 @@ void ShadowNodeFamily::setMostRecentState(State::Shared const &state) const { mostRecentState_ = state; } +std::shared_ptr ShadowNodeFamily::getMostRecentStateIfObsolete( + State const &state) const { + std::unique_lock lock(mutex_); + if (!state.isObsolete_) { + return {}; + } + return mostRecentState_; +} + void ShadowNodeFamily::dispatchRawState( StateUpdate &&stateUpdate, EventPriority priority) const { diff --git a/ReactCommon/fabric/core/shadownode/ShadowNodeFamily.h b/ReactCommon/fabric/core/shadownode/ShadowNodeFamily.h index 15998067f44..1bf431061cb 100644 --- a/ReactCommon/fabric/core/shadownode/ShadowNodeFamily.h +++ b/ReactCommon/fabric/core/shadownode/ShadowNodeFamily.h @@ -88,6 +88,15 @@ class ShadowNodeFamily { private: friend ShadowNode; friend ShadowNodeFamilyFragment; + friend State; + + /* + * Returns the most recent state if the given `state` is obsolete, + * otherwise returns `nullptr`. + * To be used by `State` only. + */ + std::shared_ptr getMostRecentStateIfObsolete( + State const &state) const; EventDispatcher::Weak eventDispatcher_; mutable std::shared_ptr mostRecentState_; diff --git a/ReactCommon/fabric/core/state/State.cpp b/ReactCommon/fabric/core/state/State.cpp index d6fb8afe8e5..d8407cb7e64 100644 --- a/ReactCommon/fabric/core/state/State.cpp +++ b/ReactCommon/fabric/core/state/State.cpp @@ -32,6 +32,15 @@ State::Shared State::getMostRecentState() const { return family->getMostRecentState(); } +State::Shared State::getMostRecentStateIfObsolete() const { + auto family = family_.lock(); + if (!family) { + return {}; + } + + return family->getMostRecentStateIfObsolete(*this); +} + size_t State::getRevision() const { return revision_; } diff --git a/ReactCommon/fabric/core/state/State.h b/ReactCommon/fabric/core/state/State.h index e48cf15c3d4..7e700da7e87 100644 --- a/ReactCommon/fabric/core/state/State.h +++ b/ReactCommon/fabric/core/state/State.h @@ -47,6 +47,12 @@ class State { */ State::Shared getMostRecentState() const; + /* + * Returns the most recent state (same as `getMostRecentState()` method) + * if this state is obsolete, otherwise returns `nullptr`. + */ + State::Shared getMostRecentStateIfObsolete() const; + /* * Returns a revision number of the `State` object. * The number is being automatically assigned during the creation of `State` diff --git a/ReactCommon/fabric/mounting/ShadowTree.cpp b/ReactCommon/fabric/mounting/ShadowTree.cpp index 4099d0f1195..65ba77f6032 100644 --- a/ReactCommon/fabric/mounting/ShadowTree.cpp +++ b/ReactCommon/fabric/mounting/ShadowTree.cpp @@ -22,6 +22,141 @@ namespace facebook { namespace react { +/* + * Generates (possibly) a new tree where all nodes with non-obsolete `State` + * objects. If all `State` objects in the tree are not obsolete for the moment + * of calling, the function returns `nullptr` (as an indication that no + * additional work is required). + */ +static ShadowNode::Unshared progressState(ShadowNode const &shadowNode) { + auto isStateChanged = false; + auto areChildrenChanged = false; + + auto newState = shadowNode.getState(); + if (newState) { + newState = newState->getMostRecentStateIfObsolete(); + if (newState) { + isStateChanged = true; + } + } + + auto newChildren = ShadowNode::ListOfShared{}; + if (shadowNode.getChildren().size() > 0) { + auto index = size_t{0}; + for (auto const &childNode : shadowNode.getChildren()) { + auto newChildNode = progressState(*childNode); + if (newChildNode) { + if (!areChildrenChanged) { + // Making a copy before the first mutation. + newChildren = shadowNode.getChildren(); + } + newChildren[index] = newChildNode; + areChildrenChanged = true; + } + index++; + } + } + + if (!areChildrenChanged && !isStateChanged) { + return nullptr; + } + + return shadowNode.clone({ + ShadowNodeFragment::propsPlaceholder(), + areChildrenChanged ? std::make_shared( + std::move(newChildren)) + : ShadowNodeFragment::childrenPlaceholder(), + isStateChanged ? newState : ShadowNodeFragment::statePlaceholder(), + }); +} + +/* + * An optimized version of the previous function (and relies on it). + * The function uses a given base tree to exclude unchanged (equal) parts + * of the three from the traversing. + */ +static ShadowNode::Unshared progressState( + ShadowNode const &shadowNode, + ShadowNode const &baseShadowNode) { + // The intuition behind the complexity: + // - A very few nodes have associated state, therefore it's mostly reading and + // it only writes when state objects were found obsolete; + // - Most before-after trees are aligned, therefore most tree branches will be + // skipped; + // - If trees are significantly different, any other algorithm will have + // close to linear complexity. + + auto isStateChanged = false; + auto areChildrenChanged = false; + + auto newState = shadowNode.getState(); + if (newState) { + newState = newState->getMostRecentStateIfObsolete(); + if (newState) { + isStateChanged = true; + } + } + + auto &children = shadowNode.getChildren(); + auto &baseChildren = baseShadowNode.getChildren(); + auto newChildren = ShadowNode::ListOfShared{}; + + auto childrenSize = children.size(); + auto baseChildrenSize = baseChildren.size(); + auto index = size_t{0}; + + // Stage 1: Aligned part. + for (index = 0; index < childrenSize && index < baseChildrenSize; index++) { + const auto &childNode = *children.at(index); + const auto &baseChildNode = *baseChildren.at(index); + + if (&childNode == &baseChildNode) { + // Nodes are identical, skipping. + continue; + } + + if (!ShadowNode::sameFamily(childNode, baseChildNode)) { + // Totally different nodes, updating is impossible. + break; + } + + auto newChildNode = progressState(childNode, baseChildNode); + if (newChildNode) { + if (!areChildrenChanged) { + // Making a copy before the first mutation. + newChildren = children; + } + newChildren[index] = newChildNode; + areChildrenChanged = true; + } + } + + // Stage 2: Misaligned part. + for (; index < childrenSize; index++) { + auto newChildNode = progressState(*children.at(index)); + if (newChildNode) { + if (!areChildrenChanged) { + // Making a copy before the first mutation. + newChildren = children; + } + newChildren[index] = newChildNode; + areChildrenChanged = true; + } + } + + if (!areChildrenChanged && !isStateChanged) { + return nullptr; + } + + return shadowNode.clone({ + ShadowNodeFragment::propsPlaceholder(), + areChildrenChanged ? std::make_shared( + std::move(newChildren)) + : ShadowNodeFragment::childrenPlaceholder(), + isStateChanged ? newState : ShadowNodeFragment::statePlaceholder(), + }); +} + static void updateMountedFlag( const SharedShadowNodeList &oldChildren, const SharedShadowNodeList &newChildren) { @@ -161,14 +296,23 @@ bool ShadowTree::tryCommit( return false; } - // Compare state revisions of old and new root - // Children of the root node may be mutated in-place if (enableStateReconciliation) { - UnsharedShadowNode reconciledNode = - reconcileStateWithTree(newRootShadowNode.get(), oldRootShadowNode); - if (reconciledNode != nullptr) { - newRootShadowNode = std::make_shared( - *reconciledNode, ShadowNodeFragment{}); + if (useNewApproachToStateReconciliation_) { + auto updatedNewRootShadowNode = + progressState(*newRootShadowNode, *oldRootShadowNode); + if (updatedNewRootShadowNode) { + newRootShadowNode = + std::static_pointer_cast(updatedNewRootShadowNode); + } + } else { + // Compare state revisions of old and new root + // Children of the root node may be mutated in-place + UnsharedShadowNode reconciledNode = + reconcileStateWithTree(newRootShadowNode.get(), oldRootShadowNode); + if (reconciledNode != nullptr) { + newRootShadowNode = std::make_shared( + *reconciledNode, ShadowNodeFragment{}); + } } } diff --git a/ReactCommon/fabric/mounting/ShadowTree.h b/ReactCommon/fabric/mounting/ShadowTree.h index a2e9aaa1742..eae9749fff6 100644 --- a/ReactCommon/fabric/mounting/ShadowTree.h +++ b/ReactCommon/fabric/mounting/ShadowTree.h @@ -88,6 +88,7 @@ class ShadowTree final { mutable ShadowTreeRevision::Number revisionNumber_{ 0}; // Protected by `commitMutex_`. MountingCoordinator::Shared mountingCoordinator_; + bool useNewApproachToStateReconciliation_{true}; }; } // namespace react