Add `cloneMultiple` method to the `ShadowNode` class
Summary:
This PR adds a new cloning method, allowing for updating multiple nodes in a single transaction. It works in two phases:
- Find which nodes have to be cloned (i.e. nodes given on input and all their ancestors)
- Clone nodes in the bottom up order - so that every node is cloned exactly once
So the idea is that when we want to update all the red nodes in this picture, we first find the nodes in the green area and the clone only them in the correct order (children are cloned before parents):
Adapting this method brought a huge performance gain to reanimated. I want to upstream it, so that:
- we can optimize it further, because making it a part of the
ShadowNodeclass gives us access to the parent field inShadowNodeFamilyso we can traverse the tree upwards, allowing for a optimal implementation of the first phase (in reanimated we repeatedly callgetAncestors, which revisits some nodes multiple times) - the community can use it
A naive approach that calls cloneTree for every node is much slower, as it has to repeat many operations.
Changelog:
[GENERAL] [ADDED] - Added cloneMultiple to ShadowNode class.
Test Plan:
I tested it with the following reanimated implementation and everything works fine:
const auto callback =
[&](const ShadowNode &shadowNode,
const std::optional<ShadowNode::ListOfShared> &newChildren) {
return shadowNode.clone(
{mergeProps(shadowNode, propsMap, shadowNode.getFamily()),
newChildren
? std::make_shared<ShadowNode::ListOfShared>(*newChildren)
: ShadowNodeFragment::childrenPlaceholder(),
shadowNode.getState()});
};
return std::static_pointer_cast<RootShadowNode>(
oldRootNode.cloneMultiple(families, callback));
I would like to add tests for it, but I'm not sure what's the best approach for that in the repo.
@javache Sure I can extract it to a separate file. Should I also move the cloneTree function? I wanted the cloneMultiple function to be a generalization of that.
@javache What is the state of this? Do you think this approach needs reworking?
@javache Sure I can extract it to a separate file. Should I also move the
cloneTreefunction? I wanted thecloneMultiplefunction to be a generalization of that.
Yeah, that seems reasonable. Let's keep it backwards compatible though and forward the API.
@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
This pull request was successfully merged by @bartlomiejbloniarz in 1161fb4fcd6a0cac3a691de1f37cc7f9d6a861a5
When will my fix make it into a release? | How to file a pick request?
@javache merged this pull request in facebook/react-native@1161fb4fcd6a0cac3a691de1f37cc7f9d6a861a5.