Construct the shared animation backend after the UIManager has a delegate - #57817
Open
zeyap wants to merge 2 commits into
Open
Construct the shared animation backend after the UIManager has a delegate#57817zeyap wants to merge 2 commits into
zeyap wants to merge 2 commits into
Conversation
Summary: `AnimationBackendCommitHook::shadowTreeWillCommit` and `AnimatedMountingOverrideDelegate::pullTransaction` are the two mutually exclusive implementations of "apply animated props on the way to the screen": the former runs when the shared animated backend is enabled, the latter when it is not. Neither was instrumented, so the cost of one could not be compared against the cost of the other. Add a `TraceSection` to each, with the size of the work as an argument — `numAnimatedFamilies` for the commit hook (it scales with the number of animated nodes on the surface) and `numMutations` for the mounting override delegate (it scales with the size of the mount transaction). The commit hook's section starts after the early-out for surfaces with no animated families, so an idle surface still costs nothing and does not emit a slice. `animationbackend` gains a direct dependency on `cxxreact:bridge` for `TraceSection.h`. Changelog: [Internal] Differential Revision: D114759537
…gate
Summary:
## Changelog:
[Internal] - Construct the shared animation backend after the UIManager has a delegate
`AnimationBackend`'s constructor calls `UIManager::addOnSurfaceStartCallback` so that `AnimatedPropsRegistry::initializeSurface` runs for each surface as it starts. That call is a no-op unless the `UIManager` already has a delegate:
```
void UIManager::addOnSurfaceStartCallback(
UIManagerDelegate::OnSurfaceStartCallback&& callback) {
if (delegate_ != nullptr) {
delegate_->uiManagerShouldAddOnSurfaceStartCallback(std::move(callback));
}
}
```
`Scheduler`'s constructor was building the backend immediately after constructing the `UIManager` and roughly seventy lines before `uiManager->setDelegate(this)`, so the callback was dropped every time and `Scheduler::onSurfaceStartCallbacks_` never received it. `AnimatedPropsRegistry::update` skips surfaces missing from `surfaceContexts_`, so correctness was left resting on `getMap()` default-constructing the entry via `operator[]` — that is, on a commit hook happening to run before the first animated update on a freshly started surface. When that ordering does not hold, early animated updates on a new surface are dropped.
Move the backend construction to just after `setDelegate`, and add a comment recording the ordering constraint. This is still well before any surface can start, so the registry is populated for every surface from the first frame.
The neighbouring `getShadowTreeRegistry().enumerate(...)` in the same constructor is also dead today — the `UIManager` is two lines old and its registry is necessarily empty — but it is left in place deliberately. It mirrors the enumerate-then-register pattern in `NativeAnimatedNodesManagerProvider` and `ViewTransitionModule`, both of which are constructed lazily and do need it, and it keeps `AnimationBackend`'s constructor correct if it is ever moved to a lazy call site.
Differential Revision: D114759538
meta-cla
Bot
added
the
CLA Signed
|
@zeyap has exported this pull request. If you are a Meta employee, you can view the originating Diff in D114759538. |
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:
Changelog:
[Internal] - Construct the shared animation backend after the UIManager has a delegate
AnimationBackend's constructor callsUIManager::addOnSurfaceStartCallbackso thatAnimatedPropsRegistry::initializeSurfaceruns for each surface as it starts. That call is a no-op unless theUIManageralready has a delegate:Scheduler's constructor was building the backend immediately after constructing theUIManagerand roughly seventy lines beforeuiManager->setDelegate(this), so the callback was dropped every time andScheduler::onSurfaceStartCallbacks_never received it.AnimatedPropsRegistry::updateskips surfaces missing fromsurfaceContexts_, so correctness was left resting ongetMap()default-constructing the entry viaoperator[]— that is, on a commit hook happening to run before the first animated update on a freshly started surface. When that ordering does not hold, early animated updates on a new surface are dropped.Move the backend construction to just after
setDelegate, and add a comment recording the ordering constraint. This is still well before any surface can start, so the registry is populated for every surface from the first frame.The neighbouring
getShadowTreeRegistry().enumerate(...)in the same constructor is also dead today — theUIManageris two lines old and its registry is necessarily empty — but it is left in place deliberately. It mirrors the enumerate-then-register pattern inNativeAnimatedNodesManagerProviderandViewTransitionModule, both of which are constructed lazily and do need it, and it keepsAnimationBackend's constructor correct if it is ever moved to a lazy call site.Differential Revision: D114759538