Repository navigation
Conversation
|
I would appreciate feedback from Android/Fabric maintainers and anyone familiar with mounting lifecycle or view recycling. Below is the implementation rationale and the remaining uncertainty. 1. Where the experiment is applied
2. What stays synchronousValidated subtree roots are detached immediately, so they are removed from the mounted hierarchy in the original transaction. The plan retires all affected tags from Only descendant hierarchy removal and view-manager deletion/recycling work are deferred. Cleanup retains the original state and view objects rather than resolving tags later. If a tag is reused before cleanup, old cleanup therefore acts on the retired object, not the replacement view. A surface stop drains retired objects before recycling pools are evicted. 3. Scheduling and budgets
Each slice stops after 256 cleanup steps or approximately 1 ms, whichever is reached first. The deadline is checked between steps; one expensive framework callback can exceed it. Remaining work waits for another draw. Hidden or detached surfaces can drain without waiting for a draw, while a visible surface never starts cleanup merely because a timeout elapsed. Observer replacement on window reattachment and synchronous/reentrant surface shutdown are covered. Admission is capped at eight tasks and 2,048 estimated operations across the queue. Each retired state reserves two operations, conservatively covering descendant removals and deletion callbacks; a single admitted plan is therefore at most 1,024 states. Exceeding admission limits falls back synchronously without flushing or corrupting previously queued work. Very large unmounts can consequently still hit the original synchronous cost. 4. Why validation was adjustedThe plan uses one tag-to-index map for DELETE membership, duplicate detection, and state indexing, plus indexed arrays for REMOVE coverage and cached parent data. This avoids repeated set construction and repeated lookups across separate passes. Validation is still linear and synchronous, and it allocates a plan. The change moves selected cleanup work; it does not make unmounting constant-time or remove validation cost. 5. Validation and performance boundaryThe PR test plan lists the local commands. After merging upstream main at eaf4c26, the focused Android JVM run passed all 145 tests in 16 suites, with zero failures, errors, or skipped tests. RNTester Debug x86_64 assembly, Android lint, Kotlin formatting, Prettier, and generated feature-flag consistency were also checked. Tests exercise both the enabled experiment and the default synchronous path, including tag reuse, emitter/state destruction, recycling, queue saturation, unsupported transactions, and draw/stop ordering. The local tracing work is Debug x86_64 evidence only. Cleanup backlog was reduced, but end-to-end frame-time improvement was not consistent, and synchronous subtree validation remains material. There is no Release physical-device benchmark supporting a performance claim yet. The flag remains off by default. 6. Feedback requested before extending this
I am happy to narrow or revise the design based on this feedback. I would like to establish the lifecycle contract and a useful benchmark before expanding eligibility or proposing that the flag be enabled. |
|
This seems to match an optimisation previously tried as RemoveDeleteTree:
This PR is narrower and improves safety by retiring tags/state/event emitters immediately, retaining object references for cleanup, and using bounded post-draw slices, but two concerns remain:
Recommendation: fix shutdown ordering and obtain Release physical-device measurements covering the unmount frame, subsequent frames, memory retention and recycling behaviour before merging. The historical removal is a reason to require stronger evidence—not proof that deferred cleanup cannot help. |
|
@javache Thanks for the historical context and the specific shutdown/eligibility concerns. I updated this PR in c7f6749. Shutdown now calls I kept the strict whole-batch admission contract and added reason-specific rejection markers, accepted-state counts, queue reservation counters and a shutdown-flush section. I then built the Release probes in Docker and ran them on an Android 15/API 35 x86_64 emulator:
The focused Android JVM run passes 149 tests in 16 suites; Fantom View passes 232 tests. Full Jest passes 6,457 tests on each of Node 22.13.0 and 24.3.0, and ESLint, Flow, type snapshot validation and generated flags pass. I also merged the upstream Kotlin build and Hermes extraction fixes and corrected the flag configuration's alphabetical ordering. Android I do not have a physical Android device. The emulator results establish functionality and synthetic-workload eligibility; representative-workload admission rates, physical-device frame windows, retained memory and recycling measurements are still missing. I am keeping the flag disabled and making no performance or merge-readiness claim. I understand that #45385 makes stronger evidence necessary before merging. The new Test All run is currently awaiting maintainer approval and has not started any jobs yet. |
Summary:
Related to #59448. Add a default-off Android Fabric experiment that detaches a
validated pure-View subtree and retires its tags, state wrappers and event
emitters immediately, then cleans up descendants in bounded UI-thread slices
after a draw callback returns. Cleanup retains object references so reused tags
cannot redirect old work to new views.
Only whole REMOVE-then-DELETE batches of exact core View classes are eligible.
Mixed/unsupported transactions and admission-limit overflow keep the synchronous
path. Shutdown evicts recycling pools before flushing retired views, preserving
the ordering introduced by #45913. Trace markers expose admission/rejection,
validation, cleanup slices and queue reservations.
This remains an experiment. There are no Release physical-device results proving
representative-workload eligibility, better frame timing or acceptable retained
memory. The flag stays disabled; the previous removal in #45385 is a reason to
require this evidence before merging or expanding scope.
Changelog:
[ANDROID] [ADDED] - Add a default-off experimental flag for deferred cleanup of validated pure-view unmount batches in Fabric.
Test Plan:
Validated in a Linux Docker container after merging main at
e57dcb3cc4.The two new recycling shutdown tests fail with the previous ordering and pass
after the fix. New fallback cases cover layout/overflow updates and 1,025 states.
one skipped, 1,684 snapshots passed on each runtime. Both runs emitted a worker
forced-exit/open-handles warning.
generated feature-flag consistency and authored Kotlin formatting passed.
recycling disabled) build successfully and pass signature/content checks.
Saved fixture and Kotlin input hashes match; configurations differ only in
the cleanup default. Source defaults and Playground are restored afterward.
These are RNTester local test APKs using the repository debug signing config.
lintDebugcomplete. The repository setsabortOnError=false: the lint report contains 63 errors, 161 warnings and twohints, with none pointing to the four authored Kotlin files. This is not a
diagnostic-free lint report.
232 tests. It emitted a one-second open-handles warning and exited normally.
completed 30 mount/unmount cycles, three background/resume cycles and three
Activity exits/reopens with no crashes/ANRs. Attached views fell from 287 to 31.
ON recorded 33 accepted 256-state unmounts and 33 unique tasks completed once;
pending tasks/reserved operations ended at zero. OFF had no experiment markers.
admissions or queued tasks, and 20 object-buffer rejections across mount/unmount
batches. These are synthetic eligibility checks; representative-workload rates
and physical-device performance remain unmeasured. Recycling was disabled in
the emulator probes; deterministic JVM tests cover recycling shutdown ordering.
Commands:
The local Gradle wrapper uses Gradle 9.4.1, JDK 17, Android SDK 37.0,
NDK 28.2.13676358, x86_64 for focused tests and arm64-v8a for Release APKs,
two workers and warnings-as-errors. The external
validation wrapper configures SDK paths, bounded heap/CPU settings and local
Maven/codeload download workarounds; it does not change repository dependencies.
Feature-flag outputs were regenerated from their source configuration.
Not run: physical-device Release performance/interaction tests, macOS/iOS E2E,
and all-architecture native suites. OFF/ON probe scripts and a Perfetto protocol
are prepared outside the PR source tree. No performance improvement is claimed.