Skip to content

Commit c3c01bd

Browse files
sammy-SCfacebook-github-bot
authored andcommitted
Clone free state progression (facebook#39357)
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: b2aa3dd7bece4ace2395ba1978eb77878489ff70
1 parent c972ad4 commit c3c01bd

File tree

6 files changed

+201
-5
lines changed

6 files changed

+201
-5
lines changed

packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,20 @@ void ShadowNode::setMounted(bool mounted) const {
282282
family_->eventEmitter_->setEnabled(mounted);
283283
}
284284

285+
void ShadowNode::progressStateIfNecessary() {
286+
if (!hasBeenMounted_ && state_) {
287+
ensureUnsealed();
288+
auto mostRecentState = family_->getMostRecentStateIfObsolete(*state_);
289+
if (mostRecentState) {
290+
state_ = mostRecentState;
291+
const auto& componentDescriptor = family_->componentDescriptor_;
292+
// Must call ComponentDescriptor::adopt to trigger any side effect
293+
// state may have. E.g. adjusting padding.
294+
componentDescriptor.adopt(*this);
295+
}
296+
}
297+
}
298+
285299
const ShadowNodeFamily& ShadowNode::getFamily() const {
286300
return *family_;
287301
}

packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,15 @@ class ShadowNode : public Sealable,
177177
*/
178178
void setMounted(bool mounted) const;
179179

180+
/*
181+
* Applies the most recent state to the ShadowNode if following conditions are
182+
* met:
183+
* - ShadowNode has a state.
184+
* - ShadowNode has not been mounted before.
185+
* - ShadowNode's current state is obsolete.
186+
*/
187+
void progressStateIfNecessary();
188+
180189
#pragma mark - DebugStringConvertible
181190

182191
#if RN_DEBUG_STRING_CONVERTIBLE

packages/react-native/ReactCommon/react/renderer/mounting/ShadowTree.cpp

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,60 @@ namespace facebook::react {
2525
using CommitStatus = ShadowTree::CommitStatus;
2626
using CommitMode = ShadowTree::CommitMode;
2727

28+
// --- Clone-less progress state algorithm ---
29+
// Note: Ideally, we don't have to const_cast but our use of constness in
30+
// C++ is overly restrictive. We do const_cast here but the only place where
31+
// we change ShadowNode is by calling `ShadowNode::progressStateIfNecessary`
32+
// where checks are in place to avoid manipulating a sealed ShadowNode.
33+
34+
static void progressStateIfNecessary(ShadowNode& newShadowNode) {
35+
newShadowNode.progressStateIfNecessary();
36+
37+
for (const auto& childNode : newShadowNode.getChildren()) {
38+
progressStateIfNecessary(const_cast<ShadowNode&>(*childNode));
39+
}
40+
}
41+
42+
static void progressStateIfNecessary(
43+
ShadowNode& newShadowNode,
44+
const ShadowNode& baseShadowNode) {
45+
newShadowNode.progressStateIfNecessary();
46+
47+
auto& newChildren = newShadowNode.getChildren();
48+
auto& baseChildren = baseShadowNode.getChildren();
49+
50+
auto newChildrenSize = newChildren.size();
51+
auto baseChildrenSize = baseChildren.size();
52+
auto index = size_t{0};
53+
54+
for (index = 0; index < newChildrenSize && index < baseChildrenSize;
55+
++index) {
56+
const auto& newChildNode = *newChildren[index];
57+
const auto& baseChildNode = *baseChildren[index];
58+
59+
if (&newChildNode == &baseChildNode) {
60+
// Nodes are identical. They are shared between `newShadowNode` and
61+
// `baseShadowNode` and it is safe to skipping.
62+
continue;
63+
}
64+
65+
if (!ShadowNode::sameFamily(newChildNode, baseChildNode)) {
66+
// The nodes are not of the same family. Tree hierarchy has changed
67+
// and we have to fall back to full sub-tree traversal from this point on.
68+
break;
69+
}
70+
71+
progressStateIfNecessary(
72+
const_cast<ShadowNode&>(newChildNode), baseChildNode);
73+
}
74+
75+
for (; index < newChildrenSize; ++index) {
76+
const auto& newChildNode = *newChildren[index];
77+
progressStateIfNecessary(const_cast<ShadowNode&>(newChildNode));
78+
}
79+
}
80+
// --- End of Clone-less progress state algorithm ---
81+
2882
/*
2983
* Generates (possibly) a new tree where all nodes with non-obsolete `State`
3084
* objects. If all `State` objects in the tree are not obsolete for the moment
@@ -344,11 +398,15 @@ CommitStatus ShadowTree::tryCommit(
344398
}
345399

346400
if (commitOptions.enableStateReconciliation) {
347-
auto updatedNewRootShadowNode =
348-
progressState(*newRootShadowNode, *oldRootShadowNode);
349-
if (updatedNewRootShadowNode) {
350-
newRootShadowNode =
351-
std::static_pointer_cast<RootShadowNode>(updatedNewRootShadowNode);
401+
if (CoreFeatures::enableClonelessStateProgression) {
402+
progressStateIfNecessary(*newRootShadowNode, *oldRootShadowNode);
403+
} else {
404+
auto updatedNewRootShadowNode =
405+
progressState(*newRootShadowNode, *oldRootShadowNode);
406+
if (updatedNewRootShadowNode) {
407+
newRootShadowNode =
408+
std::static_pointer_cast<RootShadowNode>(updatedNewRootShadowNode);
409+
}
352410
}
353411
}
354412

packages/react-native/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,3 +184,114 @@ TEST(StateReconciliationTest, testStateReconciliation) {
184184

185185
EXPECT_EQ(findDescendantNode(shadowTree, family)->getState(), state3);
186186
}
187+
188+
TEST(StateReconciliationTest, testCloneslessStateReconciliationDoesntClone) {
189+
CoreFeatures::enableClonelessStateProgression = true;
190+
auto builder = simpleComponentBuilder();
191+
192+
auto shadowNodeA = std::shared_ptr<RootShadowNode>{};
193+
auto shadowNodeAA = std::shared_ptr<ViewShadowNode>{};
194+
auto shadowNodeAB = std::shared_ptr<ScrollViewShadowNode>{};
195+
196+
// clang-format off
197+
auto element =
198+
Element<RootShadowNode>()
199+
.reference(shadowNodeA)
200+
.children({
201+
Element<ViewShadowNode>()
202+
.reference(shadowNodeAA),
203+
Element<ScrollViewShadowNode>()
204+
.reference(shadowNodeAB)
205+
});
206+
// clang-format on
207+
208+
ContextContainer contextContainer{};
209+
210+
auto initialRootShadowNode = builder.build(element);
211+
212+
auto rootShadowNode1 = initialRootShadowNode->ShadowNode::clone({});
213+
214+
auto& scrollViewComponentDescriptor = shadowNodeAB->getComponentDescriptor();
215+
auto& family = shadowNodeAB->getFamily();
216+
auto state1 = shadowNodeAB->getState();
217+
auto shadowTreeDelegate = DummyShadowTreeDelegate{};
218+
ShadowTree shadowTree{
219+
SurfaceId{11},
220+
LayoutConstraints{},
221+
LayoutContext{},
222+
shadowTreeDelegate,
223+
contextContainer};
224+
225+
shadowTree.commit(
226+
[&](const RootShadowNode& /*oldRootShadowNode*/) {
227+
return std::static_pointer_cast<RootShadowNode>(rootShadowNode1);
228+
},
229+
{true});
230+
231+
EXPECT_EQ(state1->getMostRecentState(), state1);
232+
233+
EXPECT_EQ(findDescendantNode(*rootShadowNode1, family)->getState(), state1);
234+
235+
auto state2 = scrollViewComponentDescriptor.createState(
236+
family, std::make_shared<const ScrollViewState>());
237+
238+
auto rootShadowNode2 =
239+
rootShadowNode1->cloneTree(family, [&](const ShadowNode& oldShadowNode) {
240+
return oldShadowNode.clone(
241+
{ShadowNodeFragment::propsPlaceholder(),
242+
ShadowNodeFragment::childrenPlaceholder(),
243+
state2});
244+
});
245+
246+
EXPECT_EQ(findDescendantNode(*rootShadowNode2, family)->getState(), state2);
247+
EXPECT_EQ(state1->getMostRecentState(), state1);
248+
249+
shadowTree.commit(
250+
[&](const RootShadowNode& /*oldRootShadowNode*/) {
251+
return std::static_pointer_cast<RootShadowNode>(rootShadowNode2);
252+
},
253+
{true});
254+
255+
EXPECT_EQ(state1->getMostRecentState(), state2);
256+
EXPECT_EQ(state2->getMostRecentState(), state2);
257+
258+
ShadowNode::Unshared newlyClonedShadowNode;
259+
260+
auto rootShadowNodeClonedFromReact =
261+
rootShadowNode2->cloneTree(family, [&](const ShadowNode& oldShadowNode) {
262+
newlyClonedShadowNode = oldShadowNode.clone({});
263+
return newlyClonedShadowNode;
264+
});
265+
266+
auto state3 = scrollViewComponentDescriptor.createState(
267+
family, std::make_shared<const ScrollViewState>());
268+
269+
auto rootShadowNodeClonedFromStateUpdate =
270+
rootShadowNode2->cloneTree(family, [&](const ShadowNode& oldShadowNode) {
271+
return oldShadowNode.clone(
272+
{ShadowNodeFragment::propsPlaceholder(),
273+
ShadowNodeFragment::childrenPlaceholder(),
274+
state3});
275+
});
276+
277+
shadowTree.commit(
278+
[&](const RootShadowNode& /*oldRootShadowNode*/) {
279+
return std::static_pointer_cast<RootShadowNode>(
280+
rootShadowNodeClonedFromStateUpdate);
281+
},
282+
{});
283+
284+
shadowTree.commit(
285+
[&](const RootShadowNode& /*oldRootShadowNode*/) {
286+
return std::static_pointer_cast<RootShadowNode>(
287+
rootShadowNodeClonedFromReact);
288+
},
289+
{true});
290+
291+
auto scrollViewShadowNode = findDescendantNode(shadowTree, family);
292+
293+
EXPECT_EQ(scrollViewShadowNode->getState(), state3);
294+
// Checking that newlyClonedShadowNode was not cloned unnecessarly by state
295+
// progression. This fails with the old algorithm.
296+
EXPECT_EQ(scrollViewShadowNode, newlyClonedShadowNode.get());
297+
}

packages/react-native/ReactCommon/react/utils/CoreFeatures.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,5 +22,6 @@ bool CoreFeatures::enableCleanParagraphYogaNode = false;
2222
bool CoreFeatures::disableScrollEventThrottleRequirement = false;
2323
bool CoreFeatures::enableGranularShadowTreeStateReconciliation = false;
2424
bool CoreFeatures::enableDefaultAsyncBatchedPriority = false;
25+
bool CoreFeatures::enableClonelessStateProgression = false;
2526

2627
} // namespace facebook::react

packages/react-native/ReactCommon/react/utils/CoreFeatures.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ class CoreFeatures {
6767

6868
// Default state updates and events to async batched priority.
6969
static bool enableDefaultAsyncBatchedPriority;
70+
71+
// When enabled, Fabric will avoid cloning notes to perform state progression.
72+
static bool enableClonelessStateProgression;
7073
};
7174

7275
} // namespace facebook::react

0 commit comments

Comments
 (0)