Uh oh!
There was an error while loading. Please reload this page.
[iOS] Avoid redundant recursion in RCTView accessibilityLabel - #31222
[iOS] Avoid redundant recursion in RCTView accessibilityLabel#31222mrmacete wants to merge 4 commits into
Conversation
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.
Base commit: a15a46c |
Base commit: a15a46c |
mrmacete
commented
Mar 25, 2021
Uh oh!
There was an error while loading. Please reload this page.
nfriedly
commented
Mar 25, 2021
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
commented
Apr 1, 2021
ping: @shergin what do you think about this? |
mrmacete
commented
Apr 28, 2021
ping: @shergin i think this is a small change which produces big positive impact, making RN apps easier to be tested |
ping: @shergin is there anything specific i should do to help getting this reviewed? |
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. |
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. |
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. |
mrmacete
commented
Jun 6, 2024
sad |
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. |
react-native-bot
commented
Dec 4, 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. |
react-native-bot
commented
Dec 11, 2024
This PR was closed because it has been stalled for 7 days with no activity. |
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
subviewis aRCTViewor subclass, there's no need to trigger the recursion again, because it already did implicitly when accessing the.accessibilityLabelproperty 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.