Skip to content

Commit

Permalink
Clone free state progression (facebook#39357)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: facebook#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: d183db1b9b980d469c4ec575b1dc5a4c3884c705
  • Loading branch information
sammy-SC authored and facebook-github-bot committed Sep 11, 2023
1 parent aee306c commit 3d82698
Show file tree
Hide file tree
Showing 6 changed files with 202 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,18 @@ 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_;
componentDescriptor.adopt(*this);
}
}
}

const ShadowNodeFamily& ShadowNode::getFamily() const {
return *family_;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<ShadowNode&>(*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<ShadowNode&>(newChildNode), baseChildNode);
}

for (; index < newChildrenSize; ++index) {
const auto& newChildNode = *newChildren[index];
progressStateIfNecessary(const_cast<ShadowNode&>(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
Expand Down Expand Up @@ -344,11 +398,15 @@ CommitStatus ShadowTree::tryCommit(
}

if (commitOptions.enableStateReconciliation) {
auto updatedNewRootShadowNode =
progressState(*newRootShadowNode, *oldRootShadowNode);
if (updatedNewRootShadowNode) {
newRootShadowNode =
std::static_pointer_cast<RootShadowNode>(updatedNewRootShadowNode);
if (CoreFeatures::enableClonelessStateProgression) {
progressStateIfNecessary(*newRootShadowNode, *oldRootShadowNode);
} else {
auto updatedNewRootShadowNode =
progressState(*newRootShadowNode, *oldRootShadowNode);
if (updatedNewRootShadowNode) {
newRootShadowNode =
std::static_pointer_cast<RootShadowNode>(updatedNewRootShadowNode);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,3 +184,117 @@ TEST(StateReconciliationTest, testStateReconciliation) {

EXPECT_EQ(findDescendantNode(shadowTree, family)->getState(), state3);
}

TEST(StateReconciliationTest, testCloneslessStateReconciliationDoesntClone) {
CoreFeatures::enableClonelessStateProgression = true;
auto builder = simpleComponentBuilder();

auto shadowNodeA = std::shared_ptr<RootShadowNode>{};
auto shadowNodeAA = std::shared_ptr<ViewShadowNode>{};
auto shadowNodeAB = std::shared_ptr<ScrollViewShadowNode>{};

// clang-format off
auto element =
Element<RootShadowNode>()
.reference(shadowNodeA)
.children({
Element<ViewShadowNode>()
.reference(shadowNodeAA),
Element<ScrollViewShadowNode>()
.reference(shadowNodeAB)
});
// clang-format on

ContextContainer contextContainer{};

auto initialRootShadowNode = builder.build(element);

auto rootShadowNode1 = initialRootShadowNode->ShadowNode::clone({});

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<RootShadowNode>(rootShadowNode1);
},
{true});

EXPECT_EQ(state1->getMostRecentState(), state1);

EXPECT_EQ(findDescendantNode(*rootShadowNode1, family)->getState(), state1);

auto state2 = scrollViewComponentDescriptor.createState(
family, std::make_shared<const ScrollViewState>());

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<RootShadowNode>(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(
{ShadowNodeFragment::propsPlaceholder(),
ShadowNodeFragment::childrenPlaceholder(),
ShadowNodeFragment::statePlaceholder()});
return newlyClonedShadowNode;
});

auto state3 = scrollViewComponentDescriptor.createState(
family, std::make_shared<const ScrollViewState>());

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<RootShadowNode>(
rootShadowNodeClonedFromStateUpdate);
},
{});

shadowTree.commit(
[&](const RootShadowNode& /*oldRootShadowNode*/) {
return std::static_pointer_cast<RootShadowNode>(
rootShadowNodeClonedFromReact);
},
{true});

auto scrollViewShadowNode = findDescendantNode(shadowTree, family);

EXPECT_EQ(scrollViewShadowNode->getState(), state3);
// Checking that newlyClonedShadowNode was not cloned unnecessarly by state
// progression. This fails with the old algorithm.
EXPECT_EQ(scrollViewShadowNode, newlyClonedShadowNode.get());
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 3 additions & 0 deletions packages/react-native/ReactCommon/react/utils/CoreFeatures.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 3d82698

Please sign in to comment.