Animated: Optimize onUserDrivenAnimationEnded Deopt#48511
Closed
yungsters wants to merge 3 commits into
Closed
Conversation
facebook-github-bot
added
the
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D67872307 |
Summary: While refactoring `Animated`, I noticed that many subclasses of `AnimatedNode` override `__attach` without invoking the superclass method, even though we do this for `__detach`. In order to minimize surprise (e.g. if someone were to add logic into `AnimatedNode.prototype.__attach`), this diff updates all method overrides to invoke `super.__attach()`. Changelog: [Internal] Differential Revision: D67884975
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D67872307 |
yungsters
force-pushed
the
export-D67872307
branch
from
9fe42e9 to
a1fbc81
Compare
Summary: Currently, `AnimatedNode` implements logic to start listening to updates on the current native node if a listener is added. However, the `startListeningToAnimatedNodeValue` native module method only supports native tags for instances of `AnimatedValue`, which is a subclass of `AnimatedNode`. In fact… * On Android, [`startListeningToAnimatedNodeValue`](https://fburl.com/code/bdsl4sro) throws if the node is not an instance of `ValueAnimatedNode`. * On iOS, [`startListeningToAnimatedNodeValue`](https://fburl.com/code/hlpk1rzk) does nothing if node is not an instance of `RCTValueAnimatedNode`. As such, this refactors `AnimatedNode` to never manage this subscription for native nodes. Instead, the logic is moved into the `AnimatedValue` subclass, ensuring that we never accidentally try to `startListeningToAnimatedNodeValue` with non-`AnimatedValue` native tags. Changelog: [Internal] Differential Revision: D67884973
Summary: Pull Request resolved: facebook#48511 {D60499583} added a new`onUserDrivenAnimationEnded` listener that requires `AnimatedValue` instances to have up-to-date values reported by `onAnimatedValueUpdate` (if native driver is in use). Previously, the only way to ensure `onAnimatedValueUpdate` events were always fired to update JavaScript values in `AnimatedValue` instance was to attach a listener — even an empty one. This is exactly what D60499583 did: it traverses `props` for `AnimatedNode` instances and attaches listeners to them. However, this is really inefficient and makes the code extra convoluted. Instead, this diff changes `AnimatedValue` so that it always subscribes to changes in `__attach`, and then it cleans up the extraneous props traversal and "empty listener" logic. Changelog: [Internal] Differential Revision: D67872307
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D67872307 |
yungsters
force-pushed
the
export-D67872307
branch
from
a1fbc81 to
a2230ad
Compare
Contributor
|
This pull request has been merged in 6d67d6a. |
Collaborator
|
This pull request was successfully merged by @yungsters in 6d67d6a When will my fix make it into a release? | How to file a pick request? |
8 tasks
Contributor
|
This pull request has been reverted by 6fa4cc6. |
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
3 participants
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:
{D60499583} added a new
onUserDrivenAnimationEndedlistener that requiresAnimatedNodeinstances to have up-to-date values reported byonAnimatedValueUpdate(if native driver is in use).Previously, the only way to ensure
onAnimatedValueUpdateevents were always fired to update JavaScript values inAnimatedNodeinstance was to attach a listener — even an empty one. This is exactly what D60499583 did: it traversespropsforAnimatedNodeinstances and attaches listeners to them.However, this is really inefficient and makes the code extra convoluted. Instead, this diff changes
AnimatedNodeso that it always subscribes to changes in__attach, and then it cleans up the extraneous props traversal and "empty listener" logic.Changelog:
[Internal]
Differential Revision: D67872307