Repository navigation
Document committed semantics of hasBeenMounted and threading (#59108) - #59108
Closed
bartlomiejbloniarz wants to merge 2 commits into
Closed
bartlomiejbloniarz wants to merge 2 commits into
bartlomiejbloniarz wants to merge 2 commits into
Conversation
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 Pull Request resolved: #58890 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-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
p: Software Mansion
meta-codesync
Bot
force-pushed
the
export-D125436483
branch
from
October 15, 2026 11:32
32c1e42 to
c2da34e
Compare
|
@bartlomiejbloniarz has exported this pull request. If you are a Meta employee, you can view the originating Diff in D125436483. |
Summary: Pull Request resolved: #59108 Review follow-ups for the preallocated view cleanup in the pull model: - Document that `ShadowNodeFamily::setMounted()` and `hasBeenMounted()` refer to a node of the family being part of a committed shadow tree, which can happen before the platform mounts its view. This matches how `ShadowNode::setMounted` and `updateMountedFlag` use "mounted", so the names stay. - Document the threading requirements of the `FabricMountingManager` preallocation and mount methods. - Rename the test helper `mountCreate` to `executeCreateMutation`. Changelog: [Internal] Differential Revision: D125436483
meta-codesync
Bot
changed the title
meta-codesync
Bot
force-pushed
the
export-D125436483
branch
from
October 15, 2026 11:37
c2da34e to
c8ced57
Compare
|
This pull request has been merged in 9b7c956. |
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
1 participant
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:
Review follow-ups for the preallocated view cleanup in the pull model:
ShadowNodeFamily::setMounted()andhasBeenMounted()refer to a node of the family being part of a committed shadow tree, which can happen before the platform mounts its view. This matches howShadowNode::setMountedandupdateMountedFlaguse "mounted", so the names stay.FabricMountingManagerpreallocation and mount methods.mountCreatetoexecuteCreateMutation.Changelog: [Internal]
Differential Revision: D125436483