Skip to content

Add cloneMultiple method to the ShadowNode class #50624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

bartlomiejbloniarz
Copy link
Contributor

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.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Software Mansion Partner: Software Mansion Partner labels Apr 10, 2025
@bartlomiejbloniarz bartlomiejbloniarz changed the title Add cloneMultiple Add cloneMultiple method to the ShadowNode class Apr 10, 2025
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Apr 10, 2025
const ShadowNode& oldShadowNode,
const std::optional<ShadowNode::ListOfShared>& newChildren)>& callback)
const {
std::unordered_map<const ShadowNodeFamily*, int> childrenCount;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::unordered_map<const ShadowNodeFamily*, int> childrenCount;
std::unordered_map<const std::ref<const ShadowNodeFamily>, unsigned int> childrenCounts;

@@ -252,6 +266,15 @@ class ShadowNode : public Sealable,
const ShadowNode& sourceShadowNode,
const Props::Shared& props);

Unshared cloneMultipleRecursive(
const ShadowNode& shadowNode,
const std::unordered_set<const ShadowNodeFamily*>& families,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's pass ShadowNodeFamily as a std::ref instead of a pointer maybe?

Suggested change
const std::unordered_set<const ShadowNodeFamily*>& families,
const std::unordered_set<const ShadowNodeFamily &>& families,

const std::unordered_set<const ShadowNodeFamily*>& families,
const std::function<Unshared(
const ShadowNode& oldShadowNode,
const std::optional<ShadowNode::ListOfShared>& newChildren)>&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also explain why newChildren is optional.

*/
Unshared cloneMultiple(
const std::unordered_set<const ShadowNodeFamily*>& families,
const std::function<Unshared(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also mention what happens if callback returns nullptr?

}

if (childrenCount.empty()) {
return ShadowNode::Unshared{nullptr};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to be that explicit about the return type?

Suggested change
return ShadowNode::Unshared{nullptr};
return nullptr;

@javache javache requested a review from zeyap April 11, 2025 08:41
Copy link
Member

@javache javache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these methods don't actually need to live on ShadowNode, what about creating a ShadowNodeMutations helper class (and defining it as a friend class if necessary)

Comment on lines 373 to 374
const std::unordered_set<const ShadowNodeFamily*>& families,
const std::unordered_map<const ShadowNodeFamily*, int>& childrenCount,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using pointers to ShadowNodeFamily, let's try to use refs, eg using std::reference_wrapper

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that. The problem with it is that I use unordered containers, so that would require implementing hash methods for the family.

For now I changed the loop over ancestors, to lock the weak pointer, so that when we access any fields of a family, we are sure it's still around. After that we only compare these pointers as numbers - we never dereference them so it will be safe. This would only fail if someone passed familiesToUpdate to the function, without owning them. In reanimated we keep shared_ptrs of ShadowNodes related to these families to ensure that they are still available when we perform the algorithm.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change the api to instead work with shared_ptrs. The problem is that users of this api (so for example reanimated), don't have access to shared_ptrs of the ShadowNodeFamily class, as you only can get a reference from the ShadowNode.getFamily() method.

const std::optional<ShadowNode::ListOfShared>& newChildren)>& callback)
const {
const auto family = &shadowNode.getFamily();
auto children = shadowNode.getChildren();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid copying the list

Suggested change
auto children = shadowNode.getChildren();
const auto& children = shadowNode.getChildren();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am copying that on purpose, as I want to modify it later on in the loop.

@@ -368,6 +368,80 @@ ShadowNode::Unshared ShadowNode::cloneTree(
return std::const_pointer_cast<ShadowNode>(childNode);
}

ShadowNode::Unshared ShadowNode::cloneMultipleRecursive(
const ShadowNode& shadowNode,
const std::unordered_set<const ShadowNodeFamily*>& families,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this familiesToUpdate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will rename

const std::unordered_set<const ShadowNodeFamily*>& families,
const std::function<Unshared(
const ShadowNode& oldShadowNode,
const std::optional<ShadowNode::ListOfShared>& newChildren)>& callback)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass in a ShadowNodeFragment instead of children explicitly

@bartlomiejbloniarz
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Software Mansion Partner: Software Mansion Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants