Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce memory allocations when computing accessibilityLabel #44605

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sparga
Copy link

@sparga sparga commented May 18, 2024

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.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants