Add experiments to skip waiting for updates of collection and table views #1311
Conversation
…view are committed in -accessibilityElements The wait was introduced in TextureGroup#1217 which blocks the main thread until updates are proccessed. We suspect this causes perf regressions accross the app and need to confirm this via an experiment.
| return YES; | ||
| if (!ASActivateExperimentalFeature(ASExperimentalSkipDefaultCellLayoutMode)) { | ||
| // Reload data is expensive, don't block main while doing so. | ||
| if (changeSet.includesReloadData) { |
There was a problem hiding this comment.
Added this check because countForAsyncLayout doesn't account for reload data.
|
LGTM |
|
🚫 CI failed with log |
nguyenhuy
changed the title
|
🚫 CI failed with log |
There was a problem hiding this comment.
Rather than introducing an experiment flag for ignoring the new default layout mode, we should probably revert the default to be what it was before that original diff (I had mistakenly thought this was the case.) @appleguy Is it expected that the default mode changed and if so, thoughts?
|
@Adlai-Holler Thanks for the review. Reverting back would be ideal actually. I was assuming that you were aware of the new default behavior. |
|
Let's do this – would you be willing to add a new mode named e.g. |
|
I'm hoping to look at this in more detail today, however, I may not have time and don't want to block this from moving forward. Running the experiment sounds good for sure. I'm a little more worried about changing the default again unless we are carefully coordinated because it would be dangerous for one of our big clients to accidentally ship that change. :) If Huy is confident testing this flag as an option, than we can go from there? |
|
@appleguy I think it's better for the new behavior to be opt-in, at least until all of us are confident that it's beneficial and everyone should use it. |
… table views under some circumstances (TextureGroup#1311)"
exp_skip_a11y_wait: An experiment that skips a wait when accessibility elements of a collection/table view is collected. The wait was introduced in Fix A11Y for horizontal collection nodes in Texture #1217. We suspect this causes some perf regressions and need to confirm via an experiment.exp_skip_default_cell_layout_mode: An experiment that skips a new default behavior ofASCellLayoutModeNone, introduced in Introduce ASCellLayoutMode #1273. The new behavior blocks main thread when 1) the collection/table view reloads data (including the initial load and any subsequent ones), 2) there is a small number of async cells to be laid out, or 3) the collection/table view hasn't had enough content to fill the visible viewport. While I agree that this new behavior may help with display latency when it's more important than FPS (i.e during app startup), I'm not sure if this is the right approach and, even if it is, we need to measure its perf implications within our app. My main concern is that blocking the main thread during app startup prevents the app from performing other equally important tasks that are intentionally scheduled during that time. The new behavior also affects apps that optimize the first page's size to fill the viewport, but doesn't necessarily fill it entirely due to various reasons. In such case, it's actually quite bad to block main thread for the second batch update, because users may (and are welcome to) interact with the content of the first page.