diff --git a/ReactCommon/fabric/mounting/Differentiator.cpp b/ReactCommon/fabric/mounting/Differentiator.cpp index 9f9f60207dc..6661d542440 100644 --- a/ReactCommon/fabric/mounting/Differentiator.cpp +++ b/ReactCommon/fabric/mounting/Differentiator.cpp @@ -54,7 +54,13 @@ class TinyMap final { // then we don't need to clean. cleanVector(erasedAtFront_ != numErased_); - return begin_(); + Iterator it = begin_(); + + if (it != nullptr) { + return it + erasedAtFront_; + } + + return nullptr; } inline Iterator end() { @@ -71,6 +77,10 @@ class TinyMap final { assert(key != 0); + if (begin_() == nullptr) { + return end(); + } + for (auto it = begin_() + erasedAtFront_; it != end(); it++) { if (it->first == key) { return it; @@ -86,14 +96,14 @@ class TinyMap final { } inline void erase(Iterator iterator) { - numErased_++; - // Invalidate tag. iterator->first = 0; if (iterator == begin_() + erasedAtFront_) { erasedAtFront_++; } + + numErased_++; } private: @@ -643,7 +653,7 @@ static void calculateShadowViewMutationsOptimizedMoves( } // At this point, oldTag is -1 or is in the new list, and hasn't been - // inserted or matched yet We're not sure yet if the new node is in the + // inserted or matched yet. We're not sure yet if the new node is in the // old list - generate an insert instruction for the new node. auto const &newChildPair = newChildPairs[newIndex]; insertMutations.push_back(ShadowViewMutation::InsertMutation( @@ -656,7 +666,9 @@ static void calculateShadowViewMutationsOptimizedMoves( for (auto it = newInsertedPairs.begin(); it != newInsertedPairs.end(); it++) { // Erased elements of a TinyMap will have a Tag/key of 0 - skip those - // These *should* be removed by the map, but are not always. + // These *should* be removed by the map; there are currently no KNOWN + // cases where TinyMap will do the wrong thing, but there are not yet + // any unit tests explicitly for TinyMap, so this is safer for now. if (it->first == 0) { continue; } diff --git a/ReactCommon/fabric/mounting/tests/MountingTest.cpp b/ReactCommon/fabric/mounting/tests/MountingTest.cpp index 4783516bf1c..52361e23dc5 100644 --- a/ReactCommon/fabric/mounting/tests/MountingTest.cpp +++ b/ReactCommon/fabric/mounting/tests/MountingTest.cpp @@ -77,6 +77,11 @@ TEST(MountingTest, testMinimalInstructionGeneration) { auto childD = makeNode(viewComponentDescriptor, 103, {}); auto childE = makeNode(viewComponentDescriptor, 104, {}); auto childF = makeNode(viewComponentDescriptor, 105, {}); + auto childG = makeNode(viewComponentDescriptor, 106, {}); + auto childH = makeNode(viewComponentDescriptor, 107, {}); + auto childI = makeNode(viewComponentDescriptor, 108, {}); + auto childJ = makeNode(viewComponentDescriptor, 109, {}); + auto childK = makeNode(viewComponentDescriptor, 110, {}); auto family = viewComponentDescriptor.createFamily( {10, SurfaceId(1), nullptr}, nullptr); @@ -107,6 +112,17 @@ TEST(MountingTest, testMinimalInstructionGeneration) { generateDefaultProps(viewComponentDescriptor), std::make_shared(SharedShadowNodeList{ childB, childA, childD, childF, childE, childC})}); + auto shadowNodeV7 = shadowNodeV6->clone(ShadowNodeFragment{ + generateDefaultProps(viewComponentDescriptor), + std::make_shared(SharedShadowNodeList{childF, + childE, + childC, + childD, + childG, + childH, + childI, + childJ, + childK})}); // Injecting a tree into the root node. auto rootNodeV1 = std::static_pointer_cast( @@ -139,6 +155,11 @@ TEST(MountingTest, testMinimalInstructionGeneration) { ShadowNodeFragment{ShadowNodeFragment::propsPlaceholder(), std::make_shared( SharedShadowNodeList{shadowNodeV6})})); + auto rootNodeV7 = std::static_pointer_cast( + rootNodeV6->ShadowNode::clone( + ShadowNodeFragment{ShadowNodeFragment::propsPlaceholder(), + std::make_shared( + SharedShadowNodeList{shadowNodeV7})})); // Layout and diff std::vector affectedLayoutableNodesV1{}; @@ -307,6 +328,23 @@ TEST(MountingTest, testMinimalInstructionGeneration) { assert(mutations5[3].type == ShadowViewMutation::Insert); assert(mutations5[3].newChildShadowView.tag == 105); assert(mutations5[3].index == 3); + + auto mutations6 = calculateShadowViewMutations( + DifferentiatorMode::OptimizedMoves, *rootNodeV6, *rootNodeV7); + + // The order and exact mutation instructions here may change at any time. + // This test just ensures that any changes are intentional. + // This test, in particular, ensures that a bug has been fixed: that with + // a particular sequence of inserts/removes/moves, we don't unintentionally + // create more "CREATE" mutations than necessary. + // The actual nodes that should be created in this transaction have a tag > + // 105. + assert(mutations6.size() == 25); + for (int i = 0; i < mutations6.size(); i++) { + if (mutations6[i].type == ShadowViewMutation::Create) { + assert(mutations6[i].newChildShadowView.tag > 105); + } + } } } // namespace react diff --git a/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp b/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp index d065a696663..ab2ad89b990 100644 --- a/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp +++ b/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp @@ -169,7 +169,7 @@ TEST(MountingTest, stableSmallerTreeMoreIterationsClassic) { TEST(MountingTest, stableBiggerTreeFewerIterationsOptimizedMoves) { testShadowNodeTreeLifeCycle( DifferentiatorMode::OptimizedMoves, - /* seed */ 1, + /* seed */ 0, /* size */ 512, /* repeats */ 32, /* stages */ 32); @@ -178,7 +178,7 @@ TEST(MountingTest, stableBiggerTreeFewerIterationsOptimizedMoves) { TEST(MountingTest, stableSmallerTreeMoreIterationsOptimizedMoves) { testShadowNodeTreeLifeCycle( DifferentiatorMode::OptimizedMoves, - /* seed */ 1, + /* seed */ 0, /* size */ 16, /* repeats */ 512, /* stages */ 32);