From 16d15209e55921465cf8cd0c93b11ad4d205bd69 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Thu, 14 May 2020 05:28:21 -0700 Subject: [PATCH] Remove branching for optimized differ QE Summary: Changelog: [Internal] Reviewed By: JoshuaGross Differential Revision: D21556312 fbshipit-source-id: 0d6d275de2d691cb42e5e70e5bf19bcc983cae12 --- React/Fabric/Mounting/RCTMountingManager.h | 2 - React/Fabric/Mounting/RCTMountingManager.mm | 5 +- React/Fabric/RCTSurfacePresenter.mm | 5 - .../com/facebook/react/fabric/jni/Binding.cpp | 7 +- .../com/facebook/react/fabric/jni/Binding.h | 1 - .../fabric/mounting/Differentiator.cpp | 213 ++---------------- ReactCommon/fabric/mounting/Differentiator.h | 1 - .../fabric/mounting/MountingCoordinator.cpp | 8 +- .../fabric/mounting/MountingCoordinator.h | 3 +- .../fabric/mounting/tests/MountingTest.cpp | 18 +- .../tests/ShadowTreeLifeCycleTest.cpp | 29 +-- 11 files changed, 29 insertions(+), 263 deletions(-) diff --git a/React/Fabric/Mounting/RCTMountingManager.h b/React/Fabric/Mounting/RCTMountingManager.h index eb4ca669c7a..0e8771b8a8b 100644 --- a/React/Fabric/Mounting/RCTMountingManager.h +++ b/React/Fabric/Mounting/RCTMountingManager.h @@ -26,8 +26,6 @@ NS_ASSUME_NONNULL_BEGIN @property (nonatomic, weak) id delegate; @property (nonatomic, strong) RCTComponentViewRegistry *componentViewRegistry; -@property (atomic, assign) BOOL useModernDifferentiatorMode; - /** * Schedule a mounting transaction to be performed on the main thread. * Can be called from any thread. diff --git a/React/Fabric/Mounting/RCTMountingManager.mm b/React/Fabric/Mounting/RCTMountingManager.mm index 1aa7786d859..f0c690cd6df 100644 --- a/React/Fabric/Mounting/RCTMountingManager.mm +++ b/React/Fabric/Mounting/RCTMountingManager.mm @@ -266,10 +266,7 @@ static void RNPerformMountInstructions( SystraceSection s("-[RCTMountingManager performTransaction:]"); RCTAssertMainQueue(); - auto differentiatorMode = - self.useModernDifferentiatorMode ? DifferentiatorMode::OptimizedMoves : DifferentiatorMode::Classic; - - auto transaction = mountingCoordinator->pullTransaction(differentiatorMode); + auto transaction = mountingCoordinator->pullTransaction(); if (!transaction.has_value()) { return; } diff --git a/React/Fabric/RCTSurfacePresenter.mm b/React/Fabric/RCTSurfacePresenter.mm index 2a7089d62fb..129e20edc78 100644 --- a/React/Fabric/RCTSurfacePresenter.mm +++ b/React/Fabric/RCTSurfacePresenter.mm @@ -323,11 +323,6 @@ static inline LayoutContext RCTGetLayoutContext() RCTScheduler *scheduler = [[RCTScheduler alloc] initWithToolbox:toolbox]; scheduler.delegate = self; - if (reactNativeConfig) { - _mountingManager.useModernDifferentiatorMode = - reactNativeConfig->getBool("react_fabric:enabled_optimized_moves_differ_ios"); - } - return scheduler; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index e188e9d336e..9d9a1265de6 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -271,9 +271,6 @@ void Binding::installFabricUIManager( disablePreallocateViews_ = reactNativeConfig_->getBool( "react_fabric:disabled_view_preallocation_android"); - enableOptimizedMovesDiffer_ = reactNativeConfig_->getBool( - "react_fabric:enabled_optimized_moves_differ_android"); - auto toolbox = SchedulerToolbox{}; toolbox.contextContainer = contextContainer; toolbox.componentRegistryFactory = componentsRegistry->buildRegistryFunction; @@ -583,9 +580,7 @@ void Binding::schedulerDidFinishTransaction( return; } - auto mountingTransaction = mountingCoordinator->pullTransaction( - enableOptimizedMovesDiffer_ ? DifferentiatorMode::OptimizedMoves - : DifferentiatorMode::Classic); + auto mountingTransaction = mountingCoordinator->pullTransaction(); if (!mountingTransaction.has_value()) { return; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h index 2d7128d2387..2347ee5be0e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h @@ -112,7 +112,6 @@ class Binding : public jni::HybridClass, public SchedulerDelegate { bool collapseDeleteCreateMountingInstructions_{false}; bool disablePreallocateViews_{false}; bool disableVirtualNodePreallocation_{false}; - bool enableOptimizedMovesDiffer_{false}; }; } // namespace react diff --git a/ReactCommon/fabric/mounting/Differentiator.cpp b/ReactCommon/fabric/mounting/Differentiator.cpp index 6661d542440..fd621ecfb44 100644 --- a/ReactCommon/fabric/mounting/Differentiator.cpp +++ b/ReactCommon/fabric/mounting/Differentiator.cpp @@ -254,185 +254,7 @@ static_assert( std::is_move_assignable::value, "`ShadowViewNodePair::List` must be `move assignable`."); -static void calculateShadowViewMutationsClassic( - ShadowViewMutation::List &mutations, - ShadowView const &parentShadowView, - ShadowViewNodePair::List &&oldChildPairs, - ShadowViewNodePair::List &&newChildPairs) { - // This version of the algorithm is optimized for simplicity, - // not for performance or optimal result. - - if (oldChildPairs.size() == 0 && newChildPairs.size() == 0) { - return; - } - - // Sorting pairs based on `orderIndex` if needed. - reorderInPlaceIfNeeded(oldChildPairs); - reorderInPlaceIfNeeded(newChildPairs); - - auto index = int{0}; - - // Maps inserted node tags to pointers to them in `newChildPairs`. - auto insertedPairs = TinyMap{}; - - // Lists of mutations - auto createMutations = ShadowViewMutation::List{}; - auto deleteMutations = ShadowViewMutation::List{}; - auto insertMutations = ShadowViewMutation::List{}; - auto removeMutations = ShadowViewMutation::List{}; - auto updateMutations = ShadowViewMutation::List{}; - auto downwardMutations = ShadowViewMutation::List{}; - auto destructiveDownwardMutations = ShadowViewMutation::List{}; - - // Stage 1: Collecting `Update` mutations - for (index = 0; index < oldChildPairs.size() && index < newChildPairs.size(); - index++) { - auto const &oldChildPair = oldChildPairs[index]; - auto const &newChildPair = newChildPairs[index]; - - if (oldChildPair.shadowView.tag != newChildPair.shadowView.tag) { - // Totally different nodes, updating is impossible. - break; - } - - if (oldChildPair.shadowView != newChildPair.shadowView) { - updateMutations.push_back(ShadowViewMutation::UpdateMutation( - parentShadowView, - oldChildPair.shadowView, - newChildPair.shadowView, - index)); - } - - auto oldGrandChildPairs = - sliceChildShadowNodeViewPairs(*oldChildPair.shadowNode); - auto newGrandChildPairs = - sliceChildShadowNodeViewPairs(*newChildPair.shadowNode); - calculateShadowViewMutationsClassic( - *(newGrandChildPairs.size() ? &downwardMutations - : &destructiveDownwardMutations), - oldChildPair.shadowView, - std::move(oldGrandChildPairs), - std::move(newGrandChildPairs)); - } - - int lastIndexAfterFirstStage = index; - - // Stage 2: Collecting `Insert` mutations - for (; index < newChildPairs.size(); index++) { - auto const &newChildPair = newChildPairs[index]; - - insertMutations.push_back(ShadowViewMutation::InsertMutation( - parentShadowView, newChildPair.shadowView, index)); - - insertedPairs.insert({newChildPair.shadowView.tag, &newChildPair}); - } - - // Stage 3: Collecting `Delete` and `Remove` mutations - for (index = lastIndexAfterFirstStage; index < oldChildPairs.size(); - index++) { - auto const &oldChildPair = oldChildPairs[index]; - - // Even if the old view was (re)inserted, we have to generate `remove` - // mutation. - removeMutations.push_back(ShadowViewMutation::RemoveMutation( - parentShadowView, oldChildPair.shadowView, index)); - - auto const it = insertedPairs.find(oldChildPair.shadowView.tag); - - if (it == insertedPairs.end()) { - // The old view was *not* (re)inserted. - // We have to generate `delete` mutation and apply the algorithm - // recursively. - deleteMutations.push_back( - ShadowViewMutation::DeleteMutation(oldChildPair.shadowView)); - - // We also have to call the algorithm recursively to clean up the entire - // subtree starting from the removed view. - calculateShadowViewMutationsClassic( - destructiveDownwardMutations, - oldChildPair.shadowView, - sliceChildShadowNodeViewPairs(*oldChildPair.shadowNode), - {}); - } else { - // The old view *was* (re)inserted. - // We have to call the algorithm recursively if the inserted view - // is *not* the same as removed one. - auto const &newChildPair = *it->second; - - if (newChildPair != oldChildPair) { - auto oldGrandChildPairs = - sliceChildShadowNodeViewPairs(*oldChildPair.shadowNode); - auto newGrandChildPairs = - sliceChildShadowNodeViewPairs(*newChildPair.shadowNode); - calculateShadowViewMutationsClassic( - *(newGrandChildPairs.size() ? &downwardMutations - : &destructiveDownwardMutations), - newChildPair.shadowView, - std::move(oldGrandChildPairs), - std::move(newGrandChildPairs)); - } - - // In any case we have to remove the view from `insertedPairs` as - // indication that the view was actually removed (which means that - // the view existed before), hence we don't have to generate - // `create` mutation. - insertedPairs.erase(it); - } - } - - // Stage 4: Collecting `Create` mutations - for (index = lastIndexAfterFirstStage; index < newChildPairs.size(); - index++) { - auto const &newChildPair = newChildPairs[index]; - - if (insertedPairs.find(newChildPair.shadowView.tag) == - insertedPairs.end()) { - // The new view was (re)inserted, so there is no need to create it. - continue; - } - - createMutations.push_back( - ShadowViewMutation::CreateMutation(newChildPair.shadowView)); - - calculateShadowViewMutationsClassic( - downwardMutations, - newChildPair.shadowView, - {}, - sliceChildShadowNodeViewPairs(*newChildPair.shadowNode)); - } - - // All mutations in an optimal order: - std::move( - destructiveDownwardMutations.begin(), - destructiveDownwardMutations.end(), - std::back_inserter(mutations)); - std::move( - updateMutations.begin(), - updateMutations.end(), - std::back_inserter(mutations)); - std::move( - removeMutations.rbegin(), - removeMutations.rend(), - std::back_inserter(mutations)); - std::move( - deleteMutations.begin(), - deleteMutations.end(), - std::back_inserter(mutations)); - std::move( - createMutations.begin(), - createMutations.end(), - std::back_inserter(mutations)); - std::move( - downwardMutations.begin(), - downwardMutations.end(), - std::back_inserter(mutations)); - std::move( - insertMutations.begin(), - insertMutations.end(), - std::back_inserter(mutations)); -} - -static void calculateShadowViewMutationsOptimizedMoves( +static void calculateShadowViewMutations( ShadowViewMutation::List &mutations, ShadowView const &parentShadowView, ShadowViewNodePair::List &&oldChildPairs, @@ -479,7 +301,7 @@ static void calculateShadowViewMutationsOptimizedMoves( sliceChildShadowNodeViewPairs(*oldChildPair.shadowNode); auto newGrandChildPairs = sliceChildShadowNodeViewPairs(*newChildPair.shadowNode); - calculateShadowViewMutationsOptimizedMoves( + calculateShadowViewMutations( *(newGrandChildPairs.size() ? &downwardMutations : &destructiveDownwardMutations), oldChildPair.shadowView, @@ -502,7 +324,7 @@ static void calculateShadowViewMutationsOptimizedMoves( // We also have to call the algorithm recursively to clean up the entire // subtree starting from the removed view. - calculateShadowViewMutationsOptimizedMoves( + calculateShadowViewMutations( destructiveDownwardMutations, oldChildPair.shadowView, sliceChildShadowNodeViewPairs(*oldChildPair.shadowNode), @@ -519,7 +341,7 @@ static void calculateShadowViewMutationsOptimizedMoves( createMutations.push_back( ShadowViewMutation::CreateMutation(newChildPair.shadowView)); - calculateShadowViewMutationsOptimizedMoves( + calculateShadowViewMutations( downwardMutations, newChildPair.shadowView, {}, @@ -576,7 +398,7 @@ static void calculateShadowViewMutationsOptimizedMoves( sliceChildShadowNodeViewPairs(*oldChildPair.shadowNode); auto newGrandChildPairs = sliceChildShadowNodeViewPairs(*newChildPair.shadowNode); - calculateShadowViewMutationsOptimizedMoves( + calculateShadowViewMutations( *(newGrandChildPairs.size() ? &downwardMutations : &destructiveDownwardMutations), oldChildPair.shadowView, @@ -617,7 +439,7 @@ static void calculateShadowViewMutationsOptimizedMoves( sliceChildShadowNodeViewPairs(*oldChildPair.shadowNode); auto newGrandChildPairs = sliceChildShadowNodeViewPairs(*newChildPair.shadowNode); - calculateShadowViewMutationsOptimizedMoves( + calculateShadowViewMutations( *(newGrandChildPairs.size() ? &downwardMutations : &destructiveDownwardMutations), oldChildPair.shadowView, @@ -641,7 +463,7 @@ static void calculateShadowViewMutationsOptimizedMoves( // We also have to call the algorithm recursively to clean up the // entire subtree starting from the removed view. - calculateShadowViewMutationsOptimizedMoves( + calculateShadowViewMutations( destructiveDownwardMutations, oldChildPair.shadowView, sliceChildShadowNodeViewPairs(*oldChildPair.shadowNode), @@ -677,7 +499,7 @@ static void calculateShadowViewMutationsOptimizedMoves( createMutations.push_back( ShadowViewMutation::CreateMutation(newChildPair.shadowView)); - calculateShadowViewMutationsOptimizedMoves( + calculateShadowViewMutations( downwardMutations, newChildPair.shadowView, {}, @@ -717,7 +539,6 @@ static void calculateShadowViewMutationsOptimizedMoves( } ShadowViewMutation::List calculateShadowViewMutations( - DifferentiatorMode differentiatorMode, ShadowNode const &oldRootShadowNode, ShadowNode const &newRootShadowNode) { SystraceSection s("calculateShadowViewMutations"); @@ -736,19 +557,11 @@ ShadowViewMutation::List calculateShadowViewMutations( ShadowView(), oldRootShadowView, newRootShadowView, -1)); } - if (differentiatorMode == DifferentiatorMode::Classic) { - calculateShadowViewMutationsClassic( - mutations, - ShadowView(oldRootShadowNode), - sliceChildShadowNodeViewPairs(oldRootShadowNode), - sliceChildShadowNodeViewPairs(newRootShadowNode)); - } else { - calculateShadowViewMutationsOptimizedMoves( - mutations, - ShadowView(oldRootShadowNode), - sliceChildShadowNodeViewPairs(oldRootShadowNode), - sliceChildShadowNodeViewPairs(newRootShadowNode)); - } + calculateShadowViewMutations( + mutations, + ShadowView(oldRootShadowNode), + sliceChildShadowNodeViewPairs(oldRootShadowNode), + sliceChildShadowNodeViewPairs(newRootShadowNode)); return mutations; } diff --git a/ReactCommon/fabric/mounting/Differentiator.h b/ReactCommon/fabric/mounting/Differentiator.h index 96adb5b6ed5..a7afd4f21c6 100644 --- a/ReactCommon/fabric/mounting/Differentiator.h +++ b/ReactCommon/fabric/mounting/Differentiator.h @@ -21,7 +21,6 @@ enum class DifferentiatorMode { Classic, OptimizedMoves }; * The list of mutations might be and might not be optimal. */ ShadowViewMutationList calculateShadowViewMutations( - DifferentiatorMode differentiatorMode, ShadowNode const &oldRootShadowNode, ShadowNode const &newRootShadowNode); diff --git a/ReactCommon/fabric/mounting/MountingCoordinator.cpp b/ReactCommon/fabric/mounting/MountingCoordinator.cpp index 396740c1fd9..158959d40d5 100644 --- a/ReactCommon/fabric/mounting/MountingCoordinator.cpp +++ b/ReactCommon/fabric/mounting/MountingCoordinator.cpp @@ -66,8 +66,8 @@ bool MountingCoordinator::waitForTransaction( lock, timeout, [this]() { return lastRevision_.has_value(); }); } -better::optional MountingCoordinator::pullTransaction( - DifferentiatorMode differentiatorMode) const { +better::optional MountingCoordinator::pullTransaction() + const { std::lock_guard lock(mutex_); if (!lastRevision_.has_value()) { @@ -80,9 +80,7 @@ better::optional MountingCoordinator::pullTransaction( telemetry.willDiff(); auto mutations = calculateShadowViewMutations( - differentiatorMode, - baseRevision_.getRootShadowNode(), - lastRevision_->getRootShadowNode()); + baseRevision_.getRootShadowNode(), lastRevision_->getRootShadowNode()); telemetry.didDiff(); diff --git a/ReactCommon/fabric/mounting/MountingCoordinator.h b/ReactCommon/fabric/mounting/MountingCoordinator.h index 3926f49618b..e4389d6421f 100644 --- a/ReactCommon/fabric/mounting/MountingCoordinator.h +++ b/ReactCommon/fabric/mounting/MountingCoordinator.h @@ -52,8 +52,7 @@ class MountingCoordinator final { * However, a consumer should always call it on the same thread (e.g. on the * main thread) or ensure sequentiality of mount transactions separately. */ - better::optional pullTransaction( - DifferentiatorMode differentiatorMode) const; + better::optional pullTransaction() const; /* * Blocks the current thread until a new mounting transaction is available or diff --git a/ReactCommon/fabric/mounting/tests/MountingTest.cpp b/ReactCommon/fabric/mounting/tests/MountingTest.cpp index 52361e23dc5..51734798661 100644 --- a/ReactCommon/fabric/mounting/tests/MountingTest.cpp +++ b/ReactCommon/fabric/mounting/tests/MountingTest.cpp @@ -227,8 +227,7 @@ TEST(MountingTest, testMinimalInstructionGeneration) { }*/ // Calculating mutations. - auto mutations1 = calculateShadowViewMutations( - DifferentiatorMode::OptimizedMoves, *rootNodeV1, *rootNodeV2); + auto mutations1 = calculateShadowViewMutations(*rootNodeV1, *rootNodeV2); // The order and exact mutation instructions here may change at any time. // This test just ensures that any changes are intentional. @@ -244,8 +243,7 @@ TEST(MountingTest, testMinimalInstructionGeneration) { assert(mutations1[1].index == 0); // Calculating mutations. - auto mutations2 = calculateShadowViewMutations( - DifferentiatorMode::OptimizedMoves, *rootNodeV2, *rootNodeV3); + auto mutations2 = calculateShadowViewMutations(*rootNodeV2, *rootNodeV3); // The order and exact mutation instructions here may change at any time. // This test just ensures that any changes are intentional. @@ -261,8 +259,7 @@ TEST(MountingTest, testMinimalInstructionGeneration) { assert(mutations2[1].oldChildShadowView.tag == 100); // Calculating mutations. - auto mutations3 = calculateShadowViewMutations( - DifferentiatorMode::OptimizedMoves, *rootNodeV3, *rootNodeV4); + auto mutations3 = calculateShadowViewMutations(*rootNodeV3, *rootNodeV4); // The order and exact mutation instructions here may change at any time. // This test just ensures that any changes are intentional. @@ -283,8 +280,7 @@ TEST(MountingTest, testMinimalInstructionGeneration) { assert(mutations3[3].index == 2); // Calculating mutations. - auto mutations4 = calculateShadowViewMutations( - DifferentiatorMode::OptimizedMoves, *rootNodeV4, *rootNodeV5); + auto mutations4 = calculateShadowViewMutations(*rootNodeV4, *rootNodeV5); // The order and exact mutation instructions here may change at any time. // This test just ensures that any changes are intentional. @@ -309,8 +305,7 @@ TEST(MountingTest, testMinimalInstructionGeneration) { assert(mutations4[5].newChildShadowView.tag == 102); assert(mutations4[5].index == 3); - auto mutations5 = calculateShadowViewMutations( - DifferentiatorMode::OptimizedMoves, *rootNodeV5, *rootNodeV6); + auto mutations5 = calculateShadowViewMutations(*rootNodeV5, *rootNodeV6); // The order and exact mutation instructions here may change at any time. // This test just ensures that any changes are intentional. @@ -329,8 +324,7 @@ TEST(MountingTest, testMinimalInstructionGeneration) { assert(mutations5[3].newChildShadowView.tag == 105); assert(mutations5[3].index == 3); - auto mutations6 = calculateShadowViewMutations( - DifferentiatorMode::OptimizedMoves, *rootNodeV6, *rootNodeV7); + auto mutations6 = calculateShadowViewMutations(*rootNodeV6, *rootNodeV7); // The order and exact mutation instructions here may change at any time. // This test just ensures that any changes are intentional. diff --git a/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp b/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp index ab2ad89b990..c3aef70d4ab 100644 --- a/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp +++ b/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp @@ -20,7 +20,6 @@ namespace facebook { namespace react { static void testShadowNodeTreeLifeCycle( - DifferentiatorMode differentiatorMode, uint_fast32_t seed, int treeSize, int repeats, @@ -72,8 +71,8 @@ static void testShadowNodeTreeLifeCycle( // Building an initial view hierarchy. auto viewTree = stubViewTreeFromShadowNode(*emptyRootNode); - viewTree.mutate(calculateShadowViewMutations( - differentiatorMode, *emptyRootNode, *currentRootNode)); + viewTree.mutate( + calculateShadowViewMutations(*emptyRootNode, *currentRootNode)); for (int j = 0; j < stages; j++) { auto nextRootNode = currentRootNode; @@ -99,8 +98,8 @@ static void testShadowNodeTreeLifeCycle( allNodes.push_back(nextRootNode); // Calculating mutations. - auto mutations = calculateShadowViewMutations( - differentiatorMode, *currentRootNode, *nextRootNode); + auto mutations = + calculateShadowViewMutations(*currentRootNode, *nextRootNode); // Mutating the view tree. viewTree.mutate(mutations); @@ -148,27 +147,8 @@ static void testShadowNodeTreeLifeCycle( using namespace facebook::react; -TEST(MountingTest, stableBiggerTreeFewerIterationsClassic) { - testShadowNodeTreeLifeCycle( - DifferentiatorMode::Classic, - /* seed */ 1, - /* size */ 512, - /* repeats */ 32, - /* stages */ 32); -} - -TEST(MountingTest, stableSmallerTreeMoreIterationsClassic) { - testShadowNodeTreeLifeCycle( - DifferentiatorMode::Classic, - /* seed */ 1, - /* size */ 16, - /* repeats */ 512, - /* stages */ 32); -} - TEST(MountingTest, stableBiggerTreeFewerIterationsOptimizedMoves) { testShadowNodeTreeLifeCycle( - DifferentiatorMode::OptimizedMoves, /* seed */ 0, /* size */ 512, /* repeats */ 32, @@ -177,7 +157,6 @@ TEST(MountingTest, stableBiggerTreeFewerIterationsOptimizedMoves) { TEST(MountingTest, stableSmallerTreeMoreIterationsOptimizedMoves) { testShadowNodeTreeLifeCycle( - DifferentiatorMode::OptimizedMoves, /* seed */ 0, /* size */ 16, /* repeats */ 512,