From 7467b08d9b8c2ac6c41021ed6e31933ef29d01e3 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Tue, 12 Sep 2023 05:39:06 -0700 Subject: [PATCH] Clone free state progression (#39357) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/39357 changelog: [internal] # Problem ## When React clones the wrong node revision Whenever React wants to commit a new change, it first needs to clone shadow nodes. React sometimes clones from the wrong revision. This has mostly been fine, Fabric does state reconciliation to pass newest state forward. State reconciliation is needed, as we need to keep native state in the shadow tree. However, when React clones a node that has never been through layout step, it will clone a node without any layout information and its yoga node is dirtied. Even though there might be a subsequent revision of the node with layout information already calculated. As a result, Yoga needs to traverse bigger parts of the tree, even though layout has been calculated before. It is just cached on a different revision that was used as a source. There are two main sources (there is more but they don't help to paint the picture) when this can happen. Background Executor and State Progression. Let's start with the simpler one but less severe: Background Executor. Background Executor moves layout from JavaScript thread. React can start cloning nodes right away, even though they might not have layout information calculated yet. This is a race condition and depending on when the node is cloned, we can see different results. In this case, React eventually clones node from the correct revision with the layout cache. It will be in a correct state in the end. This case is not as bad as far as I can tell but I included it here because it better illustrates what is going on. State Progression is where things get worse. In this scenario, React will never clone from the correct revision and will never recover from this. Anytime React clones node with a state that needs to be progressed, it will get cloned one more time during commit but React will hold the wrong revision. Depending on where this node is located in the view hierarchy, it may lead to expensive layout calculations. Example: Let's use notation A/r1 as node of family A revision 1. - React calls create node. Node A/r1 is created and React holds reference to this. It will later use it to clone it. Node A has native state that was updated. New revision A/r2 is created. Now React and RN do not observe the same node anymore (this is sometimes necessary). - React now clones node A to create A/r3. This revision may have the wrong yoga cache. Now this might sound like one off but let's explore what happens next. - During commit, Fabric must do state progression to give node A/r3 state from A/r2. This requires cloning and new revision A/r4 is created. React has again a wrong node that does not have Yoga cache and can't recover from this state. The blast radius of this varies depending on where in the tree the node is. # Solution Clone-less state progression. In this algorithm, state progression never clones. It checks if a ShadowNode has ever been mounted via `ShadowNode::hasBeenMounted_` to check if a node can be safely mutated without the need to clone and checks if current state the node is holding is obsolete (like the previous algorithm). The clone-less state progression depends on the fact that any shadow node cloned from React is still unsealed and is not exposed to a multithreaded environment. This makes it safe to mutate it in place, without the need to clone. WARNING: there is important semantic difference with the approach. With the old algorithm, you couldn't go back to a shadow node with old state. New state was always enforced when state reconciliation was enabled. The clone-less algorithm does not support that, because it can't mutate such a node in place. Reviewed By: javache Differential Revision: D49012353 fbshipit-source-id: 1e1d4aea9112b6956fba02d04acfc5170f6134e4 --- .../react/renderer/core/ShadowNode.cpp | 14 ++ .../react/renderer/core/ShadowNode.h | 9 + .../react/renderer/mounting/ShadowTree.cpp | 68 +++++++- .../tests/StateReconciliationTest.cpp | 155 +++++++++++++++--- .../ReactCommon/react/utils/CoreFeatures.cpp | 1 + .../ReactCommon/react/utils/CoreFeatures.h | 3 + 6 files changed, 224 insertions(+), 26 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp b/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp index 737c20ced71b98..5bbdb15b43cd84 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp +++ b/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp @@ -282,6 +282,20 @@ void ShadowNode::setMounted(bool mounted) const { family_->eventEmitter_->setEnabled(mounted); } +void ShadowNode::progressStateIfNecessary() { + if (!hasBeenMounted_ && state_) { + ensureUnsealed(); + auto mostRecentState = family_->getMostRecentStateIfObsolete(*state_); + if (mostRecentState) { + state_ = mostRecentState; + const auto& componentDescriptor = family_->componentDescriptor_; + // Must call ComponentDescriptor::adopt to trigger any side effect + // state may have. E.g. adjusting padding. + componentDescriptor.adopt(*this); + } + } +} + const ShadowNodeFamily& ShadowNode::getFamily() const { return *family_; } diff --git a/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h b/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h index 8203dbc8ba447c..0d6c64fbb1209c 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h +++ b/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h @@ -177,6 +177,15 @@ class ShadowNode : public Sealable, */ void setMounted(bool mounted) const; + /* + * Applies the most recent state to the ShadowNode if following conditions are + * met: + * - ShadowNode has a state. + * - ShadowNode has not been mounted before. + * - ShadowNode's current state is obsolete. + */ + void progressStateIfNecessary(); + #pragma mark - DebugStringConvertible #if RN_DEBUG_STRING_CONVERTIBLE diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/ShadowTree.cpp b/packages/react-native/ReactCommon/react/renderer/mounting/ShadowTree.cpp index 4f189ff675f83a..563f3c38a96efb 100644 --- a/packages/react-native/ReactCommon/react/renderer/mounting/ShadowTree.cpp +++ b/packages/react-native/ReactCommon/react/renderer/mounting/ShadowTree.cpp @@ -25,6 +25,60 @@ namespace facebook::react { using CommitStatus = ShadowTree::CommitStatus; using CommitMode = ShadowTree::CommitMode; +// --- Clone-less progress state algorithm --- +// Note: Ideally, we don't have to const_cast but our use of constness in +// C++ is overly restrictive. We do const_cast here but the only place where +// we change ShadowNode is by calling `ShadowNode::progressStateIfNecessary` +// where checks are in place to avoid manipulating a sealed ShadowNode. + +static void progressStateIfNecessary(ShadowNode& newShadowNode) { + newShadowNode.progressStateIfNecessary(); + + for (const auto& childNode : newShadowNode.getChildren()) { + progressStateIfNecessary(const_cast(*childNode)); + } +} + +static void progressStateIfNecessary( + ShadowNode& newShadowNode, + const ShadowNode& baseShadowNode) { + newShadowNode.progressStateIfNecessary(); + + auto& newChildren = newShadowNode.getChildren(); + auto& baseChildren = baseShadowNode.getChildren(); + + auto newChildrenSize = newChildren.size(); + auto baseChildrenSize = baseChildren.size(); + auto index = size_t{0}; + + for (index = 0; index < newChildrenSize && index < baseChildrenSize; + ++index) { + const auto& newChildNode = *newChildren[index]; + const auto& baseChildNode = *baseChildren[index]; + + if (&newChildNode == &baseChildNode) { + // Nodes are identical. They are shared between `newShadowNode` and + // `baseShadowNode` and it is safe to skipping. + continue; + } + + if (!ShadowNode::sameFamily(newChildNode, baseChildNode)) { + // The nodes are not of the same family. Tree hierarchy has changed + // and we have to fall back to full sub-tree traversal from this point on. + break; + } + + progressStateIfNecessary( + const_cast(newChildNode), baseChildNode); + } + + for (; index < newChildrenSize; ++index) { + const auto& newChildNode = *newChildren[index]; + progressStateIfNecessary(const_cast(newChildNode)); + } +} +// --- End of Clone-less progress state algorithm --- + /* * 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 @@ -344,11 +398,15 @@ CommitStatus ShadowTree::tryCommit( } if (commitOptions.enableStateReconciliation) { - auto updatedNewRootShadowNode = - progressState(*newRootShadowNode, *oldRootShadowNode); - if (updatedNewRootShadowNode) { - newRootShadowNode = - std::static_pointer_cast(updatedNewRootShadowNode); + if (CoreFeatures::enableClonelessStateProgression) { + progressStateIfNecessary(*newRootShadowNode, *oldRootShadowNode); + } else { + auto updatedNewRootShadowNode = + progressState(*newRootShadowNode, *oldRootShadowNode); + if (updatedNewRootShadowNode) { + newRootShadowNode = + std::static_pointer_cast(updatedNewRootShadowNode); + } } } diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp b/packages/react-native/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp index bba5f9a5b533b6..cdcfa42bbc2131 100644 --- a/packages/react-native/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp @@ -62,45 +62,35 @@ const ShadowNode* findDescendantNode( } } // namespace -TEST(StateReconciliationTest, testStateReconciliation) { - auto builder = simpleComponentBuilder(); +class StateReconciliationTest : public ::testing::TestWithParam { + public: + StateReconciliationTest() : builder_(simpleComponentBuilder()) { + CoreFeatures::enableClonelessStateProgression = GetParam(); + } + + ComponentBuilder builder_; +}; +TEST_P(StateReconciliationTest, testStateReconciliation) { auto shadowNodeA = std::shared_ptr{}; auto shadowNodeAA = std::shared_ptr{}; auto shadowNodeAB = std::shared_ptr{}; - auto shadowNodeABA = std::shared_ptr{}; - auto shadowNodeABB = std::shared_ptr{}; - auto shadowNodeABC = std::shared_ptr{}; // clang-format off auto element = Element() .reference(shadowNodeA) - .finalize([](RootShadowNode &shadowNode){ - shadowNode.sealRecursive(); - }) .children({ Element() .reference(shadowNodeAA), Element() .reference(shadowNodeAB) - .children({ - Element() - .children({ - Element() - .reference(shadowNodeABA), - Element() - .reference(shadowNodeABB), - Element() - .reference(shadowNodeABC) - }) - }) }); // clang-format on ContextContainer contextContainer{}; - auto shadowNode = builder.build(element); + auto shadowNode = builder_.build(element); auto rootShadowNodeState1 = shadowNode->ShadowNode::clone({}); @@ -184,5 +174,128 @@ TEST(StateReconciliationTest, testStateReconciliation) { }, {true}); - EXPECT_EQ(findDescendantNode(shadowTree, family)->getState(), state3); + // Warning: + // there is important semantic difference with the approach. With the old + // algorithm, you couldn't go back to a shadow node with old state. New state + // was always enforced when state reconciliation was enabled. The clone-less + // algorithm does not support that, because it can't mutate such a node in + // place. + if (!GetParam()) { + EXPECT_EQ(findDescendantNode(shadowTree, family)->getState(), state3); + } else { + EXPECT_EQ(findDescendantNode(shadowTree, family)->getState(), state2); + } +} + +TEST_P(StateReconciliationTest, testCloneslessStateReconciliationDoesntClone) { + auto shadowNodeA = std::shared_ptr{}; + auto shadowNodeAA = std::shared_ptr{}; + auto shadowNodeAB = std::shared_ptr{}; + + // clang-format off + auto element = + Element() + .reference(shadowNodeA) + .children({ + Element() + .reference(shadowNodeAA), + Element() + .reference(shadowNodeAB) + }); + // clang-format on + + ContextContainer contextContainer{}; + + auto rootShadowNode1 = builder_.build(element); + + auto& scrollViewComponentDescriptor = shadowNodeAB->getComponentDescriptor(); + auto& family = shadowNodeAB->getFamily(); + auto state1 = shadowNodeAB->getState(); + auto shadowTreeDelegate = DummyShadowTreeDelegate{}; + ShadowTree shadowTree{ + SurfaceId{11}, + LayoutConstraints{}, + LayoutContext{}, + shadowTreeDelegate, + contextContainer}; + + shadowTree.commit( + [&](const RootShadowNode& /*oldRootShadowNode*/) { + return std::static_pointer_cast(rootShadowNode1); + }, + {true}); + + EXPECT_EQ(state1->getMostRecentState(), state1); + + EXPECT_EQ(findDescendantNode(*rootShadowNode1, family)->getState(), state1); + + auto state2 = scrollViewComponentDescriptor.createState( + family, std::make_shared()); + + auto rootShadowNode2 = + rootShadowNode1->cloneTree(family, [&](const ShadowNode& oldShadowNode) { + return oldShadowNode.clone( + {ShadowNodeFragment::propsPlaceholder(), + ShadowNodeFragment::childrenPlaceholder(), + state2}); + }); + + EXPECT_EQ(findDescendantNode(*rootShadowNode2, family)->getState(), state2); + EXPECT_EQ(state1->getMostRecentState(), state1); + + shadowTree.commit( + [&](const RootShadowNode& /*oldRootShadowNode*/) { + return std::static_pointer_cast(rootShadowNode2); + }, + {true}); + + EXPECT_EQ(state1->getMostRecentState(), state2); + EXPECT_EQ(state2->getMostRecentState(), state2); + + ShadowNode::Unshared newlyClonedShadowNode; + + auto rootShadowNodeClonedFromReact = + rootShadowNode2->cloneTree(family, [&](const ShadowNode& oldShadowNode) { + newlyClonedShadowNode = oldShadowNode.clone({}); + return newlyClonedShadowNode; + }); + + auto state3 = scrollViewComponentDescriptor.createState( + family, std::make_shared()); + + auto rootShadowNodeClonedFromStateUpdate = + rootShadowNode2->cloneTree(family, [&](const ShadowNode& oldShadowNode) { + return oldShadowNode.clone( + {ShadowNodeFragment::propsPlaceholder(), + ShadowNodeFragment::childrenPlaceholder(), + state3}); + }); + + shadowTree.commit( + [&](const RootShadowNode& /*oldRootShadowNode*/) { + return std::static_pointer_cast( + rootShadowNodeClonedFromStateUpdate); + }, + {}); + + shadowTree.commit( + [&](const RootShadowNode& /*oldRootShadowNode*/) { + return std::static_pointer_cast( + rootShadowNodeClonedFromReact); + }, + {true}); + + auto scrollViewShadowNode = findDescendantNode(shadowTree, family); + + EXPECT_EQ(scrollViewShadowNode->getState(), state3); + + if (GetParam()) { + // Checking that newlyClonedShadowNode was not cloned unnecessarly by state + // progression. This fails with the old algorithm. + EXPECT_EQ(scrollViewShadowNode, newlyClonedShadowNode.get()); + } } +INSTANTIATE_TEST_SUITE_P( + StateReconciliationTestInstantiation, + StateReconciliationTest, + testing::Values(false, true)); diff --git a/packages/react-native/ReactCommon/react/utils/CoreFeatures.cpp b/packages/react-native/ReactCommon/react/utils/CoreFeatures.cpp index 88def7c490d0d4..c428d1e665dab8 100644 --- a/packages/react-native/ReactCommon/react/utils/CoreFeatures.cpp +++ b/packages/react-native/ReactCommon/react/utils/CoreFeatures.cpp @@ -22,5 +22,6 @@ bool CoreFeatures::enableCleanParagraphYogaNode = false; bool CoreFeatures::disableScrollEventThrottleRequirement = false; bool CoreFeatures::enableGranularShadowTreeStateReconciliation = false; bool CoreFeatures::enableDefaultAsyncBatchedPriority = false; +bool CoreFeatures::enableClonelessStateProgression = false; } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/utils/CoreFeatures.h b/packages/react-native/ReactCommon/react/utils/CoreFeatures.h index 8b3a53bc353527..919748cfd34be7 100644 --- a/packages/react-native/ReactCommon/react/utils/CoreFeatures.h +++ b/packages/react-native/ReactCommon/react/utils/CoreFeatures.h @@ -67,6 +67,9 @@ class CoreFeatures { // Default state updates and events to async batched priority. static bool enableDefaultAsyncBatchedPriority; + + // When enabled, Fabric will avoid cloning notes to perform state progression. + static bool enableClonelessStateProgression; }; } // namespace facebook::react