react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Add `cloneMultiple` method to the `ShadowNode` class

Open bartlomiejbloniarz opened this issue 8 months ago • 4 comments

Summary:

This PR adds a new cloning method, allowing for updating multiple nodes in a single transaction. It works in two phases:

  1. Find which nodes have to be cloned (i.e. nodes given on input and all their ancestors)
  2. 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): Screenshot 2025-04-10 at 14 31 14

Adapting this method brought a huge performance gain to reanimated. I want to upstream it, so that:

  1. we can optimize it further, because making it a part of the ShadowNode class gives us access to the parent field in ShadowNodeFamily so we can traverse the tree upwards, allowing for a optimal implementation of the first phase (in reanimated we repeatedly call getAncestors, which revisits some nodes multiple times)
  2. 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.

bartlomiejbloniarz avatar Apr 10 '25 13:04 bartlomiejbloniarz

@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.

bartlomiejbloniarz avatar Apr 11 '25 09:04 bartlomiejbloniarz

@javache What is the state of this? Do you think this approach needs reworking?

bartlomiejbloniarz avatar May 12 '25 13:05 bartlomiejbloniarz

@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.

Yeah, that seems reasonable. Let's keep it backwards compatible though and forward the API.

javache avatar May 23 '25 10:05 javache

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar May 23 '25 10:05 facebook-github-bot

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?

react-native-bot avatar May 28 '25 17:05 react-native-bot

@javache merged this pull request in facebook/react-native@1161fb4fcd6a0cac3a691de1f37cc7f9d6a861a5.

facebook-github-bot avatar May 28 '25 17:05 facebook-github-bot