From 57fce665fbf61b561329bc5d4030abeb2f5cdc05 Mon Sep 17 00:00:00 2001 From: David Aurelio Date: Wed, 20 Feb 2019 11:52:38 -0800 Subject: [PATCH] Clone children only during layout, allow mixing shared + owned children Summary: @public Limit child cloning to layout calculation. This also allows for mixing shared and owned children. Rationale: We do allow for shared children if the caller manages themselves. The single known use case is React Native. So far, we have cloned children eagerly whenever child lists are mutated, or layout is run. This was to allow for a quick check of the owner of any first child, assuming that either *all* or *no* child of a node are shared. For Yoga/Java, we want to get rid of global weak JNI refs, and these are also used to invoke clone callbacks. We can achieve that goal by switching to an alternative approach, passing additional data to the layout pass. This additional data has to be passed to any configured cloning callback. Therefore, it is desirable to **only call cloning functions during the layout pass.** The obvious solution seems to be to not uphold the invariant of the first child determining shared/owned state of all siblings, and allow for a mix of shared and own children. Reviewed By: shergin Differential Revision: D14136223 fbshipit-source-id: 34490cfeeb2170c99d6ed1b9bdcbcedb316813af --- ReactCommon/yoga/yoga/YGNode.cpp | 25 +--------- ReactCommon/yoga/yoga/YGNode.h | 16 ++++++ ReactCommon/yoga/yoga/Yoga.cpp | 84 ++++++++++---------------------- 3 files changed, 43 insertions(+), 82 deletions(-) diff --git a/ReactCommon/yoga/yoga/YGNode.cpp b/ReactCommon/yoga/yoga/YGNode.cpp index 4fcef90ea..fedb3b9e5 100644 --- a/ReactCommon/yoga/yoga/YGNode.cpp +++ b/ReactCommon/yoga/yoga/YGNode.cpp @@ -386,30 +386,7 @@ void YGNode::clearChildren() { // Other Methods void YGNode::cloneChildrenIfNeeded() { - // YGNodeRemoveChild in yoga.cpp has a forked variant of this algorithm - // optimized for deletions. - - const uint32_t childCount = static_cast(children_.size()); - if (childCount == 0) { - // This is an empty set. Nothing to clone. - return; - } - - const YGNodeRef firstChild = children_.front(); - if (firstChild->getOwner() == this) { - // If the first child has this node as its owner, we assume that it is - // already unique. We can do this because if we have it has a child, that - // means that its owner was at some point cloned which made that subtree - // immutable. We also assume that all its sibling are cloned as well. - return; - } - - for (uint32_t i = 0; i < childCount; ++i) { - const YGNodeRef oldChild = children_[i]; - YGNodeRef newChild = config_->cloneNode(oldChild, this, i); - replaceChild(newChild, i); - newChild->setOwner(this); - } + iterChildrenAfterCloningIfNeeded([](YGNodeRef) {}); } void YGNode::markDirtyAndPropogate() { diff --git a/ReactCommon/yoga/yoga/YGNode.h b/ReactCommon/yoga/yoga/YGNode.h index 63087fe46..3c2a99031 100644 --- a/ReactCommon/yoga/yoga/YGNode.h +++ b/ReactCommon/yoga/yoga/YGNode.h @@ -143,6 +143,22 @@ public: return children_; } + // Applies a callback to all children, after cloning them if they are not + // owned. + template + void iterChildrenAfterCloningIfNeeded(T callback) { + int i = 0; + for (YGNodeRef& child : children_) { + if (child->getOwner() != this) { + child = config_->cloneNode(child, this, i); + child->setOwner(this); + } + i += 1; + + callback(child); + } + } + YGNodeRef getChild(uint32_t index) const { return children_.at(index); } diff --git a/ReactCommon/yoga/yoga/Yoga.cpp b/ReactCommon/yoga/yoga/Yoga.cpp index 290e50a8c..22c2a0388 100644 --- a/ReactCommon/yoga/yoga/Yoga.cpp +++ b/ReactCommon/yoga/yoga/Yoga.cpp @@ -302,14 +302,16 @@ static void YGConfigFreeRecursive(const YGNodeRef root) { void YGNodeFreeRecursiveWithCleanupFunc( const YGNodeRef root, YGNodeCleanupFunc cleanup) { - while (YGNodeGetChildCount(root) > 0) { - const YGNodeRef child = YGNodeGetChild(root, 0); + uint32_t skipped = 0; + while (YGNodeGetChildCount(root) > skipped) { + const YGNodeRef child = YGNodeGetChild(root, skipped); if (child->getOwner() != root) { // Don't free shared nodes that we don't own. - break; + skipped += 1; + } else { + YGNodeRemoveChild(root, child); + YGNodeFreeRecursive(child); } - YGNodeRemoveChild(root, child); - YGNodeFreeRecursive(child); } if (cleanup != nullptr) { cleanup(root); @@ -381,70 +383,40 @@ bool YGNodeIsReferenceBaseline(YGNodeRef node) { } void YGNodeInsertChild( - const YGNodeRef node, + const YGNodeRef owner, const YGNodeRef child, const uint32_t index) { YGAssertWithNode( - node, + owner, child->getOwner() == nullptr, "Child already has a owner, it must be removed first."); YGAssertWithNode( - node, - !node->hasMeasureFunc(), + owner, + !owner->hasMeasureFunc(), "Cannot add child: Nodes with measure functions cannot have children."); - node->cloneChildrenIfNeeded(); - node->insertChild(child, index); - YGNodeRef owner = child->getOwner() ? nullptr : node; + owner->insertChild(child, index); child->setOwner(owner); - node->markDirtyAndPropogate(); + owner->markDirtyAndPropogate(); } void YGNodeRemoveChild(const YGNodeRef owner, const YGNodeRef excludedChild) { - // This algorithm is a forked variant from cloneChildrenIfNeeded in YGNode - // that excludes a child. - const uint32_t childCount = YGNodeGetChildCount(owner); - - if (childCount == 0) { + if (YGNodeGetChildCount(owner) == 0) { // This is an empty set. Nothing to remove. return; } - const YGNodeRef firstChild = YGNodeGetChild(owner, 0); - if (firstChild->getOwner() == owner) { - // If the first child has this node as its owner, we assume that it is - // already unique. We can now try to delete a child in this list. - if (owner->removeChild(excludedChild)) { - excludedChild->setLayout( - YGNode().getLayout()); // layout is no longer valid - excludedChild->setOwner(nullptr); - owner->markDirtyAndPropogate(); - } - return; - } - // Otherwise we have to clone the node list except for the child we're trying - // to delete. We don't want to simply clone all children, because then the - // host will need to free the clone of the child that was just deleted. - uint32_t nextInsertIndex = 0; - for (uint32_t i = 0; i < childCount; i++) { - const YGNodeRef oldChild = owner->getChild(i); - if (excludedChild == oldChild) { - // Ignore the deleted child. Don't reset its layout or owner since it is - // still valid in the other owner. However, since this owner has now - // changed, we need to mark it as dirty. - owner->markDirtyAndPropogate(); - continue; - } - YGNodeRef newChild = - owner->getConfig()->cloneNode(oldChild, owner, nextInsertIndex); - owner->replaceChild(newChild, nextInsertIndex); - newChild->setOwner(owner); - nextInsertIndex++; - } - while (nextInsertIndex < childCount) { - owner->removeChild(nextInsertIndex); - nextInsertIndex++; + // Children may be shared between parents, which is indicated by not having an + // owner. We only want to reset the child completely if it is owned + // exclusively by one node. + auto childOwner = excludedChild->getOwner(); + if (owner->removeChild(excludedChild)) { + if (owner == childOwner) { + excludedChild->setLayout({}); // layout is no longer valid + excludedChild->setOwner(nullptr); + } + owner->markDirtyAndPropogate(); } } @@ -1822,12 +1794,8 @@ static void YGZeroOutLayoutRecursivly(const YGNodeRef node) { node->setLayoutDimension(0, 0); node->setLayoutDimension(0, 1); node->setHasNewLayout(true); - node->cloneChildrenIfNeeded(); - const uint32_t childCount = YGNodeGetChildCount(node); - for (uint32_t i = 0; i < childCount; i++) { - const YGNodeRef child = node->getChild(i); - YGZeroOutLayoutRecursivly(child); - } + + node->iterChildrenAfterCloningIfNeeded(YGZeroOutLayoutRecursivly); } static float YGNodeCalculateAvailableInnerDim(