Repository navigation
Destroy preallocated views that the pull model never mounted (#58890) - #58890
Open
bartlomiejbloniarz wants to merge 1 commit into
Open
bartlomiejbloniarz wants to merge 1 commit into
bartlomiejbloniarz wants to merge 1 commit into
Conversation
|
@bartlomiejbloniarz has imported this pull request. If you are a Meta employee, you can view this in D123648108. |
Summary: With `enableMountingCoordinatorPullModelAndroid`, the UI thread pulls mount transactions instead of the JS thread pushing every commit. When one commit mounts a subtree and the next commit unmounts it before the UI thread pulls, the pulled transaction contains neither the Create nor the Delete for it. The views preallocated for that subtree stay in `SurfaceMountingManager`, and their tags stay in `FabricMountingManager`'s allocated view registry. The cleanup that destroys never-mounted preallocated views does not run either, because `ShadowNodeFamily` already marked the family as mounted at commit time. In a test app that repeatedly mounts and unmounts 1200 views, nearly all of them leaked as Java views and registry entries. This PR changes how preallocated views are cleaned up when the pull model is enabled: - The allocated view registry records whether each view is only preallocated, or was created by a mount (a Create or the surface root). - `ShadowNodeFamily` gets `onFamilyDestroyed`, whose callback runs for every destroyed family, and a `hasBeenMounted()` getter. `onUnmountedFamilyDestroyed` is deprecated. It keeps its behaviour by wrapping `onFamilyDestroyed` and skipping mounted families. - `destroyUnmountedShadowNode` destroys only views that are still preallocated-only. A view created by a mount is left to its Delete, which can be pulled after the family is destroyed. - Each queued preallocation holds a weak reference to its family. The drain skips views whose family is already destroyed, so it does not preallocate a view that nothing would clean up. `FabricMountingManager::preallocateShadowView` is removed, because the drain now registers and preallocates its views directly and no production code called it. ## Changelog: [ANDROID] [FIXED] - Fix preallocated views leaking when `enableMountingCoordinatorPullModelAndroid` is enabled Test Plan: - `ShadowNodeFamilyTest`: `onFamilyDestroyed` runs for both mounted and unmounted families and reports `hasBeenMounted()` correctly, and the deprecated `onUnmountedFamilyDestroyed` still skips mounted families. There is no open-source target for these gtests, so I built them with the NDK and ran them on an Android emulator. - `FabricMountingManagerInstrumentationTest` has new cases for the following. It has no Gradle target in the open-source repository, so I ran it on an Android emulator through a local androidTest source set that is not part of this PR. - the drain skipping a view whose family was destroyed (pull model); - a committed family destroying its preallocated view (pull model); - a committed family keeping a view that a mount created (pull model). With the native changes reverted, the cases for the destroyed committed family and the drain skip fail. - Release build of a test app on an Android emulator. The app mounts and unmounts a subtree of 1200 views in a loop, then idles. View counts at idle: | Configuration | Java views before → after | Registry entries before → after | | --- | --- | --- | | Pull model | 1197 → 3 | 1197 → 3 | | Pull model + Props 2.0 | 1203 → 3 | 1203 → 3 | There were no crashes or new soft exceptions. - `yarn cxx-api-validate`, `yarn format-check-cpp` and `yarn format-check-kotlin` pass. Differential Revision: D123648108 Pulled By: bartlomiejbloniarz
meta-codesync
Bot
force-pushed
the
bartlomiejbloniarz/pull-model-preallocation-leak
branch
from
October 9, 2026 11:43
b2ae295 to
4ade8a2
Compare
|
@bartlomiejbloniarz has exported this pull request. If you are a Meta employee, you can view the originating Diff in D123648108. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
With
enableMountingCoordinatorPullModelAndroid, the UI thread pulls mount transactions instead of the JS thread pushing every commit. When one commit mounts a subtree and the next commit unmounts it before the UI thread pulls, the pulled transaction contains neither the Create nor the Delete for it. The views preallocated for that subtree stay inSurfaceMountingManager, and their tags stay inFabricMountingManager's allocated view registry. The cleanup that destroys never-mounted preallocated views does not run either, becauseShadowNodeFamilyalready marked the family as mounted at commit time. In a test app that repeatedly mounts and unmounts 1200 views, nearly all of them leaked as Java views and registry entries.This PR changes how preallocated views are cleaned up when the pull model is enabled:
ShadowNodeFamilygetsonFamilyDestroyed, whose callback runs for every destroyed family, and ahasBeenMounted()getter.onUnmountedFamilyDestroyedis deprecated. It keeps its behaviour by wrappingonFamilyDestroyedand skipping mounted families.destroyUnmountedShadowNodedestroys only views that are still preallocated-only. A view created by a mount is left to its Delete, which can be pulled after the family is destroyed.FabricMountingManager::preallocateShadowViewis removed, because the drain now registers and preallocates its views directly and no production code called it.Changelog:
[ANDROID] [FIXED] - Fix preallocated views leaking when
enableMountingCoordinatorPullModelAndroidis enabledTest Plan:
ShadowNodeFamilyTest:onFamilyDestroyedruns for both mounted and unmounted families and reportshasBeenMounted()correctly, and the deprecatedonUnmountedFamilyDestroyedstill skips mounted families. There is no open-source target for these gtests, so I built them with the NDK and ran them on an Android emulator.FabricMountingManagerInstrumentationTesthas new cases for the following. It has no Gradle target in the open-source repository, so I ran it on an Android emulator through a local androidTest source set that is not part of this PR.With the native changes reverted, the cases for the destroyed committed family and the drain skip fail.
Release build of a test app on an Android emulator. The app mounts and unmounts a subtree of 1200 views in a loop, then idles. View counts at idle:
There were no crashes or new soft exceptions.
yarn cxx-api-validate,yarn format-check-cppandyarn format-check-kotlinpass.Differential Revision: D123648108
Pulled By: bartlomiejbloniarz