From b1c749075d358db1266507c3f146d907f780929b 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 --- tests/YGPersistenceTest.cpp | 41 +++++++++++++++++- yoga/YGNode.cpp | 25 +---------- yoga/YGNode.h | 16 +++++++ yoga/Yoga.cpp | 84 ++++++++++++------------------------- 4 files changed, 82 insertions(+), 84 deletions(-) diff --git a/tests/YGPersistenceTest.cpp b/tests/YGPersistenceTest.cpp index c9d94e89..69bd59b1 100644 --- a/tests/YGPersistenceTest.cpp +++ b/tests/YGPersistenceTest.cpp @@ -6,6 +6,7 @@ */ #include #include +#include TEST(YogaTest, cloning_shared_root) { const YGConfigRef config = YGConfigNew(); @@ -104,7 +105,7 @@ TEST(YogaTest, cloning_shared_root) { YGConfigFree(config); } -TEST(YogaTest, mutating_children_of_a_clone_clones) { +TEST(YogaTest, mutating_children_of_a_clone_clones_only_after_layout) { const YGConfigRef config = YGConfigNew(); const YGNodeRef root = YGNodeNewWithConfig(config); @@ -129,7 +130,7 @@ TEST(YogaTest, mutating_children_of_a_clone_clones) { ASSERT_EQ(1, YGNodeGetChildCount(root2)); ASSERT_EQ(2, YGNodeGetChildCount(root3)); ASSERT_EQ(root3_child1, YGNodeGetChild(root3, 1)); - ASSERT_NE(YGNodeGetChild(root2, 0), YGNodeGetChild(root3, 0)); + ASSERT_EQ(YGNodeGetChild(root2, 0), YGNodeGetChild(root3, 0)); const YGNodeRef root4 = YGNodeClone(root3); ASSERT_EQ(root3_child1, YGNodeGetChild(root4, 1)); @@ -137,7 +138,12 @@ TEST(YogaTest, mutating_children_of_a_clone_clones) { YGNodeRemoveChild(root4, root3_child1); ASSERT_EQ(2, YGNodeGetChildCount(root3)); ASSERT_EQ(1, YGNodeGetChildCount(root4)); + ASSERT_EQ(YGNodeGetChild(root3, 0), YGNodeGetChild(root4, 0)); + + YGNodeCalculateLayout(root4, YGUndefined, YGUndefined, YGDirectionLTR); ASSERT_NE(YGNodeGetChild(root3, 0), YGNodeGetChild(root4, 0)); + YGNodeCalculateLayout(root3, YGUndefined, YGUndefined, YGDirectionLTR); + ASSERT_NE(YGNodeGetChild(root2, 0), YGNodeGetChild(root3, 0)); YGNodeFreeRecursive(root4); YGNodeFreeRecursive(root3); @@ -245,3 +251,34 @@ TEST(YogaTest, cloning_and_freeing) { ASSERT_EQ(initialInstanceCount, YGNodeGetInstanceCount()); } + +TEST(YogaTest, mixed_shared_and_owned_children) { + // Don't try this at home! + + YGNodeRef root0 = YGNodeNew(); + YGNodeRef root1 = YGNodeNew(); + + YGNodeRef root0_child0 = YGNodeNew(); + YGNodeRef root0_child0_0 = YGNodeNew(); + YGNodeInsertChild(root0, root0_child0, 0); + YGNodeInsertChild(root0_child0, root0_child0_0, 0); + + YGNodeRef root1_child0 = YGNodeNew(); + YGNodeRef root1_child2 = YGNodeNew(); + YGNodeInsertChild(root1, root1_child0, 0); + YGNodeInsertChild(root1, root1_child2, 1); + + auto children = root1->getChildren(); + children.insert(children.begin() + 1, root0_child0); + root1->setChildren(children); + auto secondChild = YGNodeGetChild(root1, 1); + ASSERT_EQ(secondChild, YGNodeGetChild(root0, 0)); + ASSERT_EQ(YGNodeGetChild(secondChild, 0), YGNodeGetChild(root0_child0, 0)); + + YGNodeCalculateLayout(root1, YGUndefined, YGUndefined, YGDirectionLTR); + secondChild = YGNodeGetChild(root1, 1); + ASSERT_NE(secondChild, YGNodeGetChild(root0, 0)); + ASSERT_EQ(YGNodeGetOwner(secondChild), root1); + ASSERT_NE(YGNodeGetChild(secondChild, 0), YGNodeGetChild(root0_child0, 0)); + ASSERT_EQ(YGNodeGetOwner(YGNodeGetChild(secondChild, 0)), secondChild); +} diff --git a/yoga/YGNode.cpp b/yoga/YGNode.cpp index 4fcef90e..fedb3b9e 100644 --- a/yoga/YGNode.cpp +++ b/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/yoga/YGNode.h b/yoga/YGNode.h index 63087fe4..3c2a9903 100644 --- a/yoga/YGNode.h +++ b/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/yoga/Yoga.cpp b/yoga/Yoga.cpp index 290e50a8..22c2a038 100644 --- a/yoga/Yoga.cpp +++ b/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(