From b6bbbf8efabe281e45d31269eb568b4c29e012d4 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Sat, 25 Jun 2022 16:41:23 -0700 Subject: [PATCH] RemoveDeleteTree mount instruction Summary: TL;DR: For applications using JS navigation, save 50-95% of CPU during mounting phase in N>2 navigations that replace ~most of screen. During investigation of performance on the UI thread of React Native applications, I noticed that the /initial/ render of an screen for an application using JS navigation is /mostly/ consumed (on the UI thread) by tearing-down the previous View hierarchy. In one 185ms segment on the UI thread in production, 95% of the CPU time was Remove/Delete instructions and only 5% of CPU time was consumed by actually displaying the new hierarchy (this is specific to Android and also assumes that View Preallocation is being used, so post-commit work consists of Insert and UpdateLayout mutations primarily). There are /some/ cases where the C++ differ knows that we are deleting an entire subtree and therefore we could communicate this to the mounting layer. All that matters is that these Views are removed from the View hierarchy immediately; and secondarily that their memory is cleaned up ASAP, but that doesn't need to happen immediately. Some additional constraints and notes: 1) As noted in the comments, we cannot simply stop producing Remove and Delete instructions. We need to produce /both/ the new RemoveDeleteTree instruction, /and/ produce all the Remove/Delete instructions, primarily because LayoutAnimations relies heavily on these Remove/Delete instructions and certain things would break if we removed those instructions entirely. However, we can mark those Remove/Delete instructions as redundant, process them only in LayoutAnimations, and not send them to the Android mounting layer. 2) We want to make sure that View Recycling is not impacted. Since Android cannot take advantage of View Recycling until /after/ the second major render (preallocation of views will happen before any views are recycled), this doesn't impact View Recycling and we'll make sure Views are recycled whenever they are deleted. Thus, we do two things: 1) Introduce a new RemoveDeleteTree operation that can delete an entire subtree recursively as part of one operation. This allows us to avoid serializing hundreds or thousands of instructions and prevents JNI traffic. 2) Besides removing the topmost View from the View hierarchy, and ensuring it's not drawn, the full teardown and recycling of the tree can happen /after/ the paint. In some flows with JS navigation this saves us 95% of CPU during the mount phase. In the general case it is probably closer to 25-50% of CPU time that is saved and/or deferred. Changelog: [Android][Changed] Significant perf optimization to Fabric Remove/Delete operations Reviewed By: ryancat Differential Revision: D37257864 fbshipit-source-id: a7d33fc74683939965cfb98be4db7890644110b2 --- React/Fabric/Mounting/RCTMountingManager.mm | 5 + .../react/config/ReactFeatureFlags.java | 5 + .../com/facebook/react/fabric/jni/Binding.cpp | 4 + .../react/fabric/jni/FabricMountItem.cpp | 7 + .../react/fabric/jni/FabricMountItem.h | 8 +- .../fabric/jni/FabricMountingManager.cpp | 24 +- .../mounting/SurfaceMountingManager.java | 223 ++++++++++++++++++ .../mountitems/IntBufferBatchMountItem.java | 9 + .../LayoutAnimationKeyFrameManager.cpp | 10 + .../renderer/mounting/Differentiator.cpp | 40 +++- .../renderer/mounting/ShadowViewMutation.cpp | 36 ++- .../renderer/mounting/ShadowViewMutation.h | 38 ++- .../react/renderer/mounting/StubViewTree.cpp | 5 + 13 files changed, 398 insertions(+), 16 deletions(-) diff --git a/React/Fabric/Mounting/RCTMountingManager.mm b/React/Fabric/Mounting/RCTMountingManager.mm index 7acc436ec88488..ccff829a79ae17 100644 --- a/React/Fabric/Mounting/RCTMountingManager.mm +++ b/React/Fabric/Mounting/RCTMountingManager.mm @@ -104,6 +104,11 @@ static void RCTPerformMountInstructions( break; } + case ShadowViewMutation::RemoveDeleteTree: { + // TODO - not supported yet + break; + } + case ShadowViewMutation::Update: { auto &oldChildShadowView = mutation.oldChildShadowView; auto &newChildShadowView = mutation.newChildShadowView; diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index 3bc6ed6cfc009e..5110af0a4ed716 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -135,4 +135,9 @@ public class ReactFeatureFlags { * Enable prop iterator setter-style construction of Props in C++ (this flag is not used in Java). */ public static boolean enableCppPropsIteratorSetter = false; + + /** + * Allow Differentiator.cpp and FabricMountingManager.cpp to generate a RemoveDeleteTree mega-op. + */ + public static boolean enableRemoveDeleteTreeInstruction = false; } 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 f8448b5a3f2e31..be2a0f1e749381 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 @@ -451,6 +451,10 @@ void Binding::installFabricUIManager( Props::enablePropIteratorSetter; BaseTextProps::enablePropIteratorSetter = Props::enablePropIteratorSetter; + // RemoveDelete mega-op + ShadowViewMutation::PlatformSupportsRemoveDeleteTreeInstruction = + getFeatureFlagValue("enableRemoveDeleteTreeInstruction"); + auto toolbox = SchedulerToolbox{}; toolbox.contextContainer = contextContainer; toolbox.componentRegistryFactory = componentsRegistry->buildRegistryFunction; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.cpp index eadb1fb1560b33..05979307245a06 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.cpp @@ -28,6 +28,13 @@ CppMountItem CppMountItem::RemoveMountItem( int index) { return {CppMountItem::Type::Remove, parentView, shadowView, {}, index}; } +CppMountItem CppMountItem::RemoveDeleteTreeMountItem( + ShadowView const &parentView, + ShadowView const &shadowView, + int index) { + return { + CppMountItem::Type::RemoveDeleteTree, parentView, shadowView, {}, index}; +} CppMountItem CppMountItem::UpdatePropsMountItem( ShadowView const &oldShadowView, ShadowView const &newShadowView) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.h index 6117ea25a2e072..1de6263467e70a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountItem.h @@ -35,6 +35,11 @@ struct CppMountItem final { ShadowView const &shadowView, int index); + static CppMountItem RemoveDeleteTreeMountItem( + ShadowView const &parentView, + ShadowView const &shadowView, + int index); + static CppMountItem UpdatePropsMountItem( ShadowView const &oldShadowView, ShadowView const &newShadowView); @@ -64,7 +69,8 @@ struct CppMountItem final { UpdateLayout = 128, UpdateEventEmitter = 256, UpdatePadding = 512, - UpdateOverflowInset = 1024 + UpdateOverflowInset = 1024, + RemoveDeleteTree = 2048, }; #pragma mark - Fields diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp index a917a2011ca7e8..c67032f9b9a7de 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp @@ -69,6 +69,8 @@ static inline int getIntBufferSizeForType(CppMountItem::Type mountItemType) { case CppMountItem::Type::Insert: case CppMountItem::Type::Remove: return 3; // tag, parentTag, index + case CppMountItem::Type::RemoveDeleteTree: + return 3; // tag, parentTag, index case CppMountItem::Type::Delete: case CppMountItem::Type::UpdateProps: case CppMountItem::Type::UpdateState: @@ -332,15 +334,25 @@ void FabricMountingManager::executeMount( break; } case ShadowViewMutation::Remove: { - if (!isVirtual) { + if (!isVirtual && !mutation.isRedundantOperation) { cppCommonMountItems.push_back(CppMountItem::RemoveMountItem( parentShadowView, oldChildShadowView, index)); } break; } + case ShadowViewMutation::RemoveDeleteTree: { + if (!isVirtual) { + cppCommonMountItems.push_back( + CppMountItem::RemoveDeleteTreeMountItem( + parentShadowView, oldChildShadowView, index)); + } + break; + } case ShadowViewMutation::Delete: { - cppDeleteMountItems.push_back( - CppMountItem::DeleteMountItem(oldChildShadowView)); + if (!mutation.isRedundantOperation) { + cppDeleteMountItems.push_back( + CppMountItem::DeleteMountItem(oldChildShadowView)); + } break; } case ShadowViewMutation::Update: { @@ -609,6 +621,12 @@ void FabricMountingManager::executeMount( temp[2] = mountItem.index; env->SetIntArrayRegion(intBufferArray, intBufferPosition, 3, temp); intBufferPosition += 3; + } else if (mountItemType == CppMountItem::RemoveDeleteTree) { + temp[0] = mountItem.oldChildShadowView.tag; + temp[1] = mountItem.parentShadowView.tag; + temp[2] = mountItem.index; + env->SetIntArrayRegion(intBufferArray, intBufferPosition, 3, temp); + intBufferPosition += 3; } else { LOG(ERROR) << "Unexpected CppMountItem type"; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java index 96dcb33cc1852c..4722e4185e5ce9 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java @@ -51,6 +51,7 @@ import java.util.LinkedList; import java.util.Queue; import java.util.Set; +import java.util.Stack; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentLinkedQueue; import javax.annotation.Nullable; @@ -74,6 +75,11 @@ public class SurfaceMountingManager { private RootViewManager mRootViewManager; private MountItemExecutor mMountItemExecutor; + // Stack of deferred-removal tags for Views that can be + // removed asynchronously. Guaranteed to be disconnected + // from the viewport and these tags will not be reused in the future. + private final Stack mReactTagsToRemove = new Stack<>(); + // This is null *until* StopSurface is called. private Set mTagSetForStoppedSurface; @@ -561,6 +567,223 @@ public void run() { } } + @UiThread + public void removeDeleteTreeAt(final int tag, final int parentTag, int index) { + if (isStopped()) { + return; + } + + UiThreadUtil.assertOnUiThread(); + ViewState parentViewState = getNullableViewState(parentTag); + + // TODO: throw exception here? + if (parentViewState == null) { + ReactSoftExceptionLogger.logSoftException( + MountingManager.TAG, + new IllegalStateException( + "Unable to find viewState for tag: [" + parentTag + "] for removeDeleteTreeAt")); + return; + } + + if (!(parentViewState.mView instanceof ViewGroup)) { + String message = + "Unable to remove+delete a view from a view that is not a ViewGroup. ParentTag: " + + parentTag + + " - Tag: " + + tag + + " - Index: " + + index; + FLog.e(TAG, message); + throw new IllegalStateException(message); + } + + final ViewGroup parentView = (ViewGroup) parentViewState.mView; + + if (parentView == null) { + throw new IllegalStateException("Unable to find view for tag [" + parentTag + "]"); + } + + if (SHOW_CHANGED_VIEW_HIERARCHIES) { + // Display children before deleting any + FLog.e( + TAG, + "removeDeleteTreeAt: [" + tag + "] -> [" + parentTag + "] idx: " + index + " BEFORE"); + logViewHierarchy(parentView, false); + } + + ViewGroupManager viewGroupManager = getViewGroupManager(parentViewState); + + // Verify that the view we're about to remove has the same tag we expect + View view = viewGroupManager.getChildAt(parentView, index); + int actualTag = (view != null ? view.getId() : -1); + if (actualTag != tag) { + int tagActualIndex = -1; + int parentChildrenCount = parentView.getChildCount(); + for (int i = 0; i < parentChildrenCount; i++) { + if (parentView.getChildAt(i).getId() == tag) { + tagActualIndex = i; + break; + } + } + + // TODO T74425739: previously, we did not do this check and `removeViewAt` would be executed + // below, sometimes crashing there. *However*, interestingly enough, `removeViewAt` would not + // complain if you removed views from an already-empty parent. This seems necessary currently + // for certain ViewManagers that remove their own children - like BottomSheet? + // This workaround seems not-great, but for now, we just return here for + // backwards-compatibility. Essentially, if a view has already been removed from the + // hierarchy, we treat it as a noop. + if (tagActualIndex == -1) { + FLog.e( + TAG, + "removeDeleteTreeAt: [" + + tag + + "] -> [" + + parentTag + + "] @" + + index + + ": view already removed from parent! Children in parent: " + + parentChildrenCount); + return; + } + + // Here we are guaranteed that the view is still in the View hierarchy, just + // at a different index. In debug mode we'll crash here; in production, we'll remove + // the child from the parent and move on. + // This is an issue that is safely recoverable 95% of the time. If this allows corruption + // of the view hierarchy and causes bugs or a crash after this point, there will be logs + // indicating that this happened. + // This is likely *only* necessary because of Fabric's LayoutAnimations implementation. + // If we can fix the bug there, or remove the need for LayoutAnimation index adjustment + // entirely, we can just throw this exception without regression user experience. + logViewHierarchy(parentView, true); + ReactSoftExceptionLogger.logSoftException( + TAG, + new IllegalStateException( + "Tried to remove+delete view [" + + tag + + "] of parent [" + + parentTag + + "] at index " + + index + + ", but got view tag " + + actualTag + + " - actual index of view: " + + tagActualIndex)); + index = tagActualIndex; + } + + try { + viewGroupManager.removeViewAt(parentView, index); + } catch (RuntimeException e) { + // Note: `getChildCount` may not always be accurate! + // We don't currently have a good explanation other than, in situations where you + // would empirically expect to see childCount > 0, the childCount is reported as 0. + // This is likely due to a ViewManager overriding getChildCount or some other methods + // in a way that is strictly incorrect, but potentially only visible here. + // The failure mode is actually that in `removeViewAt`, a NullPointerException is + // thrown when we try to perform an operation on a View that doesn't exist, and + // is therefore null. + // We try to add some extra diagnostics here, but we always try to remove the View + // from the hierarchy first because detecting by looking at childCount will not work. + // + // Note that the lesson here is that `getChildCount` is not /required/ to adhere to + // any invariants. If you add 9 children to a parent, the `getChildCount` of the parent + // may not be equal to 9. This apparently causes no issues with Android and is common + // enough that we shouldn't try to change this invariant, without a lot of thought. + int childCount = viewGroupManager.getChildCount(parentView); + + logViewHierarchy(parentView, true); + + throw new IllegalStateException( + "Cannot remove child at index " + + index + + " from parent ViewGroup [" + + parentView.getId() + + "], only " + + childCount + + " children in parent. Warning: childCount may be incorrect!", + e); + } + + // Display children after deleting any + if (SHOW_CHANGED_VIEW_HIERARCHIES) { + final int finalIndex = index; + UiThreadUtil.runOnUiThread( + new Runnable() { + @Override + public void run() { + FLog.e( + TAG, + "removeViewAt: [" + + tag + + "] -> [" + + parentTag + + "] idx: " + + finalIndex + + " AFTER"); + logViewHierarchy(parentView, false); + } + }); + } + + // The View has been removed from the View hierarchy; now it + // and all of its children, if any, need to be deleted, recursively. + // We want to maintain the legacy ordering: delete (and call onViewStateDeleted) + // for leaf nodes, and then parents, recursively. + mReactTagsToRemove.push(tag); + runDeferredTagRemovalAndDeletion(); + } + + @UiThread + private void runDeferredTagRemovalAndDeletion() { + UiThreadUtil.runOnUiThread( + new Runnable() { + @Override + public void run() { + int deletedViews = 1; + while (!mReactTagsToRemove.empty()) { + int reactTag = mReactTagsToRemove.pop(); + ViewState thisViewState = getNullableViewState(reactTag); + if (thisViewState != null) { + View thisView = thisViewState.mView; + int numChildren = 0; + if (thisView instanceof ViewGroup) { + View nextChild = null; + // For reasons documented elsewhere in this class, getChildCount is not + // necessarily + // reliable, and so we rely instead on requesting children directly. + while ((nextChild = ((ViewGroup) thisView).getChildAt(numChildren)) != null) { + if (numChildren == 0) { + // Push tag onto the stack so we reprocess it after all children + mReactTagsToRemove.push(reactTag); + } + mReactTagsToRemove.push(nextChild.getId()); + numChildren++; + } + // Removing all at once is more efficient than removing one-by-one + ((ViewGroup) thisView).removeAllViews(); + } + if (numChildren == 0) { + deletedViews++; + mTagToViewState.remove(reactTag); + onViewStateDeleted(thisViewState); + } + // circuit breaker + // TODO: check frame time + if (deletedViews > 200) { + break; + } + } + } + + if (!mReactTagsToRemove.empty()) { + runDeferredTagRemovalAndDeletion(); + } + } + }); + } + @UiThread public void createView( @NonNull String componentName, diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java index 0f247a845c6edf..693f889651eb24 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java @@ -49,6 +49,7 @@ public class IntBufferBatchMountItem implements MountItem { static final int INSTRUCTION_UPDATE_EVENT_EMITTER = 256; static final int INSTRUCTION_UPDATE_PADDING = 512; static final int INSTRUCTION_UPDATE_OVERFLOW_INSET = 1024; + static final int INSTRUCTION_REMOVE_DELETE_TREE = 2048; private final int mSurfaceId; private final int mCommitNumber; @@ -139,6 +140,9 @@ public void execute(@NonNull MountingManager mountingManager) { surfaceMountingManager.addViewAt(parentTag, tag, mIntBuffer[i++]); } else if (type == INSTRUCTION_REMOVE) { surfaceMountingManager.removeViewAt(mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++]); + } else if (type == INSTRUCTION_REMOVE_DELETE_TREE) { + surfaceMountingManager.removeDeleteTreeAt( + mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++]); } else if (type == INSTRUCTION_UPDATE_PROPS) { surfaceMountingManager.updateProps(mIntBuffer[i++], mObjBuffer[j++]); } else if (type == INSTRUCTION_UPDATE_STATE) { @@ -221,6 +225,11 @@ public String toString() { s.append( String.format( "REMOVE [%d]->[%d] @%d\n", mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++])); + } else if (type == INSTRUCTION_REMOVE_DELETE_TREE) { + s.append( + String.format( + "REMOVE+DELETE TREE [%d]->[%d] @%d\n", + mIntBuffer[i++], mIntBuffer[i++], mIntBuffer[i++])); } else if (type == INSTRUCTION_UPDATE_PROPS) { Object props = mObjBuffer[j++]; String propsString = diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp index e1198fd368def3..c42a1317a4574c 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp +++ b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp @@ -1224,6 +1224,16 @@ void LayoutAnimationKeyFrameManager::queueFinalMutationsForCompletedKeyFrame( mutationsList.push_back(ShadowViewMutation::RemoveMutation( finalMutation.parentShadowView, prev, finalMutation.index)); break; + case ShadowViewMutation::Type::RemoveDeleteTree: + // Note: Currently, there is a guarantee that if RemoveDeleteTree + // operations are generated, we /also/ generate corresponding + // Remove/Delete operations that are marked as "redundant". + // LayoutAnimations will process the redundant operations here, and + // ignore this mega-op. In the future for perf reasons it would be + // nice to remove the redundant operations entirely but we would need + // to find a way to make the RemoveDeleteTree operation work with + // LayoutAnimations (that might not be possible). + break; case ShadowViewMutation::Type::Update: mutationsList.push_back(ShadowViewMutation::UpdateMutation( prev, finalMutation.newChildShadowView)); diff --git a/ReactCommon/react/renderer/mounting/Differentiator.cpp b/ReactCommon/react/renderer/mounting/Differentiator.cpp index 590358ed669893..41d4de1e8e5368 100644 --- a/ReactCommon/react/renderer/mounting/Differentiator.cpp +++ b/ReactCommon/react/renderer/mounting/Differentiator.cpp @@ -341,7 +341,8 @@ static void calculateShadowViewMutationsV2( ShadowViewMutation::List &mutations, ShadowView const &parentShadowView, ShadowViewNodePair::NonOwningList &&oldChildPairs, - ShadowViewNodePair::NonOwningList &&newChildPairs); + ShadowViewNodePair::NonOwningList &&newChildPairs, + bool isRecursionRedundant = false); struct OrderedMutationInstructionContainer { ShadowViewMutation::List createMutations{}; @@ -1083,7 +1084,9 @@ static void calculateShadowViewMutationsV2( ShadowViewMutation::List &mutations, ShadowView const &parentShadowView, ShadowViewNodePair::NonOwningList &&oldChildPairs, - ShadowViewNodePair::NonOwningList &&newChildPairs) { + ShadowViewNodePair::NonOwningList &&newChildPairs, + bool isRecursionRedundant) { + SystraceSection s("Differentiator::calculateShadowViewMutationsV2"); if (oldChildPairs.empty() && newChildPairs.empty()) { return; } @@ -1201,13 +1204,39 @@ static void calculateShadowViewMutationsV2( continue; } + // If we take this path, technically the operations and recursion below + // are redundant. However, some parts of the Fabric ecosystem (namely, as + // of writing this, LayoutAnimations) rely heavily on getting /explicit/ + // Remove/Delete instructions for every single node in the tree. Thus, we + // generate the "RemoveDeleteTree" instruction as well as all of the + // individual Remove/Delete operations below, but we mark those as + // redundant. The platform layer can then discard the unnecessary + // instructions. RemoveDeleteTreeMutation is a significant performance + // improvement but could be improved significantly by eliminating the need + // for any of the redundant instructions in the future. + if (ShadowViewMutation::PlatformSupportsRemoveDeleteTreeInstruction && + !isRecursionRedundant) { + mutationContainer.removeMutations.push_back( + ShadowViewMutation::RemoveDeleteTreeMutation( + parentShadowView, + oldChildPair.shadowView, + static_cast(oldChildPair.mountIndex))); + } + mutationContainer.deleteMutations.push_back( - ShadowViewMutation::DeleteMutation(oldChildPair.shadowView)); + ShadowViewMutation::DeleteMutation( + oldChildPair.shadowView, + isRecursionRedundant || + ShadowViewMutation:: + PlatformSupportsRemoveDeleteTreeInstruction)); mutationContainer.removeMutations.push_back( ShadowViewMutation::RemoveMutation( parentShadowView, oldChildPair.shadowView, - static_cast(oldChildPair.mountIndex))); + static_cast(oldChildPair.mountIndex), + isRecursionRedundant || + ShadowViewMutation:: + PlatformSupportsRemoveDeleteTreeInstruction)); // We also have to call the algorithm recursively to clean up the entire // subtree starting from the removed view. @@ -1220,7 +1249,8 @@ static void calculateShadowViewMutationsV2( oldChildPair.shadowView, sliceChildShadowNodeViewPairsFromViewNodePair( oldChildPair, innerScope), - {}); + {}, + ShadowViewMutation::PlatformSupportsRemoveDeleteTreeInstruction); } } else if (index == oldChildPairs.size()) { // If we don't have any more existing children we can choose a fast path diff --git a/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp b/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp index 437f6634dba277..2873f06e4da2f2 100644 --- a/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp +++ b/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp @@ -12,6 +12,12 @@ namespace facebook { namespace react { +/** + * Initialize static feature flags for this module. + * These flags should be treated as temporary. + */ +bool ShadowViewMutation::PlatformSupportsRemoveDeleteTreeInstruction = false; + ShadowViewMutation ShadowViewMutation::CreateMutation(ShadowView shadowView) { return { /* .type = */ Create, @@ -22,13 +28,16 @@ ShadowViewMutation ShadowViewMutation::CreateMutation(ShadowView shadowView) { }; } -ShadowViewMutation ShadowViewMutation::DeleteMutation(ShadowView shadowView) { +ShadowViewMutation ShadowViewMutation::DeleteMutation( + ShadowView shadowView, + bool isRedundantOperation) { return { /* .type = */ Delete, /* .parentShadowView = */ {}, /* .oldChildShadowView = */ std::move(shadowView), /* .newChildShadowView = */ {}, /* .index = */ -1, + /* .isRedundantOperation */ isRedundantOperation, }; } @@ -48,13 +57,28 @@ ShadowViewMutation ShadowViewMutation::InsertMutation( ShadowViewMutation ShadowViewMutation::RemoveMutation( ShadowView parentShadowView, ShadowView childShadowView, - int index) { + int index, + bool isRedundantOperation) { return { /* .type = */ Remove, /* .parentShadowView = */ std::move(parentShadowView), /* .oldChildShadowView = */ std::move(childShadowView), /* .newChildShadowView = */ {}, /* .index = */ index, + /* .isRedundantOperation */ isRedundantOperation, + }; +} + +ShadowViewMutation ShadowViewMutation::RemoveDeleteTreeMutation( + ShadowView parentShadowView, + ShadowView childShadowView, + int index) { + return { + /* .type = */ RemoveDeleteTree, + /* .parentShadowView = */ std::move(parentShadowView), + /* .oldChildShadowView = */ std::move(childShadowView), + /* .newChildShadowView = */ {}, + /* .index = */ index, }; } @@ -91,12 +115,14 @@ ShadowViewMutation::ShadowViewMutation( ShadowView parentShadowView, ShadowView oldChildShadowView, ShadowView newChildShadowView, - int index) + int index, + bool isRedundantOperation) : type(type), parentShadowView(std::move(parentShadowView)), oldChildShadowView(std::move(oldChildShadowView)), newChildShadowView(std::move(newChildShadowView)), - index(index) {} + index(index), + isRedundantOperation(isRedundantOperation) {} #if RN_DEBUG_STRING_CONVERTIBLE @@ -112,6 +138,8 @@ std::string getDebugName(ShadowViewMutation const &mutation) { return "Remove"; case ShadowViewMutation::Update: return "Update"; + case ShadowViewMutation::RemoveDeleteTree: + return "RemoveDeleteTree"; } } diff --git a/ReactCommon/react/renderer/mounting/ShadowViewMutation.h b/ReactCommon/react/renderer/mounting/ShadowViewMutation.h index 36313cdeaa197d..7beba76b25eb2a 100644 --- a/ReactCommon/react/renderer/mounting/ShadowViewMutation.h +++ b/ReactCommon/react/renderer/mounting/ShadowViewMutation.h @@ -25,6 +25,10 @@ struct ShadowViewMutation final { ShadowViewMutation() = delete; +#pragma mark - Platform feature flags + + static bool PlatformSupportsRemoveDeleteTreeInstruction; + #pragma mark - Designated Initializers /* @@ -35,7 +39,9 @@ struct ShadowViewMutation final { /* * Creates and returns an `Delete` mutation. */ - static ShadowViewMutation DeleteMutation(ShadowView shadowView); + static ShadowViewMutation DeleteMutation( + ShadowView shadowView, + bool isRedundantOperation = false); /* * Creates and returns an `Insert` mutation. @@ -49,6 +55,18 @@ struct ShadowViewMutation final { * Creates and returns a `Remove` mutation. */ static ShadowViewMutation RemoveMutation( + ShadowView parentShadowView, + ShadowView childShadowView, + int index, + bool isRedundantOperation = false); + + /* + * Creates and returns a `RemoveDelete` mutation. + * This is a signal to (for supported platforms) + * remove and delete an entire subtree with a single + * instruction. + */ + static ShadowViewMutation RemoveDeleteTreeMutation( ShadowView parentShadowView, ShadowView childShadowView, int index); @@ -62,7 +80,14 @@ struct ShadowViewMutation final { #pragma mark - Type - enum Type { Create = 1, Delete = 2, Insert = 4, Remove = 8, Update = 16 }; + enum Type { + Create = 1, + Delete = 2, + Insert = 4, + Remove = 8, + Update = 16, + RemoveDeleteTree = 32 + }; #pragma mark - Fields @@ -72,6 +97,12 @@ struct ShadowViewMutation final { ShadowView newChildShadowView = {}; int index = -1; + // RemoveDeleteTree causes many Remove/Delete operations to be redundant. + // However, we must internally produce all of them for any consumers that + // rely on explicit instructions to remove/delete every node in the tree. + // Notably (as of the time of writing this) LayoutAnimations. + bool isRedundantOperation = false; + // Some platforms can have the notion of virtual views - views that are in the // ShadowTree hierarchy but never are on the platform. Generally this is used // so notify the platform that a view exists so that we can keep EventEmitters @@ -85,7 +116,8 @@ struct ShadowViewMutation final { ShadowView parentShadowView, ShadowView oldChildShadowView, ShadowView newChildShadowView, - int index); + int index, + bool isRedundantOperation = false); }; using ShadowViewMutationList = std::vector; diff --git a/ReactCommon/react/renderer/mounting/StubViewTree.cpp b/ReactCommon/react/renderer/mounting/StubViewTree.cpp index 62d77a08a8c99e..fb1fc8e1ac9e68 100644 --- a/ReactCommon/react/renderer/mounting/StubViewTree.cpp +++ b/ReactCommon/react/renderer/mounting/StubViewTree.cpp @@ -200,6 +200,11 @@ void StubViewTree::mutate(ShadowViewMutationList const &mutations) { break; } + case ShadowViewMutation::RemoveDeleteTree: { + // TODO: do something here + break; + } + case ShadowViewMutation::Update: { STUB_VIEW_LOG({ LOG(ERROR) << "StubView: Update [" << mutation.newChildShadowView.tag