mirror of
https://github.com/facebook/react-native.git
synced 2025-11-01 09:14:26 +00:00
Gaspard Viot
74bdab8bd8
Summary: While investigating the root cause of app hanging on older devices in Instruments, I noticed that the heaviest stack trace was pointing to `RCTRecursiveAccessibilityLabel` in RCTView.m. <details> <summary>Heaviest stack trace in Instruments</summary> <img width="473" alt="Screenshot 2024-05-17 at 4 22 48 PM" src="https://github.com/facebook/react-native/assets/849905/fab8ed01-7a2f-4113-b2ca-04e76f25cd9d"> </details> The profiling was done on an iPad (5th generation) running iOS 16.7.4. The app is text heavy which makes the issue more visible than in RNTester for instance. ### Before <img width="854" alt="Screenshot 2024-05-17 at 4 19 46 PM" src="https://github.com/facebook/react-native/assets/849905/5e3cc7ad-299c-4814-ab4a-031c0e677b12"> It turns out that `[NSMutableString stringWithString:@""]` is initialized in every call of the recursion even though most of the time it's only used to check the length at the end and return `nil`. My change only initialize the mutable string if it's going to be used. I applied the same logic to the equivalent Fabric component. It's a small change that improved the accessibility label generation by 60ms in my case. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [IOS] [CHANGED] - Reduce memory allocations when computing accessibilityLabel Pull Request resolved: https://github.com/facebook/react-native/pull/44605 Test Plan: Running the same measurements after the change, computing the accessibility label is not the heaviest stack trace anymore. And the line by line tracing shows that `[NSMutableString stringWithString:@""]` impact has been significantly reduced. ### After <img width="769" alt="Screenshot 2024-05-17 at 4 53 04 PM" src="https://github.com/facebook/react-native/assets/849905/1ad638ac-ba7e-4dca-ac77-10df5d2dad49"> I have been using this change in production thanks to a patch-package and it effectively improved the performances when navigating between screens. I also tested in RNTester with and without Fabric. For both architectures, I made sure the return value of `RCTRecursiveAccessibilityLabel`. Interestingly, when there is no label, the Fabric implementation returns an empty string while the `RCTView.m` returns `nil`. I'm open to align both implementations to return `nil` if you believe there is no underlying reason requiring the Fabric implementation to return an empty string. Reviewed By: cipolleschi Differential Revision: D67620818 Pulled By: javache fbshipit-source-id: 1a6937075a5ff5a9ad03fbbf910d64b3884c0fe0