From 22a067b79e50381101cdc69cbb17bf99eb07edc8 Mon Sep 17 00:00:00 2001
From: Joshua Gross <joshuagross@fb.com>
Date: Tue, 5 Jul 2022 19:06:52 -0700
Subject: [PATCH] RemoveDeleteTree memory optimizations

Summary:
Internal tests are indicating that there's an OOM that happens during the RemoveDeleteTree worker loop while trying to expand the size of the tag stack.

In practice during manual testing I was not able to get the capacity beyond 40, but for deletions of very large trees, it's easy to imagine the size of the stack growing to at least the number of nodes in the tree being deleted. To mitigate this, we relax our requirements around the order in which onViewStateDeleted can be called and call it top-down instead of bottom up to maintain a smaller stack (see comments for more details).

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D37619259

fbshipit-source-id: 8f7af0137a868606a72fdc7bdca13c5e89b69573
---
 .../mounting/SurfaceMountingManager.java      | 24 ++++++++++++++-----
 1 file changed, 18 insertions(+), 6 deletions(-)

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 15088ad95ec8c3..3d8f8efdd478d8 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
@@ -1457,6 +1457,7 @@ private boolean haveExceededNonBatchedFrameTime(long frameTimeNanos) {
     @ThreadConfined(UI)
     public void doFrameGuarded(long frameTimeNanos) {
       int deletedViews = 0;
+      Stack<Integer> localChildren = new Stack<>();
       try {
         while (!mReactTagsToRemove.empty()) {
           int reactTag = mReactTagsToRemove.pop();
@@ -1473,6 +1474,8 @@ public void doFrameGuarded(long frameTimeNanos) {
             continue;
           }
 
+          localChildren.clear();
+
           ViewState thisViewState = getNullableViewState(reactTag);
           if (thisViewState != null) {
             View thisView = thisViewState.mView;
@@ -1491,7 +1494,7 @@ public void doFrameGuarded(long frameTimeNanos) {
               while ((nextChild = ((ViewGroup) thisView).getChildAt(numChildren)) != null) {
                 int childId = nextChild.getId();
                 childrenAreManaged = childrenAreManaged || getNullableViewState(childId) != null;
-                mReactTagsToRemove.push(nextChild.getId());
+                localChildren.push(nextChild.getId());
                 numChildren++;
               }
               // Removing all at once is more efficient than removing one-by-one
@@ -1516,13 +1519,21 @@ public void doFrameGuarded(long frameTimeNanos) {
               }
             }
             if (childrenAreManaged) {
-              // Push tag onto the stack so we reprocess it after all children
-              mReactTagsToRemove.push(reactTag);
-            } else {
-              mTagToViewState.remove(reactTag);
-              onViewStateDeleted(thisViewState);
+              // Push tags onto the stack so we process all children
+              mReactTagsToRemove.addAll(localChildren);
             }
 
+            // Immediately remove tag and notify listeners.
+            // Note that this causes RemoveDeleteTree to call onViewStateDeleted
+            // in a top-down matter (parents first) vs a bottom-up matter (leaf nodes first).
+            // Hopefully this doesn't matter but you should ensure that any custom
+            // onViewStateDeleted logic is resilient to both semantics.
+            // In the initial version of RemoveDeleteTree we attempted to maintain
+            // the bottom-up event listener behavior but this causes additional
+            // memory pressure as well as complexity.
+            mTagToViewState.remove(reactTag);
+            onViewStateDeleted(thisViewState);
+
             // Circuit breaker: after processing every N tags, check that we haven't
             // exceeded the max allowed time. Since we don't know what other work needs
             // to happen on the UI thread during this frame, and since this works tends to be
@@ -1542,6 +1553,7 @@ public void doFrameGuarded(long frameTimeNanos) {
           // other mounting instructions have been executed, all in-band Remove
           // instructions have already had a chance to execute here.
           mErroneouslyReaddedReactTags.clear();
+          mReactTagsToRemove.clear();
         }
       }
     }