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

[iOS] Avoid redundant recursion in RCTView accessibilityLabel #31222

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mrmacete
Copy link

@mrmacete mrmacete commented Mar 23, 2021

Summary

This is a more conservative approach to address the exact same problems highlighted in this other pull request: #29801 (which i prefer to this one in the longer term, to be clear).

This change greatly improves performance (and stability) when testing ReactNative iOS apps using XCTest or Appium, which heavily rely on accessibility queries in order to interact with UI elements.

In particular, if subview is a RCTView or subclass, there's no need to trigger the recursion again, because it already did implicitly when accessing the .accessibilityLabel property in the previous line.

Doing the recursion twice is expensive in general, but in this particular situation especially because the recursion is always exhaustive so the entire subtree has been walked, and the existing code will trigger another one just right after, which will lead to the same result but at twice cost (and that's only for a single visited node in the recursion).

Now let's imagine all this expensive stuff going on when asking for a property which usually it's just a stored value, and XCTest basically does this for pretty much every element in the ui, in its own recursion of the hierarchy each time it interacts with an element. Plus the fact that all this happens on the main thread: with apps over a certain threshold of structural complexity this is indistinguishable from an infinite loop which keeps allocating memory, so the UI freezes up for a while and eventually iOS kills the app for jetsam.

This change shouldn't affect the result of -[RCTView accessibilityLabel] in any other way than improving performance.

The reasoning is the same for -[RCTViewComponentView accessibilityLabel].

Changelog

[iOS] [Fixed] - Avoid redundant recursion in -[RCTView accessibilityLabel] and -[RCTViewComponentView accessibilityLabel]

Test Plan

Nothing easy to share.

If `subview` is a `RCTView` or subclass, there's no need to trigger the
recursion again, because it already did implicitly when accessing the
`.accessibilityLabel` property in the previous line.

This greatly improves performances (and stability) when testing
ReactNative apps using XCTest or Appium, which heavily rely on
accessibility queries in order to interact with UI elements.
@mrmacete mrmacete requested a review from shergin as a code owner March 23, 2021 17:35
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 23, 2021
@analysis-bot
Copy link

analysis-bot commented Mar 23, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: a15a46c

@mrmacete mrmacete changed the title Avoid redundant recursion in RCTView accessibilityLabel [iOS] Avoid redundant recursion in RCTView accessibilityLabel Mar 23, 2021
@analysis-bot
Copy link

analysis-bot commented Mar 23, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,951,749 -658
android hermes armeabi-v7a 8,440,772 -606
android hermes x86 9,445,428 -568
android hermes x86_64 9,387,788 -524
android jsc arm64-v8a 10,683,900 -573
android jsc armeabi-v7a 10,156,034 -521
android jsc x86 10,739,089 -476
android jsc x86_64 11,321,711 -430

Base commit: a15a46c

@mrmacete
Copy link
Author

i think also @nfriedly and @rigdern should take a look a this

@nfriedly
Copy link

Oof, I forgot about my PR. Yea, this does sound like a good interim improvement.

To be honest, I hadn't even realized RN was doing two recursive lookups for each subview!

@mrmacete
Copy link
Author

mrmacete commented Apr 1, 2021

ping: @shergin what do you think about this?

@mrmacete
Copy link
Author

ping: @shergin i think this is a small change which produces big positive impact, making RN apps easier to be tested

@mrmacete
Copy link
Author

mrmacete commented May 20, 2021

ping: @shergin is there anything specific i should do to help getting this reviewed?

Copy link

github-actions bot commented Dec 8, 2023

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 8, 2023
@nfriedly
Copy link

nfriedly commented Dec 8, 2023

This should have been reviewed and merged in months ago. My team just had to go through the whole thing again for fabric.

@github-actions github-actions bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 9, 2023
Copy link

github-actions bot commented Jun 6, 2024

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jun 6, 2024
@mrmacete
Copy link
Author

mrmacete commented Jun 6, 2024

sad

@nfriedly
Copy link

nfriedly commented Jun 6, 2024

I don't think either of the people who looked at my earlier PR still work at FB :(

Not sure who should look at it now, probably someone from this list who has recent commits: https://github.com/facebook/react-native/graphs/contributors

Although, before we tag anybody, you should probably fill out their CLA, and I think the PR needs to be rebased - both of the files have moved, they're now at

(It looks like they both still need the fix, though.)

Update: After this is updated, https://github.com/joevilches and https://github.com/NickGerleman might be good folks to tag, who could hopefully either review this, or at least know who should review it.

@github-actions github-actions bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants