facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
116.49k stars 24.01k forks source link

Reduce memory allocations when computing accessibilityLabel #44605

Open sparga opened 2 weeks ago

sparga commented 2 weeks ago

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.

Heaviest stack trace in Instruments Screenshot 2024-05-17 at 4 22 48 PM

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

Screenshot 2024-05-17 at 4 19 46 PM

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:

[IOS] [CHANGED] - Reduce memory allocations when computing accessibilityLabel

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

Screenshot 2024-05-17 at 4 53 04 PM

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.