Skip to content

Commit

Permalink
RemoveDeleteTree memory optimizations
Browse files Browse the repository at this point in the history
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
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed Jul 6, 2022
1 parent cf2e27c commit 22a067b
Showing 1 changed file with 18 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -1473,6 +1474,8 @@ public void doFrameGuarded(long frameTimeNanos) {
continue;
}

localChildren.clear();

ViewState thisViewState = getNullableViewState(reactTag);
if (thisViewState != null) {
View thisView = thisViewState.mView;
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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();
}
}
}
Expand Down

0 comments on commit 22a067b

Please sign in to comment.