diff --git a/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt b/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt index cb790c15da..64f55f41ad 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt @@ -20,12 +20,61 @@ import org.mozilla.fenix.R import org.mozilla.fenix.ext.components import org.mozilla.fenix.helpers.HomeActivityTestRule -// BEFORE INCREASING THESE VALUES, PLEASE CONSULT WITH THE PERF TEAM. +// BEFORE CHANGING EXPECTED_* VALUES, PLEASE READ THE TEST CLASS KDOC. + +/** + * The number of times a StrictMode violation is suppressed during this start up scenario. + * Incrementing the expected value indicates a potential performance regression. + * + * One feature of StrictMode is to detect potential performance regressions and, in particular, to + * detect main thread IO. This includes network requests (which can block for multiple seconds), + * file read/writes (which generally block for tens to hundreds of milliseconds), and file stats + * (like most SharedPreferences accesses, which block for small amounts of time). Main thread IO + * should be replaced with a background operation that posts to the main thread when the IO request + * is complete. + * + * Say no to main thread IO! 🙅 + */ private const val EXPECTED_SUPPRESSION_COUNT = 19 -@Suppress("TopLevelPropertyNaming") // it's silly this would have a different naming convention b/c no const + +/** + * The number of times we call the `runBlocking` coroutine method on the main thread during this + * start up scenario. Increment the expected values indicates a potential performance regression. + * + * runBlocking indicates that we're blocking the current thread waiting for the result of another + * coroutine. While the main thread is blocked, 1) we can't handle user input and the user may feel + * Firefox is slow and 2) we can't use the main thread to continue initialization that must occur on + * the main thread (like initializing UI), slowing down start up overall. Blocking calls should + * generally be replaced with a slow operation on a background thread launching onto the main thread + * when completed. However, in a very small number of cases, blocking may be impossible to avoid. + */ private val EXPECTED_RUNBLOCKING_RANGE = 0..1 // CI has +1 counts compared to local runs: increment these together + +/** + * The number of `ConstraintLayout`s we inflate that are children of a `RecyclerView` during this + * start up scenario. Incrementing the expected value indicates a potential performance regression. + * THIS IS AN EXPERIMENTAL METRIC and we are not yet confident reducing this count will mitigate + * start up regressions. If you do not find it useful or if it's too noisy, you can consider + * removing it. + * + * ConstraintLayout is expensive to inflate (though fast to measure/layout) so we want to avoid + * creating too many of them synchronously during start up. Generally, these should be inflated + * asynchronously or replaced with cheaper layouts (if they're not too expensive to measure/layout). + * If the view hierarchy uses Jetpack Compose, switching to that is also an option. + */ private val EXPECTED_RECYCLER_VIEW_CONSTRAINT_LAYOUT_CHILDREN = 4..5 // The messaging framework is not deterministic and could add a +1 to the count + +/** + * The number of layouts we inflate during this start up scenario. Incrementing the expected value + * indicates a potential performance regression. THIS IS AN EXPERIMENTAL METRIC and we are not yet + * confident reducing this count will mitigate start up regressions. If you do not find it useful or + * if it's too noisy, you can consider removing it. + * + * Each layout inflation is suspected of having overhead (e.g. accessing each layout resource from + * disk) so suspect inflating more layouts may slow down start up. Ideally, layouts would be merged + * such that there is one inflation that includes all of the views needed on start up. + */ private val EXPECTED_NUMBER_OF_INFLATION = 14..15 // The messaging framework is not deterministic and could add a +1 to the count @@ -52,19 +101,19 @@ private val failureMsgNumberOfInflation = getErrorMessage( "will most likely mean we're adding extra work on the UI thread." ) /** - * A performance test to limit the number of StrictMode suppressions and number of runBlocking used - * on startup. - * - * This test was written by the perf team. - * - * StrictMode detects main thread IO, which is often indicative of a performance issue. - * It's easy to suppress StrictMode so we wrote a test to ensure we have a discussion - * if the StrictMode count changes. + * A performance test that attempts to minimize start up performance regressions using heuristics + * rather than benchmarking. These heuristics measure occurrences of known performance anti-patterns + * and fails when the occurrence count changes. If the change indicates a regression, we should + * re-evaluate the PR to see if we can avoid the potential regression and, if not, change the + * expected value. If it indicates an improvement, we can change the expected value. The expected + * values can be updated without consulting the performance team. * - * RunBlocking is mostly used to return values to a thread from a coroutine. However, if that - * coroutine takes too long, it can lead that thread to block every other operations. + * See `EXPECTED_*` above for explanations of the heuristics this test currently supports. * - * The perf team is code owners for this package so they should be notified when the counts are modified. + * The benefits of a heuristics-based performance test are that it is uses less CI time to get + * results so we can run it more often (e.g. for each PR) and it is less noisy than a benchmark. + * However, the downsides of this style of test is that if a heuristic value increases, it may not + * represent a real, significant performance regression. */ class StartupExcessiveResourceUseTest { @get:Rule