Renimated calls findNodeHandle with class instances, which is a slow path in React #6719
Assignees
Labels
Missing repro
This issue need minimum repro scenario
Platform: Android
This issue is specific to Android
Platform: iOS
This issue is specific to iOS
Platform: Web
This issue is specific to web
Comments
github-actions
bot
added
Platform: Android
|
Hey! 👋 The issue doesn't seem to contain a minimal reproduction. Could you provide a snack or a link to a GitHub repository under your username that reproduces the problem? |
<Animated.View /> |
This was referenced Nov 18, 2024
|
Partial fix: #6720 |
|
Thank you for highlighting another important issue! I've started to refactor how we use findNodeHandler. Here's the PR: link. I plan to adopt a similar approach to what Expo implemented here: expo/expo#33016. |
github-merge-queue bot
pushed a commit
that referenced
this issue
Nov 22, 2024
## Summary Partially addresses #6719. This is better because it doesn't enter the traversal codepath inside React. The `findNodeHandle` calculation inside the `Animated.View` itself is not so bad because it's short circuited inside for native refs (at least with `View`). Whereas passing a class instance like here always triggers the slower path in React. Regardless, it's already computed so why compute again? ## Test plan I've added console logs in our app to verify whether `animatedComponent._componentViewTag === findNodeHandle(animatedComponent)` and it always turned out true in the cases I hit. Not sure if there any cases where those could be different. Co-authored-by: Krzysztof Piaskowy <krzysztof.piaskowy@swmansion.com>
github-merge-queue bot
pushed a commit
that referenced
this issue
Dec 4, 2024
## Summary This PR addresses the issue reported at [https://github.com/software-mansion/react-native-reanimated/issues/6719](https://github.com/software-mansion/react-native-reanimated/issues/6719) and aims to: - **Unify the method used to obtain the view tag**, as there are currently several approaches. - **Avoid passing a class component to the findNodeHandler**. Instead, we'll pass a ref to the component, similar to what Expo implemented here: [https://github.com/expo/expo/pull/33016](https://github.com/expo/expo/pull/33016). - **Limit unnecessary invocations of findNodeHandler** to no more than one call per render. - **Remove the invocation of findHostInstance from Paper renderer** on the New Architecture. **Additional Remarks:** - When a class component is passed to `createAnimatedComponent`, it will still fall back to the slow path where we can get a native ref. - In `NativeEventManager`, we need to call findNodeHandler again after every render to ensure that the children of `<GestureDetector />` haven't changed their native tags. - `LayoutAnimationConfig` still uses findNodeHandler. It requires a complete refactor of their functionality to eliminate its use, and I plan to handle this in another PR. - `findHostInstance_DEPRECATED` always follows the slow path even for native refs, which is why I've implemented our own version of `findHostInstance` to optimize the happy path. Fixes #6719 Related PRs: - #6030 - #5960 - #4445 - expo/expo#33016 ## Test plan - [x] check Paper - [x] check Fabric - [x] check Web
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
findNodeHandleis a notoriously slow path in React — in fact, the function it's calling internally is literally calledfindCurrentFiberUsingSlowPath. This is quite inefficient and should be avoided whenever possible.As @kmagiera suggested, filing an issue about it.
While doing it with native refs is fine (it'll exit early before going into the slow path), calling it on a class instance triggers the bad codepath. I see Reanimated doing this in a few places:
It would be nice to audit all
findNodeHandlecallsites and remove any that pass class component instances.Steps to reproduce
See above
Snack or a link to a repository
https://github.com/software-mansion/react-native-reanimated
Reanimated version
3.16.1
React Native version
0.74.1
Platforms
Android, iOS, macOS, Web
JavaScript runtime
Hermes
Workflow
React Native
Architecture
Paper (Old Architecture)
Build type
Debug app & dev bundle
Device
iOS simulator
Device model
No response
Acknowledgements
Yes
The text was updated successfully, but these errors were encountered: