Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,80 @@ ShadowNode::Unshared ShadowNode::cloneTree(
return std::const_pointer_cast<ShadowNode>(childNode);
}

ShadowNode::Unshared ShadowNode::cloneMultipleRecursive(
const ShadowNode& shadowNode,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this just be a instance method? Alternatively make it static if it doesn't depend on instance state.

const std::unordered_set<const ShadowNodeFamily*>& families,
Copy link
Copy Markdown
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
Copy Markdown
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_map<const ShadowNodeFamily*, int>& childrenCount,
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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::function<Unshared(
const ShadowNode& oldShadowNode,
const std::optional<ShadowNode::ListOfShared>& newChildren)>& callback)
const {
const auto family = &shadowNode.getFamily();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use const auto * to make it explicit this isn't a copy. Even better would be to just use the ref here, and only deref it when looking it up.

auto children = shadowNode.getChildren();
Copy link
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could also model this as a std::optional and only do the copy when you first need to. That also avoids the need for shouldUpdateChildren.

auto count = childrenCount.at(family);
auto shouldUpdateChildren = false;
std::optional<ShadowNode::ListOfShared> newChildren;

for (int i = 0; count > 0 && i < children.size(); i++) {
const auto childFamily = &children[i]->getFamily();
if (childrenCount.contains(childFamily)) {
count--;
shouldUpdateChildren = true;
children[i] = cloneMultipleRecursive(
*children[i], families, childrenCount, callback);
}
}

if (shouldUpdateChildren) {
newChildren = std::move(children);
}

if (families.contains(family)) {
return callback(shadowNode, newChildren);
}

return shadowNode.clone(
{ShadowNodeFragment::propsPlaceholder(),
newChildren
? std::make_shared<ShadowNode::ListOfShared>(std::move(*newChildren))
: ShadowNodeFragment::childrenPlaceholder()});
}

ShadowNode::Unshared ShadowNode::cloneMultiple(
const std::unordered_set<const ShadowNodeFamily*>& families,
const std::function<Unshared(
const ShadowNode& oldShadowNode,
const std::optional<ShadowNode::ListOfShared>& newChildren)>& callback)
Copy link
Copy Markdown
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

const {
std::unordered_map<const ShadowNodeFamily*, int> childrenCount;
Copy link
Copy Markdown
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;


for (const auto& family : families) {
if (childrenCount.contains(family)) {
continue;
}

childrenCount[family] = 0;

auto ancestor = family;
while ((ancestor != nullptr) && ancestor != this->family_.get()) {
ancestor = ancestor->parent_.lock().get();
auto ancestorIt = childrenCount.find(ancestor);
if (ancestorIt != childrenCount.end()) {
ancestorIt->second++;
break;
}

childrenCount[ancestor] = 1;
}
}

if (childrenCount.empty()) {
return ShadowNode::Unshared{nullptr};
Copy link
Copy Markdown
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;

}

return cloneMultipleRecursive(*this, families, childrenCount, callback);
}

#pragma mark - DebugStringConvertible

#if RN_DEBUG_STRING_CONVERTIBLE
Expand Down
23 changes: 23 additions & 0 deletions packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,20 @@ class ShadowNode : public Sealable,
const std::function<Unshared(const ShadowNode& oldShadowNode)>& callback)
const;

/*
* Clones the nodes (and the subtree containing all the nodes) by
* replacing the `oldShadowNode` for every `shadowNodeFamily` from `families`
* with a node that `callback` returns.
*
* Returns `nullptr` if the operation cannot be performed successfully.
*/
Unshared cloneMultiple(
const std::unordered_set<const ShadowNodeFamily*>& families,
const std::function<Unshared(
Copy link
Copy Markdown
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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The implementation is not really well defined if you call this method on anything that's not the RootShadowNode. Should this live on the root instead?

const ShadowNode& oldShadowNode,
const std::optional<ShadowNode::ListOfShared>& newChildren)>&
Copy link
Copy Markdown
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.

callback) const;

#pragma mark - Getters

ComponentName getComponentName() const;
Expand Down Expand Up @@ -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
Copy Markdown
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_map<const ShadowNodeFamily*, int>& childrenCount,
const std::function<Unshared(
const ShadowNode& oldShadowNode,
const std::optional<ShadowNode::ListOfShared>& newChildren)>&
callback) const;

protected:
/*
* Traits associated with the particular `ShadowNode` class and an instance of
Expand Down
Loading